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

Allow passing unsupported properties to Hibernate ORM #26128

Merged
merged 2 commits into from Jun 21, 2022

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Jun 14, 2022

Fixes #19129.

Creating as draft because this PR is based on #26126, which needs to be merged first. => Done

In short, this adds a configuration property which can be used like this:

quarkus.hibernate-orm.unsupported-properties."hibernate.order_inserts" = true

IMO, the name makes it clear that such properties will not necessarily work, but the documentation makes it extra-clear:

     * Properties that should be passed on directly to Hibernate ORM.
     * Use the full configuration property key here,
     * for instance `quarkus.hibernate-orm.unsupported-properties."hibernate.order_inserts" = true`.
     *
     * [WARNING]
     * ====
     * Properties set here are completely unsupported:
     * as Quarkus doesn't generally know about these properties and their purpose,
     * there is absolutely no guarantee that they will work correctly,
     * and even if they do, that may change when upgrading to a newer version of Quarkus
     * (even just a micro/patch version).
     * ====
     *
     * Consider using a supported configuration property before falling back to unsupported ones.
     * If none exists, make sure to file a feature request so that a supported configuration property can be added to Quarkus,
     * and more importantly so that the configuration property is tested regularly.

To be even safer, setting such a property triggers a warning on startup:

Persistence-unit [<default>] sets unsupported properties. These properties may not work correctly, and even if they do, that may change when upgrading to a newer version of Quarkus (even just a micro/patch version). Consider using a supported configuration property before falling back to unsupported ones. If there is no supported equivalent, make sure to file a feature request so that a supported configuration property can be added to Quarkus, and more importantly so that the configuration property is tested regularly. Unsupported properties being set: [hibernate.order_inserts]

Also, such properties have the lowest precedence. If Quarkus needs to set the same property as the user for whatever reason, Quarkus will win, and another warning will be displayed on startup:

Persistence-unit [<default>] sets property 'hibernate.order_inserts' to a custom value through 'quarkus.hibernate-orm.unsupported-properties."hibernate.order_inserts"', but Quarkus already set that property independently. The custom value will be ignored.

I think that should address all of @Sanne's concerns?

@Sanne
Copy link
Member

Sanne commented Jun 15, 2022

Yes, thanks this is a very sensible approach and resolves my concerns.

One thing to remember is that some properties are applied by optional Service implementations - occasionally we override these implementations, so it's possible that some properties will be silently ignored.
I'm not saying this to block this approach but just in terms to keep it in mind; perhaps for future evolutions we can try to track such situations and also log a warning. Perhaps document that such "unsupported properties" are not guaranteed to being applied - people should test their effect is working as intended.

@yrodiere
Copy link
Member Author

Thanks.

Perhaps document that such "unsupported properties" are not guaranteed to being applied - people should test their effect is working as intended.

Right. I think this should cover it?

     * [WARNING]
     * ====
     * Properties set here are completely unsupported:
     * as Quarkus doesn't generally know about these properties and their purpose,
     * there is absolutely no guarantee that they will work correctly,
     * and even if they do, that may change when upgrading to a newer version of Quarkus
     * (even just a micro/patch version).
     * ====

I didn't add a dedicated section to the documentation because I didn't want to encourage people to use this. If they really need it, it's in the configuration reference, and we can always point them to it.

@yrodiere yrodiere force-pushed the i19129-unsupported-properties branch from 64f585e to e8f6bdd Compare June 16, 2022 15:20
@yrodiere yrodiere marked this pull request as ready for review June 16, 2022 15:20
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@yrodiere yrodiere force-pushed the i19129-unsupported-properties branch from e8f6bdd to 3165949 Compare June 17, 2022 08:47
@quarkus-bot

This comment has been minimized.

@yrodiere yrodiere force-pushed the i19129-unsupported-properties branch from 3165949 to 1c4c41b Compare June 17, 2022 12:50
@quarkus-bot

This comment has been minimized.

@yrodiere yrodiere marked this pull request as draft June 17, 2022 14:50
@yrodiere
Copy link
Member Author

I'll have a closer look at test failures on Monday; I suspect some tests are having side-effects on others...

@yrodiere
Copy link
Member Author

I'll have a closer look at test failures on Monday; I suspect some tests are having side-effects on others...

The problem was how QuarkusUnitTest handles @QuarkusTestResource: basically it only takes into account "global" resources and those that are defined on the very first test to run.

Anyway, that doesn't fit what we need to do in UnsupportedPropertiesTest, so I just added a overrideRuntimeConfigKey method to QuarkusUnitTest.

@yrodiere yrodiere marked this pull request as ready for review June 20, 2022 12:43
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 20, 2022

Failing Jobs - Building 4ec38ce

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/resteasy-reactive/rest-client-reactive-jaxb/deployment 

📦 extensions/resteasy-reactive/rest-client-reactive-jaxb/deployment

io.quarkus.rest.client.reactive.jaxb.test.SimpleJaxbTest.shouldConsumePlainXMLEntity - More details - Source on GitHub

org.jboss.resteasy.reactive.ClientWebApplicationException: Received: 'Internal Server Error, status code 500' when invoking: Rest Client method: 'io.quarkus.rest.client.reactive.jaxb.test.SimpleJaxbTest$XmlClient#plain'
	at org.jboss.resteasy.reactive.client.impl.RestClientRequestContext.unwrapException(RestClientRequestContext.java:170)
	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.handleException(AbstractResteasyReactiveContext.java:317)

io.quarkus.rest.client.reactive.jaxb.test.SimpleJaxbTest.shouldConsumeXMLEntity - More details - Source on GitHub

org.jboss.resteasy.reactive.ClientWebApplicationException: Received: 'Internal Server Error, status code 500' when invoking: Rest Client method: 'io.quarkus.rest.client.reactive.jaxb.test.SimpleJaxbTest$XmlClient#dto'
	at org.jboss.resteasy.reactive.client.impl.RestClientRequestContext.unwrapException(RestClientRequestContext.java:170)
	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.handleException(AbstractResteasyReactiveContext.java:317)

@yrodiere
Copy link
Member Author

The failing tests are unrelated, see #26251

@gsmet gsmet merged commit 2302e13 into quarkusio:main Jun 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 21, 2022
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jun 21, 2022
@Sanne
Copy link
Member

Sanne commented Jun 21, 2022

Nice, thanks!

@kay-horst
Copy link
Contributor

Hello! This is a very useful addition, thanks a lot!

@yrodiere yrodiere linked an issue Jul 20, 2022 that may be closed by this pull request
@yrodiere yrodiere deleted the i19129-unsupported-properties branch August 2, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants