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

Use last field of a compound PK when setting auto ids #689

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

Conversation

gheckenbach
Copy link

In complex models with multi-field primary keys, the auto_increment field is last. Without this, @primary_key is set into values directly (key is a table), like values[{"user_id", "id"}] = 123. While id in this example is unique regardless of user_id, it's reasonable to include user_id in the primary key to better locate the user's data.

@leafo
Copy link
Owner

leafo commented Apr 30, 2020

Think you can add a test for this or do you want me to? I'm not familiar enough with mysql for this case so I would have to verify with a test

@gheckenbach
Copy link
Author

I will add a test.

@gheckenbach
Copy link
Author

I've spent the last 4 hours trying to get all the prerequisites installed such that I can run "busted spec_mysql" and will have to continue tomorrow. A quick test might be to change the spec_mysql/models.moon class Posts so that user_id is before id, and add "PRIMARY KEY (user_id, id)", and then add a test that verifies that id still gets incremented in the model returned from create. I haven't verified that, yet, but will continue working to get my environment set up to do so.

@gheckenbach gheckenbach force-pushed the gheckenbach/insert_id-compound-pk branch from 4c6950f to 982bfca Compare May 5, 2020 18:47
Comment on lines -24 to -25
{"title", types.text null: false}
{"body", types.text null: false}
Copy link
Author

Choose a reason for hiding this comment

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

Mysql tests were broken without this. It probably slipped since the mysql tests weren't included in ci.sh.

@@ -44,7 +76,7 @@ class Likes extends Model
create_table @table_name!, {
{"user_id", types.integer}
{"post_id", types.integer}
{"count", types.integer}
{"count", types.integer default: 0}
Copy link
Author

Choose a reason for hiding this comment

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

Also broke mysql tests. Fixed.

@gheckenbach
Copy link
Author

Had a couple false starts, but this one looks good. I considered modifying Posts as I previously suggested, but too much other test code would have to change to support it. I found it cleaner to just add a new model, for testing the new functionality.

@gheckenbach
Copy link
Author

Hey @leafo, have you had a chance to look at the diff? Just checking in, thanks.

leafo added a commit that referenced this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants