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

core_ext/class.rb:subclasses conflicts with ActiveRecord #4741

Closed
atinm opened this Issue Aug 16, 2017 · 11 comments

Comments

Projects
None yet
4 participants
@atinm

atinm commented Aug 16, 2017

Environment

  • jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439

Expected Behavior

After require 'jruby/core_ext' ActiveRecord::Base.subclasses should continue to return an array of the ActiveRecord::Base subclasses but instead returns the Java subclasses of ActiveRecord::Base that do not have the same methods available on them anymore.

  • Example in irb:
    $ irb
    jruby-9.1.12.0 :001 > require 'active_record'
     => true 
    jruby-9.1.12.0 :002 > class Foo < ActiveRecord::Base
    jruby-9.1.12.0 :003?>   def bar
    jruby-9.1.12.0 :004?>     end
    jruby-9.1.12.0 :005?>   end
     => :bar 
    jruby-9.1.12.0 :006 > ActiveRecord::Base.subclasses
     => [Foo (call 'Foo.connection' to establish a connection)] 
    jruby-9.1.12.0 :007 > ActiveRecord::Base.subclasses.first.table_name
     => "foos" 
    jruby-9.1.12.0 :008 > require 'jruby/core_ext'
     => true 
    jruby-9.1.12.0 :009 > ActiveRecord::Base.subclasses
     => [#<ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x4efc25fc>] 
    jruby-9.1.12.0 :010 > ActiveRecord::Base.subclasses.first.table_name
    NoMethodError: undefined method `table_name' for #
    <ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x396e6d9>
            from (irb):10:in `<eval>'
            from org/jruby/RubyKernel.java:1000:in 'eval'
            from org/jruby/RubyKernel.java:1298:in 'loop'
            from org/jruby/RubyKernel.java:1120:in 'catch'
            from org/jruby/RubyKernel.java:1120:in 'catch'
            from /home/u/.rvm/rubies/jruby-9.1.12.0/bin/irb:13:in '<main>'

Analysis

  • JRuby is defining subclasses at:
    https://github.com/jruby/jruby/blob/f7746d0be94f5d9f7cd27af610f1bc5dd2622794/lib/ruby/stdlib/jruby/core_ext/class.rb#L31

  • ActiveRecord is also defining subclasses at:
    https://github.com/rails/rails/blob/cfb1e4dfd8813d3d5c75a15a750b3c53eebdea65/activesupport/lib/active_support/core_ext/class/subclasses.rb#L50

JRuby really shouldn't be messing up ActiveRecord's subclasses call if it breaks ActiveRecord usage.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 23, 2017

Member

it was really meant to be ~ internal, aj guess 😉 ... for jrubyc only. anyway JRuby can work around that.

@enebo we probably only want this for 9.2 or do you have a string preference for getting this into 9.1.13?

Member

kares commented Aug 23, 2017

it was really meant to be ~ internal, aj guess 😉 ... for jrubyc only. anyway JRuby can work around that.

@enebo we probably only want this for 9.2 or do you have a string preference for getting this into 9.1.13?

kares added a commit to kares/jruby that referenced this issue Aug 23, 2017

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 23, 2017

Member

OK so its part of JRuby's core extension API ... it can not be simply removed in another release.
... we can start deprecating subclasses (in favor of each_object(klass)) as this seems to be an issue.
so you will need to either not require 'jruby/core_ext' or simply undef the Class#subclasses method def.

Member

kares commented Aug 23, 2017

OK so its part of JRuby's core extension API ... it can not be simply removed in another release.
... we can start deprecating subclasses (in favor of each_object(klass)) as this seems to be an issue.
so you will need to either not require 'jruby/core_ext' or simply undef the Class#subclasses method def.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 23, 2017

Member

@kares yeah I think part of the solution will be to deprecate on our side and replace it with something which does not conflict. Since we have been exposing this as a public method for many years (since at least 2011?) now this is unfortunate. I am always amazed how long it takes for someone to notice these issues but perhaps AS + jrubyc is uncommon?

Member

enebo commented Aug 23, 2017

@kares yeah I think part of the solution will be to deprecate on our side and replace it with something which does not conflict. Since we have been exposing this as a public method for many years (since at least 2011?) now this is unfortunate. I am always amazed how long it takes for someone to notice these issues but perhaps AS + jrubyc is uncommon?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 23, 2017

Member

@kares oh we should get each_object usage into 9.1.13.0 internally and not use subclasses. At that point people can include jruby/core_ext before AS is loaded and have AS win while not making anything on our side stop working. So if you plan on changing our internal consumer(s) then please cp that commit to jruby-9.1.13.0 branch.

Member

enebo commented Aug 23, 2017

@kares oh we should get each_object usage into 9.1.13.0 internally and not use subclasses. At that point people can include jruby/core_ext before AS is loaded and have AS win while not making anything on our side stop working. So if you plan on changing our internal consumer(s) then please cp that commit to jruby-9.1.13.0 branch.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 23, 2017

Member

@enebo I was wrong, JRuby is actually not using subclasses internally. it was meant to be user consumed.
the problem I guess is that JRuby's version returns smt completely unexpected for the AR::Base case ((when jruby/core_ext is loaded after AS boots), guess its missing logic to exclude some noise (otherwise it seems to be doing fine) :

jruby-1.7.26 :021 > ActiveRecord::Base.subclasses.first
 => #<ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x390e814c>

... which are just generated (included) modules - should be fine to get them excluded although not 100% sure

Member

kares commented Aug 23, 2017

@enebo I was wrong, JRuby is actually not using subclasses internally. it was meant to be user consumed.
the problem I guess is that JRuby's version returns smt completely unexpected for the AR::Base case ((when jruby/core_ext is loaded after AS boots), guess its missing logic to exclude some noise (otherwise it seems to be doing fine) :

jruby-1.7.26 :021 > ActiveRecord::Base.subclasses.first
 => #<ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x390e814c>

... which are just generated (included) modules - should be fine to get them excluded although not 100% sure

@atinm

This comment has been minimized.

Show comment
Hide comment
@atinm

atinm Aug 23, 2017

In fact, it includes only the generated modules - they are 1:1 with all the subclasses of ActiveRecord::Base in my use case and the generated modules don't have 'subclasses' defined anymore after jruby/core_ext is loaded after AS is up. We're working around this by not using 'subclasses' but rather implementing our own as active record is coming in very early and jruby/core_ext is only in one of our modules. For example, in the test case I put in, there is only one subclass returned - the generated module and it's missing table_name.

atinm commented Aug 23, 2017

In fact, it includes only the generated modules - they are 1:1 with all the subclasses of ActiveRecord::Base in my use case and the generated modules don't have 'subclasses' defined anymore after jruby/core_ext is loaded after AS is up. We're working around this by not using 'subclasses' but rather implementing our own as active record is coming in very early and jruby/core_ext is only in one of our modules. For example, in the test case I put in, there is only one subclass returned - the generated module and it's missing table_name.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 25, 2017

Member

the underlying issue is that we return the raw subclasses collection that a RubyClass manages internally.
that includes include/prepend wrappers (which are not really useful for the user) are included - gets messy.

ObjectSpace does filter these out so my proposal would be to simply re-implement JRuby's subclasses using OS for 9.1.13 ... if someone needs the old way there's still a way using klass.to_java.subclases.
... expecting this would mean full compatibility with AR::Base returned sub-classes

for 9.2 JRuby might deprecate Class#subclasses altogether (provide a non core-ext polluting alternative).

Member

kares commented Aug 25, 2017

the underlying issue is that we return the raw subclasses collection that a RubyClass manages internally.
that includes include/prepend wrappers (which are not really useful for the user) are included - gets messy.

ObjectSpace does filter these out so my proposal would be to simply re-implement JRuby's subclasses using OS for 9.1.13 ... if someone needs the old way there's still a way using klass.to_java.subclases.
... expecting this would mean full compatibility with AR::Base returned sub-classes

for 9.2 JRuby might deprecate Class#subclasses altogether (provide a non core-ext polluting alternative).

@kares kares self-assigned this Aug 25, 2017

@kares kares closed this in 9a3fe9c Aug 27, 2017

@kares kares added this to the JRuby 9.2.0.0 milestone Aug 27, 2017

kares added a commit to kares/jruby that referenced this issue Aug 27, 2017

Class#subclasses should work correctly - not return internal wrappers
implementation has been changed but old behavior can be achieved
... using `klass.to_java.sublclasses` (not very useful for user-code)

also synchronize subclasses retrieval on RubyClass ~ rest of related impl

resolves #4741

kares added a commit to kares/jruby that referenced this issue Aug 28, 2017

Class#subclasses should work correctly - not return internal wrappers
implementation has been changed but old behavior can be achieved
... using `klass.to_java.sublclasses` (not very useful for user-code)

also synchronize subclasses retrieval on RubyClass ~ rest of related impl

resolves #4741
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 28, 2017

Member

@enebo willl be back-porting the patch to 9.1.13 (will change the target after its done) to make JRuby's subclasses behave as expected - its re-implemented using ObjectSpace (which filters out the internal module wrappers) so it should work fine with AS/Rails. on 9.2 the method should likely get deprecated.

Member

kares commented Aug 28, 2017

@enebo willl be back-porting the patch to 9.1.13 (will change the target after its done) to make JRuby's subclasses behave as expected - its re-implemented using ObjectSpace (which filters out the internal module wrappers) so it should work fine with AS/Rails. on 9.2 the method should likely get deprecated.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 28, 2017

Member

landed as 916b1e1

Member

kares commented Aug 28, 2017

landed as 916b1e1

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 5, 2017

Member

I need to point out that this new subclasses impl will be much slower than the built-in version due to the cost of running the each_object block. That may or may not matter for typical uses.

Under the covers, each_object for a class just does what the old subclasses did, but the Ruby code could be more efficient.

Has this been backported to 9.1 branch yet?

Member

headius commented Sep 5, 2017

I need to point out that this new subclasses impl will be much slower than the built-in version due to the cost of running the each_object block. That may or may not matter for typical uses.

Under the covers, each_object for a class just does what the old subclasses did, but the Ruby code could be more efficient.

Has this been backported to 9.1 branch yet?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Sep 5, 2017

Member

yep, backported.
we need to to the ObjectSpace walking since subclasses needs to skip wrappers -> which might be hiding the real subclass of a class (as it stands in between).

Member

kares commented Sep 5, 2017

yep, backported.
we need to to the ObjectSpace walking since subclasses needs to skip wrappers -> which might be hiding the real subclass of a class (as it stands in between).

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