-
Notifications
You must be signed in to change notification settings - Fork 64
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
Model association updates #525
Model association updates #525
Conversation
src/avram/associations/has_many.cr
Outdated
def preload_{{ assoc_name }}(preload_query : {{ model }}::BaseQuery) | ||
preload_{{ through.first.id }} do |through_query| | ||
through_query.preload_{{ through[1].id }}(preload_query) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We force the preload of the through association and in the query we tell that association to preload the next one.
This is reversing the way we were doing it before. This is required because we don't know the method names in the old direction, we were making a guess using the foreign key which doesn't handle both the has many through a belongs to and a has many through a has many scenarios.
I think this ends up making more sql calls and could potentially be N+1 but I'm not sure.
{{ through.first.id }}_query | ||
.{{ foreign_key.id }}(id) | ||
.map(&.{{ through[1].id }}_count) | ||
.sum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely making more sql calls than before. Like I said above, we can't start at the opposite end of the association, we have to start from base and travel through the "throughs"/intermediary tables because those are the only names we know.
This is why I had to add the def {{...}}_count
method to belongs to associations.
src/avram/model.cr
Outdated
macro setup_association_queries(associations, *args, **named_args) | ||
{% for assoc in associations %} | ||
def {{ assoc[:assoc_name] }}_query | ||
{{ assoc[:type] }}::BaseQuery.new | ||
end | ||
{% end %} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary because we don't have access to the types of the intermediary associations in a has many through
511cb9b
to
10a7a06
Compare
928d85c
to
9665b36
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, overall it looks pretty good, and from my testing it seems to work. Also it looks like this might actually fix #196.
There does seem to be one issue related to running specs. Running the specs the first time runs fine, but running specs a second time right after seems to throw an exception Unexpected error while running migrations:
. The thing though is, why are migrations thinking they need to be ran when specs are running the second time? 🤔 I tested against latest release and master branch, and those seem to be fine. It's just this branch that does it.
Is the destination name of the through association required or optional now? |
9665b36
to
4a8aaa9
Compare
4bd6392
to
3bc664d
Compare
@wontruefree I'm not sure I understand the question. Could you explain what you mean? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I gave this another shot after the update, and it's working. I can run specs back to back again, and my tests all passed. I think we should just merge this and get it in the hands of some people to really play around with it. So feel free to merge when you feel ready 👍
This is awesome. Thanks @matthewmcgarvey |
Fixes #523
Fixes #473
Fixes: #496
Related (but does not fix): #196
Had to bring in the changes in #494 and #499 in order to get it working.
Changes
through
field.Has Many Through
Old
New
The array is the association route to get to the customers. So Manager will need to have an association with the name of
employees
and Employee is expected to have an association calledcustomers
that returns Customer.Belongs To
Old
New
This also means that belongs to associations can now be named something other than the class and still work