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

Avoid StackoverflowError when updating entity #18866

Conversation

OmarHawk
Copy link
Contributor

@OmarHawk OmarHawk commented Jun 8, 2022

when elasticsearch is enabled. We do add Spring Data
Transient annotation on the non-owning side
of relationships to break the artificial cycle introduced with the
bidirectional relationship model

Fix #18729
Fix #14840
Fix #16136


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.

@ko5tik
Copy link
Contributor

ko5tik commented Jun 8, 2022

You should consider adding unit tests for this feature or extend existing with assertions about generated annotations
( under directory test/* )

@OmarHawk OmarHawk force-pushed the feature/avoid-stackoverflowerror-when-reindexing-entities-with-relationships branch from bbd86f2 to b03e55c Compare June 8, 2022 13:53
@OmarHawk
Copy link
Contributor Author

OmarHawk commented Jun 8, 2022

You should consider adding unit tests for this feature or extend existing with assertions about generated annotations ( under directory test/* )

True, ideally, there would be tests that check the actual functionality and not just the presence of some annotations. I'll see, what I can do easily.

when elasticsearch is enabled. We do add Spring Data
Transient annotation on the non-owning side
of relationships to break the artificial cycle introduced with the
bidirectional relationship model

Fixes jhipster#18729
@OmarHawk OmarHawk force-pushed the feature/avoid-stackoverflowerror-when-reindexing-entities-with-relationships branch from b03e55c to f697f33 Compare June 8, 2022 14:45
@OmarHawk
Copy link
Contributor Author

OmarHawk commented Jun 8, 2022

Meh, due to the changes of #17488 entities with custom id names will fail compilation 👎

Will fix this at well.

@OmarHawk
Copy link
Contributor Author

OmarHawk commented Jun 8, 2022

Ok, so whenever I put a required validation attribute to an entity's relationship, tests do get generated that perform also the previously failing things (I tested manually for now with my jdl that produced failing PUT methods).
This is already there, so I'd just extend some sample jdls with this attribute and then we are supposed to see things working or not working :-)

@OmarHawk OmarHawk force-pushed the feature/avoid-stackoverflowerror-when-reindexing-entities-with-relationships branch from f9b6a78 to b0a4536 Compare June 8, 2022 17:13
@ko5tik
Copy link
Contributor

ko5tik commented Jun 8, 2022

You should consider adding unit tests for this feature or extend existing with assertions about generated annotations ( under directory test/* )

True, ideally, there would be tests that check the actual functionality and not just the presence of some annotations. I'll see, what I can do easily.

I mean unit tests for generator itself. And maybe also for CI

@OmarHawk OmarHawk marked this pull request as ready for review June 9, 2022 06:39
@OmarHawk
Copy link
Contributor Author

OmarHawk commented Jun 9, 2022

I mean unit tests for generator itself. And maybe also for CI

Well, during the CI here, plenty of applications are produced and their automated tests are being run, so adding this case into the sample applications' datamodel to reproduce the case should be sufficient.

@DanielFran DanielFran merged commit 710234a into jhipster:main Jun 13, 2022
@DanielFran
Copy link
Member

Thanks @OmarHawk for the contribution.
Do not forget to claim the bug bounty in #14840

@OmarHawk
Copy link
Contributor Author

Thanks @OmarHawk for the contribution. Do not forget to claim the bug bounty in #14840

Thanks. :)
Claiming via https://opencollective.com/generator-jhipster/expenses/82218

@DanielFran
Copy link
Member

@OmarHawk approved

@DanielFran DanielFran added this to the 7.9.0 milestone Jun 22, 2022
@mheidt
Copy link

mheidt commented Apr 27, 2023

Are you guys certain, that @transient is the solution?
Yes, it prevents the stack overflow. But the index is empty as well.
This is the issue:
You have a relationship chain from A->B->A
I want either A->B in my elastic index as well as B->A
A solution with transient breaks either one of them.
And a Mapstruct solution doesn't work as the generated setters are rewiring it again.

@OmarHawk
Copy link
Contributor Author

Well, at least it avoids that the functionality is not available at all. And what we currently do is generating the transient annotation only in one direction of the relationship if I remember my thoughts about that again, so that the index is not really empty.

What this Pull Request also does not do is checking whether someone actually created a cycle in it's data model... this would also requiring to break the cycle somehow...

@mheidt
Copy link

mheidt commented May 3, 2023

Yes, and in most cases this solves the case, because you don't need both directions.
The rewiring of the parent relation rather bothers me.
At least there should be an additional setter that is not doing it, so that I can try to make mapstruct use that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants