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

Respond_to? returns private methods #4253

Closed
gaurish opened this issue Oct 27, 2016 · 17 comments
Closed

Respond_to? returns private methods #4253

gaurish opened this issue Oct 27, 2016 · 17 comments
Labels
Milestone

Comments

@gaurish
Copy link

@gaurish gaurish commented Oct 27, 2016

From Twitter: https://twitter.com/headius/status/788857979655323648

Environment

jruby 9.0.5.0 (2.2.3) 2016-01-26 7bee00d Java HotSpot(TM) 64-Bit Server VM 25.101-b13 on 1.8.0_101-b13 +jit [darwin-x86_64]
The test case:

test "assign private attribute" do
    model = Model.new
    assert_raises(ActiveModel::UnknownAttributeError) do
      model.assign_attributes(metadata: { a: 1 })
    end
  end

Expected Behavior

raise ActiveModel::UnknownAttributeError

Actual Behavior

Class: <NoMethodError>
Message: <"protected method `metadata=' called for #<AttributeAssignmentTest::Model:0x4203529f>">
---Backtrace---
org/jruby/RubyKernel.java:1820:in `public_send'
/Users/gaurish/code/repo/rails/activemodel/lib/active_model/attribute_assignment.rb:47:in `_assign_attribute'
/Users/gaurish/code/repo/rails/activemodel/lib/active_model/attribute_assignment.rb:40:in `block in _assign_attributes'
org/jruby/RubyHash.java:1339:in `each'
/Users/gaurish/code/repo/rails/activemodel/lib/active_model/attribute_assignment.rb:39:in `_assign_attributes'
/Users/gaurish/code/repo/rails/activemodel/lib/active_model/attribute_assignment.rb:33:in `assign_attributes'
test/cases/attribute_assignment_test.rb:83:in `block in test_assign_private_attribute'

This there is a comment about this at https://github.com/jruby/jruby/blame/21b9760280f07fe91aa5a12f23334579b5744024/core/src/main/java/org/jruby/ext/ripper/RubyRipper.java#L361

@kares
Copy link
Member

@kares kares commented Nov 2, 2016

thanks, would be better to give it a go on JRuby 9.1.5.0 instead of 9.0.5.0 (that or a reduced test-case)

@kares kares added core Rails labels Nov 2, 2016
@kirs
Copy link
Contributor

@kirs kirs commented Nov 15, 2016

9.1.5.0 has the same issue:

class Post
  protected
  def metadata
  end
end
puts Post.new.respond_to?(:metadata).inspect


$ ruby -v
ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-darwin15]
$ ruby respond.rb
false


$ ruby -v
jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 Java HotSpot(TM) 64-Bit Server VM 25.45-b02 on 1.8.0_45-b14 +jit [darwin-x86_64]
$ ruby respond.rb
true

cc @headius

@kirs
Copy link
Contributor

@kirs kirs commented Nov 15, 2016

cc @enebo

@enebo
Copy link
Member

@enebo enebo commented Nov 15, 2016

@kirs this is fixed in 9.1.6.0. Can you confirm that?

@kirs
Copy link
Contributor

@kirs kirs commented Nov 15, 2016

I confirm that ❤️
I'll update Rails to use 9.1.6.0.

I think the issue can be closed.

@kirs
Copy link
Contributor

@kirs kirs commented Nov 15, 2016

Ooops, I was wrong:

$ jruby -v
jruby 9.1.6.0 (2.3.1) 2016-11-09 0150a76 Java HotSpot(TM) 64-Bit Server VM 25.45-b02 on 1.8.0_45-b14 +jit [darwin-x86_64]
# post.rb
class Post
  protected
  def metadata
  end
end
puts Post.new.respond_to?(:metadata).inspect
$ jruby post.rb
true
@enebo enebo added this to the JRuby 9.1.7.0 milestone Nov 16, 2016
@enebo
Copy link
Member

@enebo enebo commented Nov 16, 2016

@kirs interestingly the bug we fixed in 9.1.6.0 was the exact opposite problem that we were not seeing true when we should have. Thanks for checking.

@kirs
Copy link
Contributor

@kirs kirs commented Nov 16, 2016

Maybe you fixed it for private methods, but not for protected?

On Wed, Nov 16, 2016 at 9:30 AM, Thomas E Enebo notifications@github.com
wrote:

@kirs https://github.com/kirs interestingly the bug we fixed in 9.1.6.0
was the exact opposite problem that we were not seeing true when we should
have. Thanks for checking.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4253 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAf3q_se33BTum75qDiOLIq4pC36cB2Gks5q-xOcgaJpZM4KiH7E
.

@enebo
Copy link
Member

@enebo enebo commented Nov 16, 2016

@kirs the fix was for protected methods and involved us not checking beneath a particular point due to the use of prepended modules. We were getting false back and not true due to us not searching past the prepended module. I read this issue backwards.

@kirs
Copy link
Contributor

@kirs kirs commented Nov 16, 2016

Thanks for the background! In my case above there're no prepended modules involved.

@kirs
Copy link
Contributor

@kirs kirs commented Nov 23, 2016

@enebo this is still an issue on master. Do you have any suggestion how I can fix it? It currently blocks us on the Rails side.
Thanks!

@enebo
Copy link
Member

@enebo enebo commented Nov 23, 2016

@kirs I will see if I can get any insight into this. One weird thing is respond_to? even in a method of the same type returns true while mri will return false. So post will really respond to being sent metadata but the respond_to? method will still say false?

@kirs
Copy link
Contributor

@kirs kirs commented Nov 23, 2016

@enebo not exactly. As a result, respond_to? => true on JRuby leads to NoMethodError being triggered:

class Post
  protected
  def metadata
  end
end
post = Post.new
if post.respond_to?(:metadata)
  post.metadata
else
  puts "no metadata available"
end
$ bin/jruby respond.rb
NoMethodError: protected method `metadata' called for #<Post:0x4923ab24>
  <main> at respond.rb:8

While on MRI that works as expected:

$ chruby ruby-2.3.3
$ ruby respond.rb
no metadata available
@enebo
Copy link
Member

@enebo enebo commented Nov 23, 2016

@kirs I think I have this fixed locally. Running tests now. If appears for dispatch we only care about private visibility but for respond_to? we care about both private and protected. It is a lack of symmetry.

@kirs
Copy link
Contributor

@kirs kirs commented Nov 23, 2016

Awesome. I'm looking forward for updates from you.

@enebo enebo closed this in d081169 Nov 23, 2016
@enebo
Copy link
Member

@enebo enebo commented Nov 23, 2016

@kirs kick the tires on this one. I did actually find some tagged out specs on this and I feel fairly confident about the fix but nonetheless this is pretty low-level language mojo :)

@kirs
Copy link
Contributor

@kirs kirs commented Nov 23, 2016

Thanks! ❤️
Now it works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.