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

Add association_primary to has_and_belongs_to_many #307

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AndiLavera
Copy link

First, let me say if this already exists and I am using jennifer wrong, please just let me know!

What does this PR do?

Adds association_primary to has_and_belongs_to_many. Currently, the query is built by defaulting to the primary key of the other table with T.primary. This allows users to define a different column, like uuid, while keeping id as the primary key for the table.

Any background context you want to provide?

Right now i have an App & a User model. They are connected by has_and_belongs_to_many with the join table being apps_users.

App psuedo sql table:

id, integer, not null, nextval
name, string
uuid, string, unique, not null # this is what apps_users is referencing
"apps_pkey" PRIMARY KEY, btree (id)
TABLE "apps_users" CONSTRAINT "fk_cr_606d320c60" FOREIGN KEY (app_id) REFE
RENCES apps(uuid) ON UPDATE RESTRICT ON DELETE RESTRICT

apps_users sql table:

id, integer, not null, nextval
user_id, integer
app_id, string
"apps_users_pkey" PRIMARY KEY, btree (id)
FOREIGN KEY (app_id) REFERENCES apps(uuid) ON UPDATE R
ESTRICT ON DELETE RESTRICT

Right now when i do:

user = User.find 1
user.apps if user

It throws a bad query error. The sql looks like this:

SELECT apps.* FROM apps JOIN apps_users ON apps_users.app_id = apps.id AND apps_users.user_id = $1

Notice the apps_users.app_id = apps.id. A proper query would look like:

apps_users.app_id = apps.uuid

In the user class i can do association_foreign: :uuid which is obviously wrong but changes the sql query to:

apps_users.uuid = apps.id

Almost right. That's what led me down this rabbit hole. With this PR, i can change the above to association_primary: :uuid

apps_users.id = apps.uuid

Release notes

Please fill the corresponding section regarding this pull request notes below instead of modifying CHANGELOG.md file. Please remove all redundant sections before posting request.

Model
Adds association_primary to has_and_belongs_to_many relation to allow specifying a separate column for querying other than the primary key column

Other

I am not too happy with how association_primary_key came out in ManyToMany and would love to see a more elegant solution.

@imdrasil imdrasil self-requested a review March 31, 2020 08:42
Copy link
Owner

@imdrasil imdrasil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job 👍 I'd only adjust one tiny thing. This feature definitely will be useful

@AndiLavera
Copy link
Author

Would you like me to make a spec? I haven't dug into your tests so idk what's their but i am sure this spec would be similar to whatever spec tests 'association_foreign'.

@imdrasil
Copy link
Owner

imdrasil commented Apr 5, 2020

@andrewc910 It would be great

@AndiLavera
Copy link
Author

AndiLavera commented Apr 6, 2020

Okay, i think i did it.

All i did was add association_primary in the Contact class for the specs and then change a spec to ensure the sql query is utilizing the new key instead of defaulting for the primary key. Please let me know if this was not correct.

I also change the association_primary method as we discussed earlier. Your recommendations caused a type error becuase T.c() can't accept Nil. What i did builds on your ternary operator recommendation.

Finally i added a comment which i plan to remove. Just wanted to mark it and see your thoughts. I couldn't quite understand the method join_condition in the ManyToMany file. I believe the T.primary should be changed to association_primary_key but i wanted to run it by you.

@AndiLavera
Copy link
Author

Problem: When using association_primary you cannot use add_{{name.id}}(rel : Hash). This behavior happens because ManyToMany(T, Q)#add_join_table_record hardcodes the value for association_foreign_key with objects primary key.

Fix: This newest commit changes RelationDefinition#add_{{name.id}}(rel : Hash) for has_and_belongs_to_many when using association_primary. It probably also fixes add_{{name.id}}(rel : {{klass}}) but i didn't test.

New Problem: rel.to_h.transform_keys { |key| key.to_s }[association_primary]
rel is the new object like user_profile while association_primary is a string. I couldn't figure out how to get the value with a string so i converted the object to hash. I then convert the symbol keys into string and then fetch the value. There has to be a better way to access the value of an object with a string. I am hoping you have a better way somewhere in Jennifer.

**Note:**I don't believe there are explicit tests for these methods. This was something i discovered when refactoring some controllers.

@imdrasil imdrasil self-requested a review April 15, 2020 08:07
@@ -104,7 +111,8 @@ module Jennifer
join_table!,
{
foreign_field => obj.attribute(primary_field),
association_foreign_key => rel.primary,
association_foreign_key =>
Copy link
Owner

@imdrasil imdrasil Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

association_primary ? rel.attribute(association_primary.not_nil!) : rel.primary

@@ -58,6 +60,7 @@ module Jennifer
_primary = primary_field
jt = join_table!
q = query.join(jt, type: type) { Q.c(_primary) == c(_foreign) }.join(T, type: type) do
# TODO: Replace `T.primary` with `association_primary_key`?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is correct

@imdrasil
Copy link
Owner

Yeah, my initial suggestion does work because association_primary is a method and it's type is always resolved as String | Symbol | Nil. The right place to adjust tests is spec/model/relation_definition_spec.cr (%has_and_belongs_many section).

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

Successfully merging this pull request may close these issues.

2 participants