-
-
Notifications
You must be signed in to change notification settings - Fork 247
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
base: master
Are you sure you want to change the base?
Use last field of a compound PK when setting auto ids #689
Conversation
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 |
I will add a test. |
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 |
4c6950f
to
982bfca
Compare
{"title", types.text null: false} | ||
{"body", types.text null: false} |
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.
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} |
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.
Also broke mysql tests. Fixed.
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. |
Hey @leafo, have you had a chance to look at the diff? Just checking in, thanks. |
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
. Whileid
in this example is unique regardless ofuser_id
, it's reasonable to includeuser_id
in the primary key to better locate the user's data.