-
Notifications
You must be signed in to change notification settings - Fork 84
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
allows composite keys on migration only #236
Conversation
@ataft, thanks for your contribution. Could you please add some tests to verify your changes? Thanks. |
Test has been added |
@ataft, thanks for adding the test. Your changes look reasonable to me. Since CI is not working and I have problems in getting my mssql docker image running for local tests, I'd prefer not to approve the PR until then. In the meanwhile, I'll see if I can get the GitHub Actions working or the docker image running. |
One more thing.. LoopBack4 doesn't support composite key loopbackio/loopback-next#1830. |
@ataft please squash your commits. Also the issue #1 is not closed by this PR, which is trying to prevent the DB server to auto generate an id by using the flag Also, it is important to note that |
Fix migration to use idNames() instead of idName(), allowing for a composite primary key from multiple columns Signed-off-by: ataft <[email protected]> Composite Key Test Added test for composite key Signed-off-by: ataft <[email protected]>
I squashed the commits and removed the text about closing issue #1. Regarding Loopback 4 not supporting composite keys, that's fine since we'll be on Loopback 3 until v4 comes closer to reaching parity with v3. I was keeping track of the features in v3 that were not in v4, but it got a little out of control, as you can see in the bot-closed issue: loopbackio/loopback-next#1920. |
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.
The changes look reasonable to me. I also was able to run the tests locally. Would like to get at least another approval (possibly from @marioestradarosa) before landing this PR. Thanks.
FYI - I've updated the setup.sh script to set up the docker image for sql server: #237. |
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.
Thanks @ataft for your contribution!
Verified locally. Will set up GitHub Actions as next step. Force merging this PR. |
@ataft, thanks for your contribution. Your PR has landed! 🎉 |
Fix migration to use idNames() instead of idName(), allowing for a composite primary key from multiple columns.
If you look at migration.js line 392, you can see it only adds one column to the primary key. This is because migration.js line 358 calls
this.idName(model)
instead ofthis.idNames(model)
.Fixes #138