-
Notifications
You must be signed in to change notification settings - Fork 3k
Introduce support for hibernate.type.preferred_*_jdbc_type
properties
#50454
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
base: main
Are you sure you want to change the base?
Conversation
/cc @gsmet (hibernate-orm) |
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.
Thank you. I added a few comments below.
extensions/hibernate-orm/deployment/src/test/resources/application.properties
Outdated
Show resolved
Hide resolved
...eployment/src/test/java/io/quarkus/hibernate/orm/config/properties/ConfigPropertiesTest.java
Show resolved
Hide resolved
@yrodiere thanks saw it, is still in draft so I will take them in account while i will be writing the remaining part! Thanks a lot 🙏🏻 |
@yrodiere First issue so i don't know if i missed something :) |
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.
Thanks, but the config group stuff and property naming is really off. See my comment in the conversation above for a suggestion on how to do it :)
hibernate.type.preferred_*_jdbc_type
properties
0bacffd
to
f36b2e2
Compare
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.
Thanks. The properties shouldn't be in application.properties
in this case (see below). And ideally it would be great to have an integration test, but I'll let you see if you have time for that :)
extensions/hibernate-orm/deployment/src/test/resources/application.properties
Outdated
Show resolved
Hide resolved
...eployment/src/test/java/io/quarkus/hibernate/orm/config/properties/ConfigPropertiesTest.java
Outdated
Show resolved
Hide resolved
|
||
@Test | ||
@Transactional | ||
void shouldMapHibernateOrmConfigPersistenceUnitMappingDurationProperties() { |
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.
These tests are great, but technically they just check that we're passing configurations over to Hibernate ORM.
Considering the number of things that could go wrong -- especially in native mode -- it would be great if you could also check in an actual integration tests that everything works correctly?
E.g. you could take inspiration from the "default catalog and schema" test case:
- Set up a REST resource that expose an endpoint executing a smoke test on some entity (checks it can be persisted/loaded), and another one that runs a native query (checks the schema does match the expected SQL type): https://github.com/quarkusio/quarkus/blob/18d103499400bbeb7aafd296672fe7128e2f42cf/integration-tests/jpa/src/main/java/io/quarkus/it/jpa/defaultcatalogandschema/DefaultCatalogAndSchemaResource.java
- Set up a test that calls these endpoints, and a second test that extends the first but just re-executes the test in native mode: https://github.com/quarkusio/quarkus/tree/ca3f7d8c442cfcf3156d9bbed4b8a14b81156a44/integration-tests/jpa/src/test/java/io/quarkus/it/jpa/defaultcatalogandschema
Would you mind adding that? Then we'd be completely sure the feature works.
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'm gonna add them as well!
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.
@yrodiere Added them. Not sure if you want me to add more specific assertions? I was reading the metadata of the table and it seems that the H2 dialect is remapping some preferred types to its specific types. But seems that the preferred types property mapping is working, since the id column is remapped as CHAR.
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.
You could add something like this to your test
endpoint?
var metamodel = session.getFactory().unwrap(SessionFactoryImplementor.class).getMappingMetamodel()
.findEntityDescriptor(EntityWithOverridablePreferredTypes.class);
assertThat(metamodel.getIdentifierMapping().getSingleJdbcMapping().getJdbcType()
.getJdbcTypeCode())
.isEqualTo(SqlTypes.CHAR);
assertThat(metamodel.getAttributeMapping(metamodel.getPropertyIndex("createdAt")).getSingleJdbcMapping().getJdbcType()
.getJdbcTypeCode())
.isEqualTo(SqlTypes.INSTANT);
// TODO do the same for other properties
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview 43e231e has been successfully built and deployed to https://quarkus-pr-main-50454-preview.surge.sh/version/main/guides/
|
e22b044
to
dd2bfcf
Compare
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.
Thanks! Looking great, though I suspect native tests will fail. See below.
I'll launch CI to be sure...
@Override | ||
public Map<String, String> getConfigOverrides() { | ||
return Map.of( | ||
"quarkus.hibernate-orm.database.default-schema", "SCHEMA1", |
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.
No need to customize the schema, it's not relevant to this test.
"quarkus.hibernate-orm.database.default-schema", "SCHEMA1", |
given().queryParam("expectedSchema", "SCHEMA1") | ||
.when().get("/jpa-test/overridden-preferred-types/test").then() |
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.
No need to customize the schema, it's not relevant to this test.
given().queryParam("expectedSchema", "SCHEMA1") | |
.when().get("/jpa-test/overridden-preferred-types/test").then() | |
when().get("/jpa-test/overridden-preferred-types/test").then() |
public String test(@RestQuery String expectedSchema) { | ||
assertThat(findUsingNativeQuery(expectedSchema)).isEmpty(); |
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.
No need to customize the schema, it's not relevant to this test.
public String test(@RestQuery String expectedSchema) { | |
assertThat(findUsingNativeQuery(expectedSchema)).isEmpty(); | |
public String test() { | |
assertThat(findUsingNativeQuery()).isEmpty(); |
session.flush(); | ||
session.clear(); | ||
|
||
assertThat(findUsingNativeQuery(expectedSchema)).containsExactly(entity.id); |
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.
No need to customize the schema, it's not relevant to this test.
assertThat(findUsingNativeQuery(expectedSchema)).containsExactly(entity.id); | |
assertThat(findUsingNativeQuery()).containsExactly(entity.id); |
private List<UUID> findUsingNativeQuery(String schema) { | ||
return session.createNativeQuery( | ||
""" | ||
SELECT id FROM "%s".%s WHERE isPersisted = :isPersisted | ||
""".formatted(schema, EntityWithOverridablePreferredTypes.NAME) |
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.
No need to customize the schema, it's not relevant to this test.
private List<UUID> findUsingNativeQuery(String schema) { | |
return session.createNativeQuery( | |
""" | |
SELECT id FROM "%s".%s WHERE isPersisted = :isPersisted | |
""".formatted(schema, EntityWithOverridablePreferredTypes.NAME) | |
private List<UUID> findUsingNativeQuery() { | |
return session.createNativeQuery( | |
""" | |
SELECT id FROM %s WHERE isPersisted = :isPersisted | |
""".formatted(EntityWithOverridablePreferredTypes.NAME) |
|
||
@Test | ||
@Transactional | ||
void shouldMapHibernateOrmConfigPersistenceUnitMappingDurationProperties() { |
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.
You could add something like this to your test
endpoint?
var metamodel = session.getFactory().unwrap(SessionFactoryImplementor.class).getMappingMetamodel()
.findEntityDescriptor(EntityWithOverridablePreferredTypes.class);
assertThat(metamodel.getIdentifierMapping().getSingleJdbcMapping().getJdbcType()
.getJdbcTypeCode())
.isEqualTo(SqlTypes.CHAR);
assertThat(metamodel.getAttributeMapping(metamodel.getPropertyIndex("createdAt")).getSingleJdbcMapping().getJdbcType()
.getJdbcTypeCode())
.isEqualTo(SqlTypes.INSTANT);
// TODO do the same for other properties
"quarkus.hibernate-orm.mapping.duration.preferred-jdbc-type", "INTERVAL_SECOND", | ||
"quarkus.hibernate-orm.mapping.instant.preferred-jdbc-type", "INSTANT", | ||
"quarkus.hibernate-orm.mapping.boolean.preferred-jdbc-type", "BIT", | ||
"quarkus.hibernate-orm.mapping.uuid.preferred-jdbc-type", "CHAR" |
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.
These are build-time properties, so I'm not sure they can be mentioned in a test profile for native (graalVM) tests... the properties cannot be set after native compilation.
If native tests fail, you will know why :) And in that case I'd suggest simply defining a separate (named) persistence unit where you can set all the properties you want. We've done something like this in other ITs for other features, e.g. there:
quarkus/integration-tests/jpa-mariadb/src/main/resources/application.properties
Lines 9 to 16 in 3e0bad9
quarkus.datasource."offline".db-kind=mariadb | |
quarkus.datasource."offline".db-version=10.11 | |
quarkus.hibernate-orm."offline".database.start-offline=true | |
quarkus.hibernate-orm."offline".dialect.mariadb.bytes-per-character=1 | |
quarkus.hibernate-orm."offline".dialect.mariadb.no-backslash-escapes=true | |
#quarkus.hibernate-orm."offline".dialect.mariadb.storage-engine=MY_CUSTOM_ENGINE // this fails with Unable to determine Dialect for MariaDB 10.6 (please set 'hibernate.dialect' or register a Dialect resolver | |
quarkus.hibernate-orm."offline".datasource=offline | |
quarkus.hibernate-orm."offline".packages=io.quarkus.it.jpa.mariadb |
Status for workflow
|
Status | Name | Step | Failures | Logs | Raw logs | Build scan |
---|---|---|---|---|---|---|
❌ | Initial JDK 17 Build | Build |
Failures | Logs | Raw logs | 🔍 |
You can consult the Develocity build scans.
Failures
⚙️ Initial JDK 17 Build #
- Failing: integration-tests/jpa
📦 integration-tests/jpa
❌ Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.27.0:validate (default) on project quarkus-integration-test-jpa: File '/home/runner/_work/quarkus/quarkus/integration-tests/jpa/src/main/java/io/quarkus/it/jpa/preferredhibernatetypesoverride/OverriddenPreferredTypesResource.java' has not been previously formatted. Please format file (for example by invoking `mvn -f integration-tests/jpa net.revelc.code.formatter:formatter-maven-plugin:2.27.0:format`) and commit before running validation!
Status for workflow
|
Add Quarkus properties to map the following hibernate properties:
hibernate.type.preferred_duration_jdbc_type : quarkus.hibernate-orm.mapping.duration.preferred_jdbc_type
hibernate.type.preferred_instant_jdbc_type : quarkus.hibernate-orm.mapping.duration.preferred_instant_jdbc_type
hibernate.type.preferred_boolean_jdbc_type : quarkus.hibernate-orm.mapping.preferred.boolean_jdbc_type
hibernate.type.preferred_uuid_jdbc_type : quarkus.hibernate-orm.mapping.preferred.uuid_jdbc_type
Fixes Support properties
hibernate.type.preferred_*_jdbc_type
#33328