#421 induced strange behaviour for first transition (not_most_recent_value & db_null methods)

[Paul-Yves]

When doing a transition, I now (after updating to 8.0.0) have a "no implicit conversion of nil into String" error. I tracked it down to the first transition, inside module Statesman::Adapters::ActiveRecord when not_most_recent_value is null, db_null returns nil when it used to return "null" before this PR.

I managed to patch this by reverting to the old implementation of Arel::Nodes::SqlLiteral.new("NULL") or by copying the methods above with a ::ActiveRecord::Base.connection.quote(type_cast(nil)).

The first solutions seemed the most logical since the comment of type_cast says Type casting against a column is deprecated and will be removed in Rails 6.2. and the original implementation of db_null did not type cast against a column.

However I am not sure to fully understand the situation so I would like to have your inputs before submitting a PR

[thom-oman]

Hey @Paul-Yves, thanks for getting in touch.

Yeah, this looks like this was me! I think you may be right with your patch, but I will take a look and we'll see how we can fix this.

Was there anything in particular you were doing that caused the issue? Would you be able to provide anything that helps me reproduce it?

[Paul-Yves]

Thank you for answering, I'll try my best :
We are in a rails 6.0.3.4 app, ruby 2.7.2. The state machine is very straightforward, something like :

class OrderWorkflowStateMachine
  include Statesman::Machine

  state :new, initial: true
  state :state_1
  state :state_2
  state :state_3

  states.each do |state|
    transition from: state, to: states - [state]
  end
end

and I used a breakpoint when doing transition_to(:state_1) in a newly created instance (so in the new state without transition). I reproduced the issue both in the specs (rspec) and using the app in my local development mode.

Our database is PostgreSQL 10.15 and we use the pg gem.

Relevant part of our database scheme would be

CREATE TABLE public.order_workflow_transitions (
    id bigint NOT NULL,
    to_state character varying NOT NULL,
    metadata text,
    sort_key integer NOT NULL,
    order_id bigint NOT NULL,
    most_recent boolean,
    created_at timestamp without time zone NOT NULL,
    updated_at timestamp without time zone NOT NULL
);

[Paul-Yves]

Any news about this ? Couldn't we simply revert to the previous implementation of db_null, since it was not a type casting, the change seems irrelevant to the original issue (deprecation warning)

[thom-oman]

hey @Paul-Yves was about to message apologising for not being able to look at this, it's been a busy week.

Yes, I'm pretty sure I know the fix, the fix isn't the issue here, my concern is that this wasn't caught by tests. So I want to reproduce it so I can confirm that it fixes it, not only for postgres but for mysql as well.

[Paul-Yves]

Ok, thank you very much for your dedication.

[thom-oman]

Quick update, I've managed to reproduce the issue with the script below, and the breaking is case is Postgres + most_recent column being nullable. I tried both fixes that you mentioned but only one would fix both PG and mysql, which you can see in this rough PR.

Here is the script I used:

require_relative "spec_helper"

MIGRATION_CLASS = if Rails.version.split(".").map(&:to_i).first >= 5
                    migration_version = ActiveRecord::Migration.current_version
                    ActiveRecord::Migration[migration_version]
                  else
                    ActiveRecord::Migration
                  end

class MyStateMachineNew
  include Statesman::Machine

  state :created, initial: true
  state :state_1
  state :state_2
  state :state_3

  states.each do |state|
    transition from: state, to: states - [state]
  end
end

class MyNewActiveRecordModel < ActiveRecord::Base
  self.table_name = 'my_active_record_models'

  has_many :my_active_record_model_transitions, autosave: false, foreign_key: :my_active_record_model_id
  alias_method :transitions, :my_active_record_model_transitions

  delegate :can_transition_to?, :transition_to!, :transition_to, :last_transition,
           :current_state, :in_state?, :history, to: :state_machine

  def state_machine
    @state_machine ||= MyStateMachineNew.new(
      self, transition_class: MyNewActiveRecordModelTransitionNew
    )
  end
end

class MyNewActiveRecordModelTransitionNew < ActiveRecord::Base
  self.table_name = 'my_active_record_model_transitions'

  include Statesman::Adapters::ActiveRecordTransition

  belongs_to :my_active_record_model
  serialize :metadata, JSON
end

class CreateMyNewActiveRecordModelMigration < MIGRATION_CLASS
  def change
    create_table :my_active_record_models do |t|
      t.string :current_state
      t.timestamps null: false
    end
  end
end

class CreateMyNewActiveRecordModelTransitionNewMigration < MIGRATION_CLASS
  def change
    create_table :my_active_record_model_transitions do |t|
      t.string  :to_state
      t.integer :my_active_record_model_id
      t.integer :sort_key
      t.text :metadata, default: "{}"
      t.boolean :most_recent # it's this line that is the cause
      t.timestamps null: false
      t.date :updated_on
    end

    add_index :my_active_record_model_transitions,
              %i[my_active_record_model_id sort_key],
              unique: true, name: "sort_key_index"

    add_index :my_active_record_model_transitions,
              %i[my_active_record_model_id most_recent],
              unique: true,
              name: "index_my_active_record_model_transitions_parent_latest"
  end
end

RSpec.describe "reproduce issues" do
  before do
    Statesman.configure do
      storage_adapter(Statesman::Adapters::ActiveRecord)
    end
    CreateMyNewActiveRecordModelMigration.migrate(:up)
    CreateMyNewActiveRecordModelTransitionNewMigration.migrate(:up)
    MyNewActiveRecordModelTransitionNew.reset_column_information
  end

  after do
    CreateMyNewActiveRecordModelTransitionNewMigration.migrate(:down)
    CreateMyNewActiveRecordModelMigration.migrate(:down)
  end

  it 'creates transition' do
    m = MyNewActiveRecordModel.create
    m.transition_to!(:state_1)
  end
end

[thom-oman]

hey @Paul-Yves closing this as the PR has been merged, will bump the version shortly

[Paul-Yves]

Thank you for the fix