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

Fixes to primaryKey generation. #14351

Merged
merged 8 commits into from
Mar 17, 2021
Merged

Conversation

mshima
Copy link
Member

@mshima mshima commented Mar 16, 2021

  • Reverts recent primaryKey logic change.
  • Reapply fixes to templates.
  • Extract primaryKey into a separated function
  • Create preparingFields priority for a more customizable workflow.
  • Reimplement primaryKey.ids.
  • Reimplement tests using to.deep.include instead of to.containSubset, output in case of error is not useful for the later.

Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@mshima
Copy link
Member Author

mshima commented Mar 16, 2021

@yelhouti take a look at the tests if it is the outcome you need.

@@ -320,6 +199,193 @@ function prepareEntityForTemplates(entityWithConfig, generator) {
return entityWithConfig;
}

function prepareEntityPrimaryKeyForTemplates(entityWithConfig, generator, enableCompositeId = true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mshima that is a very good idea for blueprints who don't want to support compositeIds.

@yelhouti
Copy link
Contributor

yelhouti commented Mar 17, 2021

@mshima first of all, thanks for taking the time and all you are doing for the project.

Although I don't understand why we need something this complicated (I might be missing something), I think that moving the logic of: prepareEntityPrimaryKeyForTemplates is a great step forward.

I realize that we won't be implementing the compositeKey of the templates in the main repo so I will stop trying to push things my way and override what I don't like in my blueprints.

I will try to make a PR that changes thing I don't like, and maybe it will break something I will understand better, in the mean time, please, just go ahead an merge this.

Just for the record, the main things I don't like, is that we have new fields own/derived that have a different signature from field (and that are not "real fields") but have the same name. Other than that great work with all I mentioned in my comments (+ the path variable...)

@mshima
Copy link
Member Author

mshima commented Mar 17, 2021

Although I don't understand why we need something this complicated (I might be missing something)

This is a developer preference.
I implemented as a OO approach, the current primaryKey should return its own definition correctly and the means to its child to calculate its primaryKey. While you used 3 or 4 function to do it eagerly, going the way to the root.

Apart from the implementation approach, it should be done lazily, because the info is not there yet.

I think that moving the logic of: prepareEntityPrimaryKeyForTemplates is a great step forward.

I realize that we won't be implementing the compositeKey of the templates in the main repo so I will stop trying to push things my way and override what I don't like in my blueprints.

We can implement composite key in the the templates, but I'm against implementing it as <%= if (primaryKey.composite) { %>calculateMyCompositeKeyValue<% } else { %><%= primaryKey.field[0].fieldName %><% } %> all over the place.
We should cleanup the templates instead of creating more complexity.

What I think you have missed the point is that if the compositeId was merged as I've proposed (a mapped field), it would be much less code to override at your blueprint.
Just remove that mapped field in the generator, and add the id calculation to the code.

I will try to make a PR that changes thing I don't like, and maybe it will break something I will understand better, in the mean time, please, just go ahead an merge this.

Great, thanks.
Going forward I suggest:

  • angular: change links to use getIndentifier method, so in the end, you just need to override that.
  • java: better support for manually generated ids.

Just for the record, the main things I don't like, is that we have new fields own/derived that have a different signature from field (and that are not "real fields") but have the same name.

They only have some additional fields related to the id, so IMO it's not a problem.
And in my opinion originalFields is fields originated in the entity, so it's a ownField.

The fields name can be renamed, but I really don't see problem to keep this way.

Other than that great work with all I mentioned in my comments (+ the path variable...)

Thanks.

@mshima mshima merged commit 4930b07 into jhipster:main Mar 17, 2021
@mshima mshima mentioned this pull request Mar 17, 2021
@mshima mshima deleted the skip_ci_primary_key branch March 17, 2021 12:42
@pascalgrimaud pascalgrimaud added this to the v7.0.0 milestone Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants