-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Hibernate panache codestart #21651
Hibernate panache codestart #21651
Conversation
7d3fb37
to
6cbfe22
Compare
dd18377
to
0763a08
Compare
@ia3andy I merged the codestarts |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 0763a08
Full information is available in the Build summary check run. Failures⚙️ Devtools Tests - JDK 11 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
⚙️ Devtools Tests - JDK 17 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
|
0763a08
to
e974f1f
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building e974f1f
Full information is available in the Build summary check run. Failures⚙️ Native Tests - gRPC #- Failing: integration-tests/grpc-mutual-auth integration-tests/grpc-plain-text-mutiny
📦 integration-tests/grpc-mutual-auth✖
✖
✖
✖
📦 integration-tests/grpc-plain-text-mutiny✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
|
Is there any particular reason for the non-panache entity to use getters+setters while the Panache version uses filed instrumentation? Unless I'm missing something, it would be nice to showcase that both approaches can be used without the "ceremonial" getters and setters overhead. |
@Sanne I was thinking that the field enhancement was a Panache thing, if you look at the Hibernate ORM guide it shows a regular JPA entity: https://quarkus.io/guides/hibernate-orm#setting-up-and-configuring-hibernate-orm I was thinking the bytecode transformer to transform field access to accesor was this one: https://github.com/quarkusio/quarkus/blob/main/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java But maybe I'm missing something ? |
@loicmathieu no it's an Hibernate ORM thing, Panache only adds additional capabilities to it |
@Sanne it should be documented then :), as on the Panache guide it is said that it's a Panache feature (see point 2): https://quarkus.io/guides/hibernate-orm-panache#how-and-why-we-simplify-hibernate-orm-mappings I'll update the quickstart. |
ah good point - yes I see what you mean. To be clear the Panache doc isn't entirely wrong, as when using Panache the getters/setters are added by Panache - but the doc is indeed misleading as ORM "alone" would do the same. |
a41c9e6
to
097b872
Compare
@Sanne I updated the Hibernate ORM with public fields instead of getters§setters |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be merged until we figure out #22773 .
I closed #22773 as not planned. @loicmathieu can you please rebase your PR? Thanks. |
@Sanne is it really the case? I remember from years ago that the bytecode enhancer for field accesses wasn't enabled for Hibernate without Panache. The getters are generated alright, but nothing is there in Quarkus to rewrite the field accesses from outside the entity. Only Panache does that AFAIR. |
@FroMage it's called extended enhancement in Hibernate ORM, it's disabled by default but you could enable it to let it do it for you. |
097b872
to
f86e5bb
Compare
It is. I worked on that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good
...ension-codestarts/hibernate-orm-codestart/java/src/main/java/org/acme/MyEntity.tpl.qute.java
Outdated
Show resolved
Hide resolved
771e3c5
to
b57455d
Compare
This comment has been minimized.
This comment has been minimized.
b57455d
to
613a05a
Compare
Failing Jobs - Building 613a05a
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 17 MacOS M1 #- Failing: integration-tests/mailer integration-tests/smallrye-opentracing
📦 integration-tests/mailer✖
📦 integration-tests/smallrye-opentracing✖
|
@yrodiere PR is updated and all test pass, can you approve it ? |
@loicmathieu sorry, I was on leave. Will have a look ASAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to test this locally (can't seem to use codestarts from a locally-built Quarkus). But the code looks good, so I'll just trust the tests :)
No description provided.