-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(cli): add 'generated: true' entry to id property when generating #3643
Conversation
2ddc128
to
c037e08
Compare
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.
LGTM.
Is there a (reasonably easy) way how to write a test for this new execution path?
The test for this functionality only makes sense for MySQL since in-memory and mongo generate ids automatically without |
|
||
if (answers.id) { | ||
// 'generated' makes sure id is defined, especially for database like MySQL | ||
answers['generated'] = true; |
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.
Should we have a prompt to ask if the id is automatically generated by the database
?
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.
+1 to add a prompt. The topic is more complex though because of the model-level setting forceId
.
Let's land this PR as an incremental improvement and open a new issue to investigate what changes are needed to support models where the PK is provided by clients (not generated by the DB).
I agree with you that we don't want to run I was thinking about a test that will call |
6c33325
to
193b941
Compare
193b941
to
2104f97
Compare
connect to #3538
We found in refactor test PR that id needs to be generated for any instances for the mysql tests, otherwise it complains with Error:
ER_NO_DEFAULT_FOR_FIELD: Field 'id' doesn't have a default value
.It's not required for relations. But it might be problematic if models are migrated to databases such
MySQL
.Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