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

Avoid stack level too deep in predicate builder #41394

Merged
merged 1 commit into from Feb 11, 2021

Conversation

@afrase
Copy link
Contributor

@afrase afrase commented Feb 10, 2021

Summary

If you have an association with the same name as the foreign key then a SystemStackError: stack level too deep error is raised.

This PR addresses the issue by checking if the query variable is different from attributes argument before recursively calling expand_from_hash. We have been running this fix in production for a number of years without issue at the company I work for.

This issue has been mentioned in a number of issues #26778, #34869, #31110.

Other Information

Something to note is the setter for the association can raise errors. I have another branch that fixes that but I'm not sure if using an association extension is the best way to handle it.

Thanks

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 10, 2021

I'm not sure if this is something we should be supporting. If the foreign key has the same name of the association the behavior is very confusing. Which behavior the reader and the setter should have? Should it always return the object of the id? Should it set the object or the id? What should be the behavior of the association on fixtures?

What advantages do you get in your application by defining the foreign key withe the same name of the association?

@afrase
Copy link
Contributor Author

@afrase afrase commented Feb 10, 2021

I'm not sure if this is something we should be supporting. If the foreign key has the same name of the association the behavior is very confusing. Which behavior the reader and the setter should have? Should it always return the object of the id? Should it set the object or the id? What should be the behavior of the association on fixtures?

What advantages do you get in your application by defining the foreign key withe the same name of the association?

Sorry, I need to make a correction, (it's been a while since I've dug into this issue) the error happens when the foreign key has the same name as the model it's defined in for has_many and has_one. For example

class User
  has_many :notes, foreign_key: :user # => results in SystemStackError
  has_one :supervisor, foreign_key: :user # => results in SystemStackError
end

class Supervisor
  belongs_to :user, foreign_key: :user # => works as expected
end

class Note
  belongs_to :user, foreign_key: :user # => works as expected
end

You are correct, in that Rails probably shouldn't necessarily support this but I think it should still work. Right now it's impossible to do without monkey patching expand_from_hash because of the recursive problem. Associations have the option to define the foreign key so I believe they should support having that key be named (almost) anything.

There are no advantages to doing it this way but we have a legacy database from a different application that didn't follow the rails convention of having the _id suffix for relationships.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 10, 2021

It makes sense to not break with a stack level too deep. Can you squash your three commits in one?

@afrase afrase force-pushed the recursive-association-fix branch from b686464 to f186a2b Feb 11, 2021
@afrase
Copy link
Contributor Author

@afrase afrase commented Feb 11, 2021

@rafaelfranca Should now be all squashed to one commit!

Check if `query` is different from `attributes` before recursively calling `expand_from_hash`.
Updated cases to account for new company and comment records.
@afrase afrase force-pushed the recursive-association-fix branch from f186a2b to 91d5834 Feb 11, 2021
@rafaelfranca rafaelfranca merged commit 84e115a into rails:main Feb 11, 2021
2 checks passed
rafaelfranca added a commit that referenced this issue Feb 11, 2021
Avoid stack level too deep in predicate builder
@afrase afrase deleted the recursive-association-fix branch Feb 12, 2021
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

2 participants