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

feat: composite id backend support #19849

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yelhouti
Copy link
Contributor


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.

@yelhouti
Copy link
Contributor Author

yelhouti commented Oct 5, 2022

fixes #17919

@github-actions github-actions bot removed theme: angular theme: front theme: dependencies Pull requests that update a dependency file labels Jul 6, 2023
@yelhouti
Copy link
Contributor Author

yelhouti commented Jul 6, 2023

@DanielFran I fixed the conflicts (if you could please have a look) I still have some small tests to fix ... I m willing to put more time if you are willing to merge

@yelhouti
Copy link
Contributor Author

yelhouti commented Jul 7, 2023

@mshima thanks a lot man for the work you did on vscode to generate integration tests locally. It was always a PITA to know which test should be run how. Now it all works great and I can fix tests 10 times faster. Maybe this should be documented for others in CONTRIBUTING.md .

@yelhouti yelhouti force-pushed the composite-key branch 5 times, most recently from 75a17c2 to 1cec7fc Compare July 7, 2023 23:44
@yelhouti
Copy link
Contributor Author

yelhouti commented Jul 8, 2023

@mshima @DanielFran finally tests are passing here and in my compositeKey projects. Could anyone please merge this

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

Entity suffix is not correct.
I need to do a deeper review.

@yelhouti
Copy link
Contributor Author

yelhouti commented Jul 8, 2023

@mshima sure, if you can point me to what to fix, I will gladly do

@mshima
Copy link
Member

mshima commented Jul 8, 2023

At least it looks wrong at generated sample.
IMG_0276

@yelhouti
Copy link
Contributor Author

yelhouti commented Jul 8, 2023

At least it looks wrong at generated sample. IMG_0276

When using MapsId, the id of the parent and the child are the same. Put I can change this, if you want, hopefully it won't break the composite with multi level... I must admit I don't remember why i changed it

@mshima anything else you have issues with ?

@yelhouti
Copy link
Contributor Author

Guess, I need to rebase again ...

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

We should avoid complicating templates.

This is a partial review, would be much better if split into multiple PRs.

},
get nameDotted() {
function preparePrimaryKeyFields(field) {
Object.defineProperty(field, 'fieldNameDotted', {
Copy link
Member

Choose a reason for hiding this comment

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

A more meaningful name

Suggested change
Object.defineProperty(field, 'fieldNameDotted', {
Object.defineProperty(field, 'tsFieldPath', {

enumerable: true,
configurable: true,
});
Object.defineProperty(field, 'fieldNameDottedAsserted', {
Copy link
Member

Choose a reason for hiding this comment

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

A more meaningful name

Suggested change
Object.defineProperty(field, 'fieldNameDottedAsserted', {
Object.defineProperty(field, 'tsFieldPathRequired', {

@@ -477,6 +477,12 @@ export const baseServerFiles = {
'config/WebConfigurer.java',
],
},
{
condition: generator => !generator.reactive,
Copy link
Member

Choose a reason for hiding this comment

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

Only write if any composite key is found

@@ -21,6 +21,12 @@ int databaseSizeBeforeUpdate = <%= entityInstance %>Repository.findAll()<%= call
// Update the <%= entityInstance %> using partial update
<%= persistClass %> partialUpdated<%= persistClass %> = new <%= persistClass %>();
partialUpdated<%= persistClass %>.set<%= primaryKey.nameCapitalized %>(<%= persistInstance %>.get<%= primaryKey.nameCapitalized %>());
<%_ primaryKey.composite && fields.filter(f => f.id).forEach(f => { _%>
Copy link
Member

Choose a reason for hiding this comment

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

for of is much easier to understand instead of forEach specially for ejs

@@ -47,7 +53,7 @@ webTestClient
.expectStatus()
.isOk();
<%_ } else { _%>
rest<%= entityClass %>MockMvc.perform(patch(ENTITY_API_URL_ID, partialUpdated<%= persistClass %>.get<%= primaryKey.nameCapitalized %>())<% if (authenticationUsesCsrf) { %>.with(csrf())<% }%>
rest<%= entityClass %>MockMvc.perform(patch(ENTITY_API_URL_ID, <%- persistInstanceUrlId.replaceAll(persistInstance, 'partialUpdated' + persistClass) %>)<% if (authenticationUsesCsrf) { %>.with(csrf())<% }%>
Copy link
Member

Choose a reason for hiding this comment

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

replaceAll in templates is really confusing.
We should simplify.
I will do a deeper look later.

@@ -141,7 +141,7 @@ _%>
<%_ } _%>
<%_ } _%>

<%_ const idNames = primaryKey ? [...primaryKey.fields.map(f => f.fieldName)] : [] _%>
<%_ const idNames = primaryKey ? (primaryKey.composite ? Array.from(new Set(primaryKey.fields.map(f => f.path[0]))) : [primaryKey.name]) : [] _%>
Copy link
Member

Choose a reason for hiding this comment

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

Using a set to deduplicate here is confusing.

@@ -47,6 +47,9 @@ import <%= entityAbsolutePackage %>.repository.<%= mapsIdAssoc.otherEntityNameCa
<%_ if (searchEngineElasticsearch) { _%>
import <%= entityAbsolutePackage %>.repository.search.<%= entityClass %>SearchRepository;
<%_ } _%>
<%_ if (primaryKey.composite) { _%>
import <%=packageName%>.domain.<%=primaryKey.type%>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import <%=packageName%>.domain.<%=primaryKey.type%>;
import <%= entityAbsolutePackage %>.domain.<%= primaryKey.type %>;

<%# I wouldn't expect to be needed the way @EmbeddedId works with "insertable = false, updatable = false" but it seems like it is so putting it here, this might not work for bigger depth, but for now I think its enough %>
<%_ for (const relationship of relationships.filter(r => r.id && !primaryKey.derived && r.otherEntity.primaryKey.fields.length > 1)) { _%>
<%_ for (const field of relationship.otherEntity.primaryKey.fields) { _%>
@Mapping(target = "<%= relationship.relationshipName %>.id.<%= field.fieldName %>", source = "<%= relationship.relationshipName %>.<%= field.fieldNameDotted %>")
Copy link
Member

Choose a reason for hiding this comment

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

primaryKey.name?

@@ -24,6 +24,11 @@ _%>
<%_ if (!dtoMapstruct || serviceNo) { _%>
import <%= entityAbsolutePackage %>.domain.<%= persistClass %>;
<%_ } _%>
<%_ if (primaryKey.fields.length > 1) { _%>
import <%=packageName%>.domain.<%=primaryKey.type%>;
Copy link
Member

Choose a reason for hiding this comment

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

Replace every packageName with entityAbsolutePackage and add spaces to template tag

<%_ if (primaryKey && primaryKey.hasUUID) { _%>
import java.util.UUID;
<%_ } _%>
<%_
const initDTO = (dtoEntity, entityNbr) => {
Copy link
Member

Choose a reason for hiding this comment

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

We should definitely not add a template function inside templates.

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.

None yet

5 participants