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

Find inverse associations with plural names #33729

Merged
merged 1 commit into from Aug 29, 2018

Conversation

@kddeisz
Copy link
Contributor

kddeisz commented Aug 27, 2018

Previously ActiveRecord couldn't find inverse associations if they were plural, which is a pretty standard use case. This commit changes the behavior to first attempt to find the singular version, then attempt to find the plural version. That makes it work and find plural associations as in the example below:

class Post
  has_many :comments
end

class Comment
  belongs_to :post
end

Previously the :post association reflection would only attempt to find a comment inverse, as opposed to both a comment and comments inverse. This drastically cuts down on the number of associations that don't have inverses, and can end up fixing a lot of strange behavior where the parent doesn't save the child records.

@rails-bot
Copy link

rails-bot commented Aug 27, 2018

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@kddeisz kddeisz force-pushed the kddeisz:plural-automatic-inverse branch Aug 27, 2018
activerecord/lib/active_record/reflection.rb Outdated
begin
reflection = klass._reflect_on_association(candidate)
klass <= reflection.active_record && reflection.can_find_inverse_of_automatically?
rescue NameError

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 27, 2018 Member

We should scope this recue to only the method we are expecting to raise it.

@kddeisz kddeisz force-pushed the kddeisz:plural-automatic-inverse branch 3 times, most recently Aug 27, 2018
Previously ActiveRecord couldn't find inverse associations if they were plural, which is a pretty standard use case. This commit changes the behavior to first attempt to find the singular version, then attempt to find the plural version. That makes it work and find plural associations as in the example below:

```
class Post
  has_many :comments
end

class Comment
  belongs_to :post
end
```

Previously the `:post` association reflection would only attempt to find a `comment` inverse, as opposed to both a `comment` and `comments` inverse.
@kddeisz kddeisz force-pushed the kddeisz:plural-automatic-inverse branch to 1c7275a Aug 27, 2018
@kddeisz
Copy link
Contributor Author

kddeisz commented Aug 29, 2018

@rafaelfranca any other feedback on this one?

@rafaelfranca rafaelfranca merged commit ed1eda2 into rails:master Aug 29, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kddeisz kddeisz deleted the kddeisz:plural-automatic-inverse branch Aug 29, 2018
@yahonda
Copy link
Contributor

yahonda commented Aug 29, 2018

Let me inform this test fails with Ruby 2.5.1 and ruby-head. Working fine with 2.4.4.

CI shows the same failure. https://travis-ci.org/rails/rails/jobs/422266836

$ ruby -v
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]
$ for i in sqlite3 postgresql mysql2 ; do ARCONN=$i bundle exec ruby -w -Itest test/cases/associations/inverse_associations_test.rb -n test_finds_inverse_of_for_plural_associations;  done
Using sqlite3
Run options: -n test_finds_inverse_of_for_plural_associations --seed 49610

# Running:

E

Error:
InversePolymorphicBelongsToTests#test_finds_inverse_of_for_plural_associations:
NoMethodError: undefined method `name' for nil:NilClass
    test/cases/associations/inverse_associations_test.rb:735:in `test_finds_inverse_of_for_plural_associations'


rails test test/cases/associations/inverse_associations_test.rb:734



Finished in 0.026777s, 37.3462 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
Using postgresql
Run options: -n test_finds_inverse_of_for_plural_associations --seed 42043

# Running:

E

Error:
InversePolymorphicBelongsToTests#test_finds_inverse_of_for_plural_associations:
NoMethodError: undefined method `name' for nil:NilClass
    test/cases/associations/inverse_associations_test.rb:735:in `test_finds_inverse_of_for_plural_associations'


rails test test/cases/associations/inverse_associations_test.rb:734



Finished in 0.088667s, 11.2782 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
Using mysql2
Run options: -n test_finds_inverse_of_for_plural_associations --seed 15896

# Running:

E

Error:
InversePolymorphicBelongsToTests#test_finds_inverse_of_for_plural_associations:
NoMethodError: undefined method `name' for nil:NilClass
    test/cases/associations/inverse_associations_test.rb:735:in `test_finds_inverse_of_for_plural_associations'


rails test test/cases/associations/inverse_associations_test.rb:734



Finished in 0.073057s, 13.6879 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
$
@yahonda
Copy link
Contributor

yahonda commented Aug 29, 2018

Addressed by fa132ef

kamipo added a commit to kamipo/rails that referenced this pull request Jan 31, 2019
rails#33729 affected the behavior of the has_many through record creation.

Since rails#33729, the intermediate reflection of simple has_many through
association has `inverse_of` to the association, it causes extra through
record creation, the extra through record required valid before the
association record is saved.

https://github.com/rails/rails/blob/23125378673bcc606b274027666a126573e136f8/activerecord/lib/active_record/associations/has_many_through_association.rb#L95-L102

I think that rails#33729 need to more work to care about has_many through
association, that PR should be reverted to not break existing apps.
kamipo added a commit to kamipo/rails that referenced this pull request Jan 31, 2019
…inverse"

This reverts commit ed1eda2, reversing
changes made to 3d2caab.

Reason: 7c3da6e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.