Implement json data generation for more generic tests.

[mshima]

Related to #7112 and #14103

---

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

• All continuous integration tests are green
• Tests are added where necessary
• jhipster-online was updated if necessary
• Documentation is added/updated where necessary
• Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

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.

[yelhouti]

@mshima please do not merge this as per my comments here: #13864

[yelhouti]

/hold

[mshima]

It's open for almost a month. If there are no objections, I will go ahead and merge.

• The internal changes should be merged before jhipster 7 final.
• The internal changes are important to merge as soon as possible.
• Test generation apis will be useful for others features like Add relationships support to cypress. #14103.
• Non test changes are minor.

[yelhouti]

I am working on it, it should be ready this week. Please do no merge -

[mshima]

Again:

• Non test changes to angular are minor.
• Test changes will work for any approach.
• Internal changes will work for any approach.

This PR contains basic support for implementing #14103.
This discussion is too restricted, so unless we have some more opinions about this, I will go ahead and merge.

[yelhouti]

I am outside, I am using a different approach please give me the time to get back home and explain.

[yelhouti]

@mshima, Thank you for waiting, you can see in my PR how your changes contradicts mine and I have been working on/with them for months:
1- The fact the your adding the derived fields to the list of fields is a huge breaking changes and doesn't work as expected when the key is composite:

https://github.com/jhipster/generator-jhipster/pull/13860/files#diff-e9b17eaba9d089a2355e98ba3be2e8a8f053eab876e4128b025450595a88cffeR621

Plus it breaks all the places where the fields need to be filtered out.
2- The trackBy field is now sent from the backend which is inefficient so I am against that two.
3- I really appreciate all what you are doing for the project but please stop working on composite keys I am forced to do a huge rebase every time you intervene, and it's the main reason I wasn't able to release the feature yet. If you do not agree please PING someone neutral (one of the project leads) to take the final decision..

[mshima]

@mshima, Thank you for waiting, you can see in my PR how your changes contradicts mine and I have been working on/with them for months:

😕, I am aware you are trying hard to implement this feature.
And I'm sorry but from the first review I told you to split in smaller PRs and don't eagerly calculate ids.

1- The fact the your adding the derived fields to the list of fields is a huge breaking changes and doesn't work as expected when the key is composite:

https://github.com/jhipster/generator-jhipster/pull/13860/files#diff-e9b17eaba9d089a2355e98ba3be2e8a8f053eab876e4128b025450595a88cffeR621

If you take a look at #13864, there is even working test cases for composite ids.
So I really don't know what you are talking about.

Plus it breaks all the places where the fields need to be filtered out.
2- The trackBy field is now sent from the backend which is inefficient so I am against that two.

Inefficient 😕? Really? a string concatenation?

3- I really appreciate all what you are doing for the project but please stop working on composite keys I am forced to do a huge rebase every time you intervene, and it's the main reason I wasn't able to release the feature yet. If you do not agree please PING someone neutral (one of the project leads) to take the final decision..

The PR wasn't merged for this reason, to let core developers to give their opinion.

I feel you really should take into account JHipster is a team effort and we all want JHipster to be great.
So your PRs will not be always be merged as it is.
Project leads are receiving notification for every discussion, same as every core developer.
They don't manifest for their own reason:

• Lack of time for reviewing.
• Lack of interest.
• Neutral.
• The implementation is not their expertise.

So, if you want to ping them manually and anyone wants to give their opinion.
The team list and stream leaders is available at https://www.jhipster.tech/team/
There is no stream leader for JHipster Internals, you can ping angular and java leaders.

But JHipster 7 timeline is closing, and once closed, those internal changes may be considered breaking changes they will have to be postponed to JHipster 8.

[yelhouti]

And I'm sorry but from the first review I told you to split in smaller PRs and don't eagerly calculate ids.

@mshima You will have a PR with not eagerly calculated Ids this weak, please wait for it.

If you take a look at #13864, there is even working test cases for composite ids.

It does not work when you have a derivedId (OneToOne) with an entity that has a compositeKey (making both entities have a composite Id)

Inefficient confused? Really? a string concatenation?
what I mean by inefficient is: the data is sent twice in a DTO. ex:

