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

Fix a problem when re-defining has_many :through associations #29547

Closed
wants to merge 1 commit into from
Closed

Fix a problem when re-defining has_many :through associations #29547

wants to merge 1 commit into from

Conversation

tuzz
Copy link
Contributor

@tuzz tuzz commented Jun 23, 2017

This is an attempt to fix this issue: #29123

When calling a has_many :through association we
check if the source association is defined after
the has_many :through. If it is, we throw a
HasManyThroughOrderError.
(See: f8ab3ae)

However, this error was being erroneously thrown
in cases when the has_many :through overrides
another association of the same name.

This is an attempt to fix this issue:
#29123

When calling a ‘has_many :through’ association we
check if the source association is defined after
the has_many :through. If it is, we throw a
HasManyThroughOrderError.
(See f8ab3ae)

However, this error was being erroneously thrown
in cases when the has_many :through overrides
another association of the same name.
@eileencodes
Copy link
Member

Thanks for the PR @tuzz

What is the use case for re-defining an already defined association? I think that would cause unexpected behavior and be confusing to app developers. That seems more like an app bug than a Rails bug.

@tuzz
Copy link
Contributor Author

tuzz commented Jun 28, 2017

@eileencodes: I don't think there is a use-case for re-defining an association, but given method overriding in Ruby is fairly common, I think it's reasonable to assume people will do it. In this case, a seemingly unrelated error is being thrown when this happens, which misleads people and sends them down a rabbit-hole of digging inside Rails to understand the problem.

Perhaps it would be better to explicitly guard against re-defining associations. I'm not sure.

What do you think?

@rafaelfranca
Copy link
Member

We don't want people to redefine the same association. It make no sense. Even ruby give warnings about redefining the same method. Let's disallow association override, instead of allowing it again.

@tuzz
Copy link
Contributor Author

tuzz commented Jun 29, 2017

Ok, will look into that instead.

@tuzz tuzz closed this Jun 29, 2017
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

Successfully merging this pull request may close these issues.

None yet

3 participants