-
Notifications
You must be signed in to change notification settings - Fork 366
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
Fix issue with lb4 id different to int #1783
Conversation
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
In which file should I create the tests? |
Here are few existing tests related to id injection, I think it's a good place where to add a new test for your change: loopbackio/loopback-next#3602 describes a problem with auto-generated id values when don't have the type configured by the connector. To be sure that the proposed change is actually solving the high-level problem, I'd be good to have a test that's calling Such test can go to |
Ready, @bajtos can you take a look? |
Without the 'useDefaultIdType' it works exactly the same ... |
I pushed a commit 8a58239 that fixes the test to fail on current master and pass with your changes in place. Let's see how it works when executed against different SQL/NoSQL connectors. |
@slnode test please |
@bajtos The tests fail and I can't look at the log |
Ouch. Here is the log for MySQL - it looks like the new flag is not honored and the primary column is created with numeric type (instead of a string/varchar).
Similar problem on PostgreSQL:
|
This is tricky. The most important thing to change is the model name. The original test is using an existing table/collection created for I am not sure if all SQL connectors support auto-generated PKs with non-numeric type, I am trying to disable See 67019c4 for more details. Let's wait for CI results now 🤞 |
@slnode ok to test |
Looks like my latest commit made the test to pass for SQL too. @frbuceta are you fine with the current patch? If yes, then we can clean up the git history (preferably preserving my co-authorship via https://github.blog/2018-01-29-commit-together-with-co-authors/) and proceed to land this pull request. |
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.
We need to clean up git history before landing.
Yes, I am in favor, in any case it is an honor to work with you on this |
Co-authored-by: Miroslav Bajtoš <mbajtoss@gmail.com>
Landed, thank you for the contribution 🎉 |
Introduce a new property-level metadata
useDefaultIdType
which can be used to prevent juggler from replacing PK property type provided by the user with the default PK type supplied by the connector.See loopbackio/loopback-next#3602
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machine