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.

Sign up for GitHub

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

Jump to bottom

Improve Panache projection with Hibernate ORM and Reactive #26510

Merged
merged 10 commits into from
Aug 10, 2022
Merged

Improve Panache projection with Hibernate ORM and Reactive #26510

merged 10 commits into from
Aug 10, 2022

Conversation

DavideD
Copy link
Contributor

@DavideD DavideD commented Jul 1, 2022 •
edited
Loading

Fixes #26284

It makes it possible to use projection with HQL queries with a select clause.

@loicmathieu this seems to work.

@DavideD DavideD mentioned this pull request Jul 1, 2022
Closed
@DavideD DavideD force-pushed the panache-project branch 14 times, most recently from 8b20bef to bbbffc5 Compare July 3, 2022 11:16
@DavideD DavideD marked this pull request as ready for review July 3, 2022 12:10
@quarkus-bot

This comment has been minimized.

Sign in to view
@DavideD DavideD force-pushed the panache-project branch from bbbffc5 to 8a35bd0 Compare July 3, 2022 14:05
loicmathieu
loicmathieu reviewed Jul 4, 2022
View reviewed changes
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, I was planning to ask for example with agregated query but you had the same idea and add them already ;)

Can you please update documentation / JavaDoc with a sentence explaing that if the query contains a select clause we do our best to transform it to a constructor expression, else we will build the query based on the constructor parameter list.

I would also like to have the opinion of @FroMage on this one now that the impl is matured and works.

@DavideD DavideD force-pushed the panache-project branch from 8a35bd0 to 364c1b4 Compare July 4, 2022 16:48
@DavideD
Copy link
Contributor Author

DavideD commented Jul 4, 2022

I don't mind updating the documentation, but I would like to make sure that we agree on this approach and we are going to merge it before updating the docs.

@DavideD DavideD force-pushed the panache-project branch from 364c1b4 to 22438f5 Compare July 9, 2022 12:44
@quarkus-bot

This comment has been minimized.

Sign in to view
@DavideD DavideD force-pushed the panache-project branch from 22438f5 to e99d9c2 Compare July 9, 2022 14:09
FroMage
FroMage requested changes Jul 18, 2022
View reviewed changes
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

This is great, IMO, and was missing. Can you do the same for HR and add documentation?

...me/src/main/java/io/quarkus/hibernate/orm/panache/common/runtime/CommonPanacheQueryImpl.java Outdated Show resolved Hide resolved
@DavideD DavideD force-pushed the panache-project branch 2 times, most recently from 966f6af to 90af07f Compare July 18, 2022 15:30
FroMage
FroMage approved these changes Jul 25, 2022
View reviewed changes
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

LGTM

@FroMage
Copy link
Member

FroMage commented Jul 25, 2022

@loicmathieu are you OK for me to merge this?

@loicmathieu
Copy link
Contributor

@FroMage yes, it's OK for me, I follow the discussion and had nothing to say ;)

gsmet
gsmet requested changes Aug 9, 2022
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

There are some problems in the documentation. The main one being the missing ['s. Always a good idea to do a doc rendering when introduction new sections that are not trivial.

docs/src/main/asciidoc/hibernate-orm-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-orm-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-orm-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-orm-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-reactive-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-reactive-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-reactive-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-reactive-panache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/hibernate-reactive-panache.adoc Outdated Show resolved Hide resolved
...c/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/CommonPanacheQueryImpl.java Show resolved Hide resolved
DavideD added 9 commits August 9, 2022 23:19
@DavideD
[quarkusio#26284] Support .project for queries with select clause
73c7a68 
User can now write an HQL with a select clause and project
will work. It will run a valid HQL that converts each tuple
in the result to selected type, as long as a valid constructor
exists.
@DavideD
[quarkusio#26284] Tests for .project with HQL queries having 'select'
534016b
@DavideD
[quarkusio#26284] Support select-distinct
28b6454
@DavideD
[quarkusio#26284] Test select-distinct with .project
e1152c8
@DavideD
[quarkusio#26284] Minor refactoring
b1fee37 
* Reduce number of `trim()` and `toLowerCase()` calls
* Remove unecessary calls to `.length()`
@DavideD
[quarkusio#26284] Add support for Hibernate Reactive
a023a7f 
Support `.project` with HQL queries with a select clause
@DavideD
[quarkusio#26284] Test .project with Hibernate Reactive Panache
c4dca52 
Add tests with HQL queries with a select clause and .project
@DavideD
[quarkusio#26284] Update ORM and Reactive Panache guides
6b37c9c 
Add example for Panache .project with HQL query with a select clause
@DavideD
[quarkusio#26284] Fix pre-existing typo in comment
b54010b
@DavideD
Copy link
Contributor Author

DavideD commented Aug 9, 2022 •
edited
Loading

I've updated the PR and added a commit that replaces all the it's to it is: 4b45773

@gsmet
Copy link
Member

gsmet commented Aug 9, 2022

Thanks!

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 9, 2022
@DavideD DavideD changed the title Improve Panache projection with Hibernate ORM Improve Panache projection with Hibernate ORM and Reactive Aug 10, 2022
@gsmet gsmet merged commit 8e4ee68 into quarkusio:main Aug 10, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Aug 10, 2022
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loicmathieu loicmathieu loicmathieu approved these changes

@gsmet gsmet gsmet approved these changes

@FroMage FroMage FroMage approved these changes

Assignees
No one assigned
Labels
area/documentation area/panache kind/bugfix
Projects
None yet
Milestone
2.12.0.CR1
Development

Successfully merging this pull request may close these issues.

Panache projection doesn't work with aggregate JPQL functions
4 participants
@DavideD @FroMage @loicmathieu @gsmet