Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

conflict with active_support :try #78

Closed
andreaseger opened this issue Apr 19, 2016 · 6 comments
Closed

conflict with active_support :try #78

andreaseger opened this issue Apr 19, 2016 · 6 comments

Comments

@andreaseger
Copy link

The internal use of the method try in combination with method_missing runs into an error as soon as active_support with it's try core_ext is required.

require 'bundler/inline'
gemfile true do
  gem 'dry-types'
  gem 'activesupport', require: false
end

include Dry::Types.module

CustomType = Strict::String.enum("val1", "val2")
CustomStringArray = Strict::Array.member(CustomType)

p CustomStringArray.call(["val1"])

require "active_support/core_ext/object/try"

p CustomStringArray.call(["val1"])

I'm not sure the method_missing cascade is used similar in other scenarios.

Given how common active_support is even outside rails applications this seems like a issue that should be fixed. Without to much insight into the code renaming the try method to something unique or changing how the method_missing cascade is used here might be an option so solve this.

@tak1n
Copy link

tak1n commented Apr 19, 2016

The problem here is that ActiveSupport monkey patches Object which Dry::Types::Enum implicitly inherits from.

From: /home/benny/Dev/ruby/dry-rb/bugs/.gem/ruby/2.3.0/gems/dry-types-0.7.1/lib/dry/types/array/member.rb @ line 15 Dry::Types::Array::Member#call:

    12: def call(input, meth = :call)
    13:   input.map { |el|
    14:     result = member.__send__(meth, el)
 => 15:     binding.pry
    16:     result
    17:   }
    18: end

