:y cannot be overwritten unless specified in factory #285

Closed
mifrai opened this Issue Jan 28, 2012 · 13 comments

Projects

None yet

3 participants

@mifrai
mifrai commented Jan 28, 2012

On Factory Girl 2.5.0

The :y attribute cannot be overwritten during build unless it is specified in the factory

Minimal test case

class YPos
  attr_accessor :y
end

factory :y_pos do
  # uncomment the following and it will work
  # y nil
end

> FactoryGirl.build(:y_pos, y: 2).y
=> nil
@joshuaclayton joshuaclayton added a commit that referenced this issue Jan 29, 2012
@joshuaclayton joshuaclayton Undefine most private methods on Evaluator
When method_missing is used with Evaluator, certain methods (namely
private methods from Object) may register as being defined, which we
don't want.

Closes #279, #285
5f95b96
@rymai
rymai commented Jan 31, 2012

Hi, I still got this issue (with a different error I believe) when using FactoryGirl from git:

class Stat
  include Mongoid::Document

  field :t, type: String
end

factory :stat do
  # uncomment the following and it will work
  # t '123abc'
end

> FactoryGirl.build(:stat, t: '123abc')
=> ArgumentError:
       wrong number of arguments (0 for 1)

Any idea why?

@joshuaclayton
Member

@rymai Could you paste the rest of the backtrace please?

@rymai
rymai commented Jan 31, 2012

Here you go:

1.9.3p0 :004 > Factory.build(:stat, t: '123456')
ArgumentError: wrong number of arguments (0 for 1)
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/gems/RedCloth-4.2.9/lib/redcloth/erb_extension.rb:16:in `textilize'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/attribute_assigner.rb:36:in `get'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/attribute_assigner.rb:14:in `block (2 levels) in object'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/attribute_assigner.rb:13:in `each'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/attribute_assigner.rb:13:in `block in object'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/attribute_assigner.rb:12:in `tap'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/attribute_assigner.rb:12:in `object'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/proxy/build.rb:10:in `result'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/factory.rb:48:in `run'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/syntax/methods.rb:148:in `run_factory_girl_proxy'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/syntax/methods.rb:40:in `build'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/bundler/gems/factory_girl-850116da1107/lib/factory_girl/syntax/vintage.rb:119:in `build'
    from (irb):4
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/gems/railties-3.1.3/lib/rails/commands/console.rb:45:in `start'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/gems/railties-3.1.3/lib/rails/commands/console.rb:8:in `start'
    from /Users/remy/.rvm/gems/ruby-1.9.3-p0/gems/railties-3.1.3/lib/rails/commands.rb:40:in `<top (required)>'
    from script/rails:6:in `require'
    from script/rails:6:in `<main>'1.9.3p0 :005 > 

Hum, didn't run it in the console before (only through RSpec) so I didn't see the entire backtrace. I think there's something wrong with RedCloth... What do you think?

@joshuaclayton
Member

That's what it looks like... hmm. Could you run Object.public_instance_methods.sort from a Rails console and let me know if :t is in the list? Thanks!

@rymai
rymai commented Feb 1, 2012

Hi,

no it doesn't look like it is:

1.9.3p0 > Object.public_instance_methods.include?(:t)
 => false 
@rymai
rymai commented Feb 1, 2012

Here is some interesting code from RedCloth-4.2.9/lib/redcloth/erb_extension.rb:

class ERB
  module Util

    def textilize( s )
      if s && s.respond_to?(:to_s)
        RedCloth.new( s.to_s ).to_html
      end
    end

    alias t textilize
    module_function :t
    module_function :textilize

  end
end

So, excluding from the :test group in my Gemfile obviously solves the issue, but not all specs pass now if I do so.

@joshuaclayton
Member

I'll take a stab at undefining most public methods too and see if that helps at all.

@rymai
rymai commented Feb 1, 2012

Ok, thanks in advance!

@joshuaclayton
Member

So, after digging in, it looks like redcloth includes these methods on Object (see https://github.com/jgarber/redcloth/blob/master/lib/redcloth.rb#L43). module_function's behavior is such that, when the module is included in a class, it becomes a private method. So, what seems to be happening is even though FG's Evaluator is undefining most of the private methods, this must end up getting run afterwards. So, I'm not sure what to do in this case.

@joshuaclayton
Member

Actually, I figured out a better solution. Instead of messing with figuring out how to undefine methods so method_missing could process it, I decided to define each override as a method on the individual instance. That way, no matter when methods (public or private) get added to Object, the overrides will always take precedence.

@joshuaclayton joshuaclayton added a commit that referenced this issue Feb 2, 2012
@joshuaclayton joshuaclayton Overrides become methods defined on each instance of the evaluator
There were some big issues with trying to undefine specific methods on
the Evaluator. After investigating maybe inheriting from BasicObject (or
ActiveSupport::BasicObject since BasicObject is 1.9+), that turned out
to be too much of a pain because it undefines almost everything,
including class and a handful of other methods necessary for Evaluator
to work properly.

The second solution was to undefine all private methods. The problem is,
when other libraries defining methods (private or otherwise) on Object
are loaded *after* factory girl, those methods get added and Evaluator
sees those methods on Object. So, that solution didn't work either.

This commit removes undefining methods (the sole reason of which was to
capture with method_missing and process ourselves, returning the
override or cached value) and instead introduces a new concept -
iterating over each override and defining it as a method on the
evaluator INSTANCE. This means that overrides don't collide because
they're on the instance and we don't have to worry about undefining
methods so that method_missing kicks in. Although not nececessarily as
performant, this is the most stable and guaranteed way to get this to
work, because the overrides are applied to each instance at runtime.

Closes #279, #285 (again)
d4045fe
@rymai
rymai commented Feb 2, 2012

Hi Joshua,

Thanks for that, I've checked out the branch and it seems to work well now! :)

@joshuaclayton joshuaclayton added a commit that referenced this issue Feb 3, 2012
@joshuaclayton joshuaclayton Overrides become methods defined on each instance of the evaluator
There were some big issues with trying to undefine specific methods on
the Evaluator. After investigating maybe inheriting from BasicObject (or
ActiveSupport::BasicObject since BasicObject is 1.9+), that turned out
to be too much of a pain because it undefines almost everything,
including class and a handful of other methods necessary for Evaluator
to work properly.

The second solution was to undefine all private methods. The problem is,
when other libraries defining methods (private or otherwise) on Object
are loaded *after* factory girl, those methods get added and Evaluator
sees those methods on Object. So, that solution didn't directly work either.

This commit removes undefining methods (the sole reason of which was to
capture with method_missing and process ourselves, returning the
override or cached value) and instead introduces a new concept -
iterating over each override and defining it as a method on the
evaluator INSTANCE. This means that overrides don't collide because
they're on the instance and we don't have to worry about undefining
methods so that method_missing kicks in. This is the most stable and
guaranteed way to get this to work because the overrides are applied to
each instance at runtime.

Closes #279, #285
f50550c
@joshuaclayton
Member

Just cut version 2.5.1 of the gem which should resolve this. If you have any more problems, please open a new ticket. Thanks!

@rymai
rymai commented Feb 4, 2012

Thanks that's great! I'll let you know if I find anything else! ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment