Ensure HABTM relationships produce valid class names (Fixes #17119)

[codeodor]

See #17119 for the back story.

[codeodor]

I guess I need to figure out a fix that runs on Ruby 1.9.3 and 2.0.

[codeodor]

Adjustment made to pass on 2.1, 2.0, and 1.9.3 in my environment. Now let's see how Travis does. 😄

[seuros]

Is ObjectSpace available in jruby ?

[codeodor]

It seems to be available, but turned off by default.

Since this is only in a test, do you think it is unreasonable to require JRuby users to turn ObjectSpace on for the purposes of running the tests? (i.e., it will not affect anyone using Rails, only those running the tests)

Unfortunately, const_get and const_defined? behave differently between 1.9.3, 2.0, and 2.1, so we cannot use those (if they are even available by default in JRuby -- which I'm not sure of, since they might rely on ObjectSpace for all I know).

[seuros]

we can't have failing jruby test in travis. We either have to turn on ObjectSpace or skip this test in jruby.

cc @senny @jeremy @tenderlove @rafaelfranca

[codeodor]

FWIW, ObjectSpace is used elsewhere in Rails as of the time of this comment. Not a lot, but it is there. Does Rails already require JRuby to have it turned on? Or is the project trying to move away from it?

Alternatively, do you know a way that would work in both? All else being equal, I'd rather have 100% JRuby support without requiring switching from defaults if I can.

[codeodor]

Ok, thanks. I'll test it locally on JRuby and see what I can come up with if it fails.

[seuros]

Look good to me, the failure in jruby are unrelated to your change i think.

[codeodor]

Yeah, I cannot get the tests to run in JRuby. For me, it says --dev is not a valid option. On travis https://travis-ci.org/rails/rails/jobs/37502973#L223 it is not even loading to run the tests.

[tenderlove]

I'm sure we can think of a way to test this without ObjectSpace. :-/ walking all classes in ObjectSpace seems a bit extreme.

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Oct 9, 2014, at 9:31 AM, Abdelkader Boudih notifications@github.com wrote:

In activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb:

@@ -883,4 +887,15 @@ def test_redefine_habtm
child.special_projects << SpecialProject.new("name" => "Special Project")
assert child.save, 'child object should be saved'
end
+

• def test_reflections_do_not_have_nonexistent_constants_as_class_names
• refute_nil Developer._reflections['shared_computers']

• Checking the fixture for named association is important here, because it's the only way

• we've been able to reproduce one of the 2 parts of this bug

• refute_nil File.read(File.expand_path("../../../fixtures/developers.yml", FILE)).index("shared_computers")
• Developer._reflections.values.each do |association|

•  constant = ObjectSpace.each_object(Class).detect{|c| c.to_s == association.class_name }
  

  we can't have failing jruby test in travis. We either have to turn on ObjectSpace or skip this test in jruby.

cc @senny @jeremy @tenderlove @rafaelfranca

�
Reply to this email directly or view it on GitHub.

[codeodor]

@tenderlove Because HABTM_* classes are built under the ActiveRecord model's namespace, there are multiple levels we need to check for constants.

In Ruby 1.9.3, const_get does not look recursively if we pass in Developer::HABTM_Projects for instance, while 2.0 mentions "this method will recursively look up constant names if a namespaced class name is provided.

Although I couldn't find it documented, I noticed the same behavior when using const_defined? (which was the reason the original pull request failed on 2.0 and 1.9.3).

We could create our own recursive lookup, but that seems worse to me.

I suppose we could just try accessing the constant under assert_nothing_raised ... I'll try that.

[thedarkone]

ObjectSpace is available in JRuby if used for looking up classes/modules, ie: ObjectSpace.each_object(Module) or ObjectSpace.each_object(Class), but is disabled otherwise:

irb(main):001:0> ObjectSpace.each_object(Module) {|m| }
=> 657
irb(main):002:0> ObjectSpace.each_object(Class) {|m| }
=> 539
irb(main):003:0> ObjectSpace.each_object(Object) {|m| }
RuntimeError: ObjectSpace is disabled; each_object will only work with Class, pass -X+O to enable
    from org/jruby/RubyObjectSpace.java:173:in `each_object'
    from (irb):4:in `evaluate'
    from org/jruby/RubyKernel.java:1121:in `eval'
    from org/jruby/RubyKernel.java:1521:in `loop'
    from org/jruby/RubyKernel.java:1284:in `catch'
    from org/jruby/RubyKernel.java:1284:in `catch'

[codeodor]

I just rebased against master since this has sit for so long (2 more betas have come out!)

This should be a show-stopper bug: It breaks documented functionality in at least the case where every test in your Rails app will error out if you use a relationship with a non-standard name, and who knows what else it affects.

@tenderlove you had an objection before to using assert_nothing_raised and asked if we could just use the constant. I don't think that's a good idea.

I don't like assert_nothing_raised but as I see it, we either use that or go back to checking if the constant exists using ObjectSpace.

I'm open to doing that if you prefer that.

Let me know. 😄

[robin850]

There's no need to call assert_nothing_raised ; actually it will only yield the content of the given block so we try to avoid to use it. You can simply write association.class_name.constantize inside your #each block.

[codeodor]

Thanks for the feedback. I'll remove it, but I preferred it because it makes the intent more clear. I would hate to see the test removed in the future because it doesn't appear to be testing anything. Perhaps the comments are enough.

[robin850]

I think that this is safe ; the test's name is pretty clear and there's always git blame. :-)

[robin850]

@codeodor: Could you please add a changelog entry ? Thanks a lot for your patch so far ! :-)

[codeodor]

@robin850 I'll make the suggested changes later today. However since this is simply a bug fix to a beta release, is the changelog entry desired? Normally I would add it, but I thought for something like this it's not wanted. 😄

[robin850]

Normally I would add it, but I thought for something like this it's not wanted.

Yep you're right, sorry, nevermind !

[codeodor]

Ok, refute nil changed to assert_not_nil and removed assert_nothing_raised, and rebased.

[tenderlove]

Is it possible to demonstrate this bug without specifically constantizing the class name?

[tenderlove]

@tenderlove you had an objection before to using assert_nothing_raised and asked if we could just use the constant. I don't think that's a good idea.

Why? Just do assert_equal "ExpectedClassName", class_name.constantize.name

I still have the earlier question though, is it possible to demo the bug without specifically constantizing that value. This test is testing internals.

[codeodor]

@tenderlove

Why? Just do assert_equal "ExpectedClassName", class_name.constantize.name

Then it would certainly be testing internals. For example, we know one of the classes we want to test is called ModelClass::HABTM_somethings so we could test that as one (there are others we'd want to test as well, because for example when using the fixtures it ignores the class_name option when trying to discover the class based on the relationship name in _reflections).

In any case, suppose the naming convention later changes, or another kind of abstraction is produced as an intermediate layer:

If we iterate over the classes produced by AR, then we cover those cases without tying the test to the internals (other than iterating over the reflections). If we instead hard code the class names in the test, we lose that ability.

Is it possible to demonstrate this bug without specifically constantizing the class name?

I've only thought of 2 ways to do ensure a string is a valid constant:

1. constantizing its name to raise an error if it is not valid, or
2. look through the classes in ObjectSpace to see if it exists.

The original version of this test did look at ObjectSpace but was changed to constantize the names. Is there an alternative I haven't considered?

I am totally open to having a better test. I'm not happy with this one. But I was unable to figure out a better way to produce the errors in a test. If you have any more ideas, I would love to improve it. I just feel like hard coding the class names in the test is worse than iterating through _reflections.

My biggest concern is a release of 4.2 without this fix, because I and anyone else who uses a non-standard name as a relationship will be unable to run their tests. For example, with the following line and
a fixture that references the employees relationship:

  has_many :employees, class_name: "User"

every test in the app that loads the fixture will error out.

That is the most immediate problem I encountered in the wild.

[codeodor]

I suppose we could drop the class name part of the test altogether and just ensure the fixture has the expected data that will cause an error without the fix applied, since that's the only place in the wild where I've seen the error.

I only discovered the class name issue as a by-product of that, since it was ignoring the class_name option.

So if you'd like me to just remove that part of the test in an effort to make sure this fix gets into 4.2, I'd be happy to do so.

[tenderlove]

I suppose we could drop the class name part of the test altogether and just ensure the fixture has the expected data that will cause an error without the fix applied, since that's the only place in the wild where I've seen the error.

Yes, this is exactly what I want. The generated class names, and even constantizing them are internals and I want to be free to change that. In fact, my original implementation created no constants at all for the middle models. Eventually I would like to return to that, but testing directly against the internal constant doesn't really prevent the regression you were seeing. Testing the constants directly forces us to be bound to this implementation.

[codeodor]

Ok, done. Thanks @tenderlove for helping me see the light you were sharing! 😄

[tenderlove]

Sorry for all the back and forth. Thanks for the hard work on this!!!

[chancancode]

@tenderlove @codeodor for some reason, this commit broke the build after being merged into master. Would one of you be able to take a look? If not, I'll try to figure it out later this week

[tenderlove]

Weird. Travis gave it a pass. I can take a look later.

[codeodor]

Weird that it didn't catch it before, since the commit that seems to cause the issue was introduced in 2009 so it would have run against it. 😄 (See https://github.com/rails/rails/blame/master/activerecord/test/cases/validations/uniqueness_validation_test.rb#L44)

It seems to only fail when running that test file in isolation, so maybe that's why I didn't catch it.

In any case, if we remove the developers fixture from that test (it is not used), the problem goes away. I will issue a new pull request to fix it.

[codeodor]

See #17578 for the fix.

[codeodor]

@tenderlove It was my pleasure. Thanks for having patience with me. 😁

[chancancode]

Ah, now I know what's going on. @matthewd disabled running the isolated tests on PRs on travis, that's why it didn't caught it :)