[1] pry(#<Dry::Types::Array::Member>)> meth
=> :try
[2] pry(#<Dry::Types::Array::Member>)> member
=> #<Dry::Types::Enum:0x0055df70d72fa8
 @mapping={0=>"val1", 1=>"val2"},
 @options={:values=>["val1", "val2"]},
 @type=
  #<Dry::Types::Constrained:0x0055df70d73070
   @options=
    {:rule=>
      #<Dry::Logic::Rule::Conjunction left=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:type? args=[String]> options={}> right=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:inclusion? args=[["val1", "val2"]]> options={}>>},
   @rule=
    #<Dry::Logic::Rule::Conjunction left=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:type? args=[String]> options={}> right=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:inclusion? args=[["val1", "val2"]]> options={}>>,
   @type=#<Dry::Types::Definition primitive=String options={}>>,
 @values=["val1", "val2"]>
[3] pry(#<Dry::Types::Array::Member>)> member.method(:try)
=> #<Method: Dry::Types::Enum(Object)#try>
[4] pry(#<Dry::Types::Array::Member>)> member.method(:try).source_location
=> ["/home/benny/Dev/ruby/dry-rb/bugs/.gem/ruby/2.3.0/gems/activesupport-4.2.6/lib/active_support/core_ext/object/try.rb", 62]
[5] pry(#<Dry::Types::Array::Member>)> member
=> #<Dry::Types::Enum:0x0055df70d72fa8
 @mapping={0=>"val1", 1=>"val2"},
 @options={:values=>["val1", "val2"]},
 @type=
  #<Dry::Types::Constrained:0x0055df70d73070
   @options=
    {:rule=>
      #<Dry::Logic::Rule::Conjunction left=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:type? args=[String]> options={}> right=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:inclusion? args=[["val1", "val2"]]> options={}>>},
   @rule=
    #<Dry::Logic::Rule::Conjunction left=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:type? args=[String]> options={}> right=#<Dry::Logic::Rule::Value predicate=#<Dry::Logic::Predicate id=:inclusion? args=[["val1", "val2"]]> options={}>>,
   @type=#<Dry::Types::Definition primitive=String options={}>>,
 @values=["val1", "val2"]>
[6] pry(#<Dry::Types::Array::Member>)> member.class.ancestors
=> [Dry::Types::Enum, Dry::Types::Decorator, Object, #<Module:0x0055df707e4cd0>, PP::ObjectMixin, Kernel, BasicObject]

And therefore the method_missing is never hit.

I'm not sure what dry-types should do in that cases where another thirdparty lib monkey patches basically everything.
I know we have to change because rails and therefore activesupport is kinda a monopoly, which makes me really sad..

You mentioned it's working in other scenarios. Can you give 1 example where exactly so I can lookup the differences. I'm not sure how many methods Dry::Types::Enum needs from Object, maybe 1 possible way is to explicitly inherit from BasicObject, which leads to less inspectability but it would solve that problem.

cc @timriley, @AMHOL

@andreaseger
Copy link
Author

I only tried it with some other types and there it worked. So something like.

CustomStringArray = Strict::Array.member(Coercible::String)

Form my point of view this is caused by how the array type and enum works together. I just wasn't sure if a similar approach to the explicit usage/exploit of method_missing in enum is used somewhere else.

@tak1n
Copy link

tak1n commented Apr 19, 2016

For you're given example:

CustomStringArray = Strict::Array.member(Coercible::String)

I get following behaviour:

From: /home/benny/Dev/ruby/dry-rb/bugs/.gem/ruby/2.3.0/gems/dry-types-0.7.1/lib/dry/types/array/member.rb @ line 15 Dry::Types::Array::Member#call:

    12: def call(input, meth = :call)
    13:   input.map { |el|
    14:     result = member.__send__(meth, el)
 => 15:     binding.pry
    16:     result
    17:   }
    18: end

[1] pry(#<Dry::Types::Array::Member>)> meth
=> :try
[2] pry(#<Dry::Types::Array::Member>)> member
=> #<Dry::Types::Constructor type=#<Dry::Types::Definition primitive=String options={}>>
[3] pry(#<Dry::Types::Array::Member>)> member.method(:try)
=> #<Method: Dry::Types::Constructor#try>
[4] pry(#<Dry::Types::Array::Member>)> member.class
=> Dry::Types::Constructor
[5] pry(#<Dry::Types::Array::Member>)> member.class.ancestors
=> [Dry::Types::Constructor,
 #<Dry::Equalizer:0x00560ad0b03bb0>,
 Dry::Types::Definition,
 Dry::Types::Builder,
 Dry::Types::Options,
 Dry::Equalizer::Methods,
 #<Dry::Equalizer:0x00560ad0b35020>,
 Object,
 #<Module:0x00560ad0b2c448>,
 PP::ObjectMixin,
 Kernel,
 BasicObject]
[6] pry(#<Dry::Types::Array::Member>)> member.method(:try).source_location
=> ["/home/benny/Dev/ruby/dry-rb/bugs/.gem/ruby/2.3.0/gems/dry-types-0.7.1/lib/dry/types/constructor.rb", 32]

The reason why this works is because in

def try(input, &block)
a explicit try method is defined and therefore it does not rely on method_missing.

So I see 2 possible solutions for now:

  1. Do not rely on method_missing and somehow implement Dry::Types::Enum#try
  2. Rely on method_missing with a clean slate (inherit from BasicObject)
    https://github.com/rails/rails/blob/52ce6ece8c8f74064bb64e0a0b1ddd83092718e1/activesupport/lib/active_support/core_ext/object/try.rb#L87

@andreaseger
Copy link
Author

andreaseger commented Apr 19, 2016

IMHO relying on method_missing in a gem without a clean slate is dangerous. There are quite a few gems which add them self to object (i.e. json and bson just to name two from the top of my head)

I haven't dug into the code to deep but I've tried to have Dry::Types::Enum inherit from BasicObject. The specs stayed green and it fixes the described issue. (see master...andreaseger:fix_try_method_missing_conflict )
I can make a PR from there if this will be the preferred solution.

Would it be possible to completely remove the reliance on method_missing? Meaning not only for this special case but in general.

@timriley
Copy link
Member

It looks like you're heading in a good direction, @andreaseger. To me it seems like it's the #method_missing in Dry::Types::Decorator that's probably the real issue here, because the #method_missing in Dry::Types::Constructor has a local #try method right alongside it, so it shouldn't have the issue in an ActiveSupport-tainted environment that you saw.

Decorator is mixed into 6 classes, and if your experiments with having one of them inherit from BasicObject is successful, I reckon it'd be worth rolling that out to the other 5 and seeing how it looks then. At least at this point we'd have a complete PR for further discussion :) What do you think?

@solnic
Copy link
Member

solnic commented Apr 24, 2016

Let's just implement try in Decorator and delegate explicitly.

@solnic solnic closed this as completed in 861c0b2 Jun 15, 2016
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

No branches or pull requests

4 participants