Skip to content

Add association_primary to has_and_belongs_to_many#307

Open
AndiLavera wants to merge 3 commits into
imdrasil:masterfrom
AndiLavera:association-primary
Open

Add association_primary to has_and_belongs_to_many#307
AndiLavera wants to merge 3 commits into
imdrasil:masterfrom
AndiLavera:association-primary

Conversation

@AndiLavera

Copy link
Copy Markdown

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
Comment thread src/jennifer/relation/many_to_many.cr Outdated

@imdrasil imdrasil left a comment

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

imdrasil commented Apr 5, 2020

Copy link
Copy Markdown
Owner

@andrewc910 It would be great

@AndiLavera

AndiLavera commented Apr 6, 2020

Copy link
Copy Markdown
Author

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
Copy Markdown
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
{
foreign_field => obj.attribute(primary_field),
association_foreign_key => rel.primary,
association_foreign_key =>

@imdrasil imdrasil Apr 15, 2020

Copy link
Copy Markdown
Owner

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

_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
Copy Markdown
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
Copy Markdown
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