Skip to content

Assign attribute_lists in a single write - #1837

Open
stefanlew-tractive wants to merge 1 commit into
thoughtbot:mainfrom
stefanlew-tractive:assign-attribute-lists-in-one-write
Open

Assign attribute_lists in a single write#1837
stefanlew-tractive wants to merge 1 commit into
thoughtbot:mainfrom
stefanlew-tractive:assign-attribute-lists-in-one-write

Conversation

@stefanlew-tractive

Copy link
Copy Markdown

EvaluatorClassDefiner#evaluator_class writes attribute_lists, then reads it back on the next
line:

klass.attribute_lists ||= []
klass.attribute_lists += [@attributes]

attribute_lists is a class_attribute, so the write goes through ActiveSupport's generated writer
and the second statement re-reads through the generated reader. If that read does not observe the
write, + is called on nil:

NoMethodError: undefined method '+' for nil
  lib/factory_bot/evaluator_class_definer.rb:16:in 'block in evaluator_class'

Reproduction against 6.6.0:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "factory_bot", "~> 6.6"
end

require "factory_bot"
require "minitest/autorun"

class EvaluatorClassDefinerTest < Minitest::Test
  def test_evaluator_class_does_not_read_attribute_lists_back
    parent_class = Class.new(FactoryBot::Evaluator)
    parent_class.define_singleton_method(:attribute_lists) { nil }
    parent_class.define_singleton_method(:attribute_lists=) { |value| value }

    FactoryBot::EvaluatorClassDefiner.new([], parent_class).evaluator_class
  end
end

This collapses the pair into a single read-modify-write, so the value is never read back after being
written. Behaviour is unchanged: a fresh subclass still starts from an empty list, and a child
evaluator still accumulates its parent's lists. The added spec fails on main and passes with the
change.

EvaluatorClassDefiner#evaluator_class writes attribute_lists and then
reads it back in the next statement. attribute_lists is a
class_attribute, so the write goes through ActiveSupport's generated
writer and the read comes back through the generated reader. When the
value is not observable on that second read, `+` is called on nil:

  NoMethodError: undefined method '+' for nil
    lib/factory_bot/evaluator_class_definer.rb:16:in 'block in evaluator_class'

Collapsing the pair into one read-modify-write removes the window.
Behaviour is unchanged: a fresh subclass still starts from an empty
list, and a child evaluator still accumulates its parent's lists.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant