-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Entity fields default values #25633
Entity fields default values #25633
Conversation
5c0cb04
to
a14ec0a
Compare
...ors/liquibase/templates/src/main/resources/config/liquibase/changelog/updated_entity.xml.ejs
Outdated
Show resolved
Hide resolved
5c2a5ad
to
d27816f
Compare
...e/templates/src/main/resources/config/liquibase/changelog/updated_entity_constraints.xml.ejs
Outdated
Show resolved
Hide resolved
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.
Use lodash instead of custom implementation.
8b5f901
to
1b25acf
Compare
Please note I'd squash the commits to one once the whole Pull Request is alright |
...emplates/src/main/webapp/app/entities/_entityFolder_/update/_entityFile_-form.service.ts.ejs
Outdated
Show resolved
Hide resolved
If there's anything "required" left, I'll continue next week. |
65c56aa
to
4b9ead2
Compare
If you could update your link @mshima in that one open review comment to point to something specific, I may be able to finish my Pull Request :) |
af23a1a
to
af16184
Compare
a14a406
to
fc1d90d
Compare
Anything else? :) |
fc1d90d
to
b467379
Compare
c73e79f
to
7695814
Compare
I've updated the implementation with all the suggestions, @mshima |
if (field.columnRequired && field.liquibaseDefaultValueAttributeValue) { | ||
this.handleCheckFailure( | ||
`The field ${field.fieldName} in entity ${entity.name} has both columnRequired and a defaultValue, this can lead to unexpected behaviors.`, | ||
); | ||
} |
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.
I think the check should be changed to field.columnRequired && !field.liquibaseDefaultValueAttributeValue
.
Only for new fields probably in
generator-jhipster/generators/liquibase/generator.ts
Lines 191 to 193 in 7695814
for (const field of entity.fields ?? []) { | |
prepareField(entity, field, this); | |
prepareFieldForLiquibase(entity, field); |
This can be done later if makes sense.
Any docs about how to use this new feature? |
Not available in 8.3.0, will be available in next release. |
Docs are yet to be written for the next release. I'll do that by end of the week. |
Fixes #25094
Also I used the opportunity to unify the conditions used in the incremental generator for writing the actual liquibase xml files and producing the references in master.xml and making sure they are also the same conditions as in the actual templates. This was inconsistent before.
I only saw in the Angular generator an easy way to introduce default values there. In the others, there are no means at all for default values: React one would require an update of the corresponding dependency - and vue.js, I really don't have a clue about that.
If the merge request is OK in general, I'll also enhance the documentation.
Please make sure the below checklist is followed for Pull Requests.
When you are still working on the PR, consider converting it to Draft (below reviewers) and adding
skip-ci
label, you can still see CI build result at your branch.