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

Support of Embedded and XXXToOne nested object projection + Support of @ProjectedFieldName on class fields #36594

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

humcqc
Copy link
Contributor

@humcqc humcqc commented Oct 20, 2023

Fixes #36581

Support of Embedded and ManyToOne nested object projection + Support of @ProjectedFieldName on class fields

Documentation not updated yet, will do it when we agree on the code.

@humcqc humcqc changed the title Support of Embedded and ManyToOne nested object projection + Support of @ProjectedFieldName on class fields Support of Embedded and XXXToOne nested object projection + Support of @ProjectedFieldName on class fields Oct 26, 2023
@loicmathieu
Copy link
Contributor

If I understand it correctly, the @ProjectedClass annotation allows for nested objects into a projection class. Can you provide a simple example?

Also, the documentation for projection should be updated.

@humcqc
Copy link
Contributor Author

humcqc commented Nov 17, 2023

https://github.com/quarkusio/quarkus/pull/36594/files#diff-41e4522791d7b2934d3189b9d2608d935564a31102f309bfb7f22e27e7aed968R56

"Also, the documentation for projection should be updated." , yes if you agree with the proposal , I will udpate the documentation

@loicmathieu
Copy link
Contributor

What I don't understand in your example is why the PersonDTO extends PeronsName and why it has the annotation in it. That's why I asked for a simple example.

@humcqc
Copy link
Contributor Author

humcqc commented Nov 17, 2023

What I don't understand in your example is why the PersonDTO extends PeronsName and why it has the annotation in it. That's why I asked for a simple example.

PersonName is the original DTO in integration test , I ve created PersonDTO that extend it to keep the original test and I've included Embbeded (DescriptionDTO) and ManyToOne ( AdressDTO) elements in it. It allow me to test different case with this DTO.

"Why there is an annotation in it ?" Good question. We have 3 choices:

  1. Impose the @ProjectedClass for all projected class and test if it present ( breaking change )
  2. Do not impose it, you can put it but the ProjectedClass annotation on main class have no impact -> what we have in the integration test.
  3. Just use it in nested class , and I can remove it from Main class

@loicmathieu
Copy link
Contributor

Just use it in nested class , and I can remove it from Main class

It is clearer to me, its behavior will be well defined.

PersonName is the original DTO in integration test , I ve created PersonDTO that extend it to keep the original test and I've included Embbeded (DescriptionDTO) and ManyToOne ( AdressDTO) elements in it. It allow me to test different case with this DTO.

Please let the current test as is and add a new one so we test both a simple projection and a more complex one.

@humcqc
Copy link
Contributor Author

humcqc commented Nov 17, 2023

Just use it in nested class , and I can remove it from Main class

It is clearer to me, its behavior will be well defined.

---> Ok , perhaps in that case we can rename it to NestedProjectedClass to be more precise ?

PersonName is the original DTO in integration test , I ve created PersonDTO that extend it to keep the original test and I've included Embbeded (DescriptionDTO) and ManyToOne ( AdressDTO) elements in it. It allow me to test different case with this DTO.

Please let the current test as is and add a new one so we test both a simple projection and a more complex one.

--> I didn't change the current test with simple Projection ( PersonName), it's just my new test use a new DTO ( PersonDTO) that inherit from the simple (PersonName) to avoid to duplicate the projection of simple field. And to test the inheritance, I had some issue with inheritance with the constructor part.
If you think it's simpler I can create a new class PersonName2 same as PersonName and PersonDTO will inherit this new Class ?

@humcqc
Copy link
Contributor Author

humcqc commented Nov 17, 2023

I've added simple example + doc, and renamed annotation.

Copy link

github-actions bot commented Nov 17, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@humcqc
Copy link
Contributor Author

humcqc commented Nov 17, 2023

Let me fix the jdk11 + native issue and i will come back to you.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

Overall, the PR seems more complex than needed.
There seem to be multiple functionalities in one PR and the tests seem contain more than just what's needed for the functionality.

This makes it complex to review so I didn't finish it yet.

@quarkus-bot

This comment has been minimized.

@humcqc
Copy link
Contributor Author

humcqc commented Nov 28, 2023

Overall, the PR seems more complex than needed. There seem to be multiple functionalities in one PR and the tests seem contain more than just what's needed for the functionality.

This makes it complex to review so I didn't finish it yet.

Yes you right , the PR is complex because it handles the recursivity of nested objects. And I included one test for #28844.
I will simplify and remove the test for #28844.

@quarkus-bot

This comment has been minimized.

@humcqc
Copy link
Contributor Author

humcqc commented Dec 10, 2023

Hi @loicmathieu , It should be good now, could you have a look please?

@quarkus-bot

This comment has been minimized.

humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
add testenpoint, remove comment, simplify getConstructor, rename getParametersFromClass,
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
…otated with @NestedProjectedClass

+ Remove test from PanacheFunctionalityTest, just use TestEndpoint test as requested by @loicmathieu
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
@humcqc
Copy link
Contributor Author

humcqc commented Jan 31, 2024

Hi @loicmathieu @geoand , @zakkak
I think this PR is fine like it is.
No needs to register for reflection the nested classes manually as it is done when we use the attribute ignoreNested = false.
And soon we will be able to remove this attribute when ignoreNested attribute will be false by default -> comment

Do you think it's possible to have this PR in 3.7.x ?
Thanks

@quarkus-bot

This comment has been minimized.

humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
add testenpoint, remove comment, simplify getConstructor, rename getParametersFromClass,
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
…otated with @NestedProjectedClass

+ Remove test from PanacheFunctionalityTest, just use TestEndpoint test as requested by @loicmathieu
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
humcqc added a commit to humcqc/quarkus that referenced this pull request Jan 31, 2024
@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Feb 1, 2024

Thanks for this.

We will certainly need to the commits to be cleaned up / squashed

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this and your patience for the long review process!

@loicmathieu
Copy link
Contributor

@humcqc can you please squash your commits in one commit so we can merge it?

@humcqc
Copy link
Contributor Author

humcqc commented Feb 1, 2024

@humcqc can you please squash your commits in one commit so we can merge it?

Sure, Done!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 1, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 1, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 758b9c1.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@loicmathieu loicmathieu merged commit 8edbd4b into quarkusio:main Feb 1, 2024
30 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 1, 2024
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Support of Embedded and ManyToOne nested object projection + Support of @ProjectedFieldName on class fields
3 participants