Skip to content

Model association updates#525

Merged
matthewmcgarvey merged 2 commits intoluckyframework:masterfrom
matthewmcgarvey:through-array-has-many
Nov 22, 2020
Merged

Model association updates#525
matthewmcgarvey merged 2 commits intoluckyframework:masterfrom
matthewmcgarvey:through-array-has-many

Conversation

@matthewmcgarvey
Copy link
Member

@matthewmcgarvey matthewmcgarvey commented Nov 12, 2020 •
edited
Loading

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

  • Updates "has many through"'s to take in an array in the through field.
  • Belongs to associations now have singular "where_xxx" queries

Has Many Through

Old

class Manager < BaseModel
  table do
    has_many employees : Employee
    has_many customers : Customer, through: :employees
  end
end

New

class Manager < BaseModel
  table do
    has_many employees : Employee
    has_many customers : Customer, through: [:employees, :customers]
  end
end

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 called customers that returns Customer.

Belongs To

Old

class Employee < BaseModel
  table do
    belongs_to manager : Manager
  end
end

# notice the plural where, we were using the table name
Employee::BaseQuery.new.where_managers(...)

New

class Employee < BaseModel
  table do
    belongs_to manager : Manager
  end
end

# notice the singular where, we are using the association name now
Employee::BaseQuery.new.where_manager(...)

This also means that belongs to associations can now be named something other than the class and still work

class Employee < BaseModel
  table do
    belongs_to boss : Manager
  end
end

Employee::BaseQuery.new.where_boss(...)

@matthewmcgarvey matthewmcgarvey added the BREAKING CHANGE This will cause a breaking change label Nov 12, 2020
matthewmcgarvey
matthewmcgarvey commented Nov 12, 2020
View reviewed changes
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
Copy link
Member Author

@matthewmcgarvey matthewmcgarvey Nov 12, 2020

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.

src/avram/associations/has_many.cr
{{ through.first.id }}_query
.{{ foreign_key.id }}(id)
.map(&.{{ through[1].id }}_count)
.sum
Copy link
Member Author

@matthewmcgarvey matthewmcgarvey Nov 12, 2020

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
Comment on lines +227 to +234
macro setup_association_queries(associations, *args, **named_args)
{% for assoc in associations %}
def {{ assoc[:assoc_name] }}_query
{{ assoc[:type] }}::BaseQuery.new
end
{% end %}
end
Copy link
Member Author

@matthewmcgarvey matthewmcgarvey Nov 12, 2020

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

This was referenced Nov 12, 2020
Merged
Merged
@matthewmcgarvey matthewmcgarvey marked this pull request as ready for review November 12, 2020 16:51
@matthewmcgarvey matthewmcgarvey changed the title [WIP] Rework has many through association logic Rework has many through association logic Nov 12, 2020
This was referenced Nov 14, 2020
Closed
Closed
@matthewmcgarvey matthewmcgarvey force-pushed the through-array-has-many branch 4 times, most recently from 928d85c to 9665b36 Compare November 20, 2020 04:55
@matthewmcgarvey matthewmcgarvey changed the title Rework has many through association logic Model association updates Nov 20, 2020
jwoertink
jwoertink reviewed Nov 20, 2020
View reviewed changes
Copy link
Member

@jwoertink jwoertink left a 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.

jwoertink
jwoertink reviewed Nov 20, 2020
View reviewed changes
@jkthorne
Copy link
Contributor

jkthorne commented Nov 20, 2020

Is the destination name of the through association required or optional now?

@matthewmcgarvey
Copy link
Member Author

matthewmcgarvey commented Nov 20, 2020

@wontruefree I'm not sure I understand the question. Could you explain what you mean?

jwoertink
jwoertink approved these changes Nov 21, 2020
View reviewed changes
Copy link
Member

@jwoertink jwoertink left a 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 👍

@matthewmcgarvey matthewmcgarvey merged commit 9d6c167 into luckyframework:master Nov 22, 2020
@matthewmcgarvey matthewmcgarvey deleted the through-array-has-many branch November 22, 2020 01:49
@paulcsmith
Copy link
Member

paulcsmith commented Nov 30, 2020

This is awesome. Thanks @matthewmcgarvey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jwoertink jwoertink jwoertink approved these changes

Assignees

No one assigned

Labels

BREAKING CHANGE This will cause a breaking change

Projects

None yet

Milestone

No milestone

4 participants

@matthewmcgarvey @jkthorne @paulcsmith @jwoertink