• id.employeeUsername and employee.username
• id.taskId and task.id

If you still don't agree please tell me and I will ping someone.

EDIT: Also you said:

I am aware you are trying hard to implement this feature.

I have a blueprint maintained for years now with composite support, what I have been trying to do is rebase every time with a way you like. And now that I saw what you proposed in you PR, I am able to do that. Please check my new PR and let me know.

EDIT: backend/java part with it's tests should be ready by tomorrow.

[yelhouti]

@mshima so you think it's ok to discard what I said, don't wait to ask for someone else opinion and do whatever you want in the project ? You know that I am the one assigned to implement the primaryKey functionality, I have have been updating my PR each time you asked for a change and on top of all of that you are adding changes that breaks myPR every few days.
can someone please intervene @julienDubois @deepu105 @pascalgrimaud .

[deepu105]

Gentlemen, let me step in before we start throwing keyboards at each other, trust me the keyboards these days break too fast 😂

Now on a serious note, I think approaching this as multi step problem would be beneficial to everyone involved. I don't like composite IDs very much as I think they overcomplicate stuff and there is always simpler solutions, but thats just my opinion as I'm a fan of simple code. I see that there is interest in composite keys and you guys disagree on the approach. May I suggest you break this down to multiple PRs with below approach

1.) Move all changes which has nothing much to do with composite keys but are generic improvements into the first PR so that we can merge it for v7
2.A) Both of you can present your own seperate PR for composite key solution (feel free to revert any change that was merged) and myself (and anyone else interested) can review and vote on an approach we like. Be prepared to have your PR rejected as well if we think it adds too much overall complexity etc
2.B) You both can may be arrange a call and discuss this and come to some compromise on what needs to be done and do that in the PR (the keyword here is compromise)
2.C) Drop this feature for v7

[yelhouti]

thank you @deepu105 for your lighthearted comment and for intervening.
I admit I was pissed yesterday when I saw that he merged code again breaking my PR (for the Nth time), even after I specifically asked him to hold.
After I calmed down, I rebase my work on top of his.

The reason I asked for one of you to intervene though is this, and it goes beyond the feature and it's implementation :

I feel it's very insensitive of him not to take my comments into account and merge just because his has the ability to do so. This is open source, we all do our best to have the best solution possible, and if someone raises a concern we should at least ask for a second opinion not ignore them and move forward.

Also please do not drop this for v7 I have been working on this for month and I have a working solution:

#12819

[deepu105]

@yelhouti @mshima I'm pretty sure both of you want what is best for the project and yes things can get bit heated sometimes and thats normal as long as we keep cordial about it and can have a laugh later on the same. As an OSS community it is extremely important for us to feel welcomed and valued for our contributions as we do all this for free on our own time out of passion for OSS and the community. Lets forget what has already happened and lets see how we can move forward with this. Even though composite keys are not something I'm interested in, I'm willing to do the reviews and help you guys reach a compromise.

Now as for this PR, which is merged, I dont see any huge issue and this doesnt look like it has much to do with composite key persé, so let me suggest the below to move forward unless you both are willing to contribute on a same PR

1. @mshima lets not merge anything further (unless its a bug fix) which might affect composite keys
2. @mshima @yelhouti create a PR each with a composite key implementation that you think is ideal
3. I'll review both and suggest changes where applicable (ofcourse everyone else is welcome to review as well if interested), I might also suggest taking a part from each of your PR, and might make changes of my own if applicable, as well and i'll merge the final solution that I find is most suitable (unless @jdubois or @pascalgrimaud disagrees)

Is this agreeable to both of you?

PS: Do not take these arguments and stuff too much to heart and do not generate any resentment for each other. Who knows you guys might end up as beer buddies. I have stepped on some of the core contributors toes a few times in the past and now we all drink beer and hang out when we see each other for JH conference so having good relationships in the community is definelty way more important then having perfect code.

And if I may share my wisdom, there is no such thing as perfect code other wise we all will be out of work 😂

[yelhouti]

@deepu105 thank you very much for the words of wisdom, I am more than willing to work on the same PR with @mshima I really appreciate all what he did/is doing for the project. Faux pas Happens, it's under the rug for me. Thank you all guys, hope to see you all in the next conference !

[mshima]

@deepu105 thanks.

I don't like composite IDs very much as I think they overcomplicate stuff and there is always simpler solutions, but thats just my opinion as I'm a fan of simple code.

Actually I don't care about composite IDs enough too, but I care about it been implemented right in jhipster.

May I suggest you break this down to multiple PRs with below approach

1.) Move all changes which has nothing much to do with composite keys but are generic improvements into the first PR so that we can merge it for v7

I am suggesting this from the start.
A good point of start would be #14108.
Some features like reactive, filtering/criteria changes and others should be dropped from the initial PR.
Why? Different developers reviews those features.

Now as for this PR, which is merged, I dont see any huge issue and this doesnt look like it has much to do with composite key persé

The changes related to composite id is just some fixes in the primaryKey structure, which I'm the original author.
I merged this PR because it generates json structure that will be used for #14103.

1. @mshima lets not merge anything further (unless its a bug fix) which might affect composite keys
2. @mshima @yelhouti create a PR each with a composite key implementation that you think is ideal
3. I'll review both and suggest changes where applicable (ofcourse everyone else is welcome to review as well if interested), I might also suggest taking a part from each of your PR, and might make changes of my own if applicable, as well and i'll merge the final solution that I find is most suitable (unless @jdubois or @pascalgrimaud disagrees)

Is this agreeable to both of you?

Since no core developer really care about composite ids, I thought let's treat it as a natural id and start to implement a feature that some core developer actually cares about. And is a littler less error prone.
But this mainly affects the frontend.

IMO we should:

• Decide which approach to use in the frontend.
• Create a PR cleaning up @MapsId. There is a lot of leftovers.
• Implement Allow to specify in id if we want it autoGenerated #14108.
  Every composite id is not auto generated, so this is a must have feature.

@deepu105 you to go ahead and review #12819 like I did several times, but I would be concerned to merge a PR that touches too many things at once, even if it was ready.
Unless the old code was identical, but is far from identical.

IMO the referred PR contains:

• recursive functions that could be avoided. It's harder to understand and many times only makes sense to the original author.
• changes database column names for all @MapsId relationships, major break change that should be avoided.
• others changes in internals that I'm not comfortable with.

[deepu105]

@mshima thanks for the explanation

@yelhouti I tend to agree with @mshima on some of his concerns. Your PR (#12819) is huge and would take a lot of review effort, its seems like you are trying to do #7112 and #12707 in the same PR (Sorry if Im wrong as I didnt follow this close enough). I'm 100% against this even if one is not possible without the other, I know it might be easier to implement together but is definitely not easy to review/test so I suggest you split that PR into 3

1. Changes required strictly to make composite keys work - This way you can get the feature into v7 even if the code is not perfect. Avoid any other code improvements, optimizations or internal changes so that its easier for me to review and test
2. Changes required stricly for [v6] 12072 rollback reactive #12716 (Flip one and two if the dependency is reverse)
3. Any code improvements/optimizations that you like to do to internals

Alos lets try not to do breaking changes unless its absolutely necessary as it would make upgrading even more difficult for end users. We can make consistency improvements in the next major version as well
IMO this would be a bit more work to you but this would make reviews easier and will avoid unnecssary loggerheads and heated debates. WDYT?

@DanielFran since you also did some reviews on the PR what is your take?

[MathieuAA]

@mshima Hello there, I understand you're not comfortable saying "let's merge it". What would make you more comfortable? More tests? If yes, of what kind? More reviews?

[yelhouti]

@deepu105 I tried working on criteria flattening slow first but they require changes in QueryService #13133 can you or anyone approve the proposed changes so I can work on it?

[deepu105]

Why is that a necessity. Filtering is not that widely used and Composite id is not gonna be used by majority of our users (we should still do simple IDs by default) so it is ok not to have all options working with it. We can enable/disable the option when other options are choosen. And then gradually you can add support for other options? I would say get this working for default options first and ship it. Then you can start making it work for other options

[yelhouti]

@deepu105 I will try to answer/ask questions in the other issue, I will tag you there.