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

allows composite keys on migration only #236

Merged
merged 1 commit into from
Oct 17, 2021

Conversation

ataft
Copy link
Contributor

@ataft ataft commented Oct 5, 2021

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 of this.idNames(model).

Fixes #138

@ataft ataft requested a review from dhmlau as a code owner October 5, 2021 18:08
@dhmlau
Copy link
Member

dhmlau commented Oct 5, 2021

@ataft, thanks for your contribution. Could you please add some tests to verify your changes? Thanks.

@ataft
Copy link
Contributor Author

ataft commented Oct 6, 2021

Test has been added

@dhmlau
Copy link
Member

dhmlau commented Oct 7, 2021

@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.
And I'd also like to get at least one more review.

In the meanwhile, I'll see if I can get the GitHub Actions working or the docker image running.

@dhmlau
Copy link
Member

dhmlau commented Oct 7, 2021

One more thing.. LoopBack4 doesn't support composite key loopbackio/loopback-next#1830.

@marioestradarosa marioestradarosa changed the title Allow composite key allows composite keys on migration only Oct 14, 2021
@marioestradarosa
Copy link

@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 idInjection. The fact that a work around is to use a composite key, not all use cases are using a composite one, so please update your PR description.

Also, it is important to note that loopback4 does not support composite keys when performing CRUD operations, so this PR only addresses the migration part.

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]>
@ataft
Copy link
Contributor Author

ataft commented Oct 14, 2021

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.

Copy link
Member

@dhmlau dhmlau left a 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.

@dhmlau
Copy link
Member

dhmlau commented Oct 17, 2021

FYI - I've updated the setup.sh script to set up the docker image for sql server: #237.

Copy link

@marioestradarosa marioestradarosa left a 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!

@dhmlau
Copy link
Member

dhmlau commented Oct 17, 2021

Verified locally. Will set up GitHub Actions as next step. Force merging this PR.

@dhmlau dhmlau merged commit 80d555c into loopbackio:master Oct 17, 2021
@dhmlau
Copy link
Member

dhmlau commented Oct 17, 2021

@ataft, thanks for your contribution. Your PR has landed! 🎉

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.

Composite keys not added as primary keys
3 participants