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

Trim string values defined in the Hibernate ORM configuration #22816

Merged
merged 4 commits into from
Jan 12, 2022

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Jan 11, 2022

Fixes #20492

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

+1 to merge this, but... why in the world is trimming not the default?

I suppose it's something related to the Microprofile spec, or an ongoing battle with someone else arguing we should do no such thing?

I cannot think of any property (at least in Hibernate ORM/Search) where we would assign meaning to a leading/trailing space. And even if there was one, it would be much safer to add an exception for that property than to manually use TrimmedStringConverter on every single property except the one that should not be trimmed...

Hibernate Search, internally, certainly does trim everything before processing any configuration property, but any processing in the Hibernate Search (e.g. processing a bean reference extracted from the configuration properties) is at risk too. So I guess I'll have to do the same as you for the Hibernate Search extension... ?

gsmet added 4 commits January 12, 2022 11:46

Verified

This commit was signed with the committer’s verified signature.
gsmet Guillaume Smet
After discussion with Roberto and per the MP Config spec,
TrimmedStringConverter should return null if the string is empty.

It fixes the semantic and allows to use with Optional<String>.

Verified

This commit was signed with the committer’s verified signature.
gsmet Guillaume Smet

Verified

This commit was signed with the committer’s verified signature.
gsmet Guillaume Smet

Verified

This commit was signed with the committer’s verified signature.
gsmet Guillaume Smet
…onfig
@gsmet
Copy link
Member Author

gsmet commented Jan 12, 2022

That would be a question for @radcortez . I must admit that I agree with you. We had a discussion about this back in the days with David and in the end the initial behavior was kept. IIRC the main argument for it was to respect how .properties work and their trimming rules.

I could see how you would want to not trim user-defined properties though (think email subject prefix for instance). But I agree that would be marginal.

That being said, I'm not sure that's something we can break now.

@radcortez
Copy link
Member

I suppose it's something related to the Microprofile spec, or an ongoing battle with someone else arguing we should do no such thing?

No :)

IIRC the main argument for it was to respect how .properties work and their trimming rules.

Exactly.

Maybe what we can do is trim properties in the quarkus namespace and leave the rest.

@yrodiere
Copy link
Member

Maybe what we can do is trim properties in the quarkus namespace and leave the rest.

That would be awesome. We can always override the converter if we don't need trimming for one exotic property.

@radcortez
Copy link
Member

Maybe what we can do is trim properties in the quarkus namespace and leave the rest.

That would be awesome. We can always override the converter if we don't need trimming for one exotic property.

Exactly. @gsmet do you remember any config on our side where we may want to conserve the original value?

@gsmet
Copy link
Member Author

gsmet commented Jan 12, 2022

do you remember any config on our side where we may want to conserve the original value?

That's a good question, thanks for asking... :). I think the only way to really know that would be to do a quick scan of all the configuration properties in the configuration reference page. But I can't do that anymore with my bad eyes :/.

I'm not sure having a different behavior for a specific namespace is a good idea, though.

In any case:

  • we need to discuss this on the dev list, not in a somewhat unrelated PR
  • we won't do that for 2.7, it's 2.8 material so I think we should get this PR in anyway

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 12, 2022
@radcortez
Copy link
Member

Sure. I was not proposing to do it here.

I'll open a separate issue so we don't forget.

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 12, 2022

Failing Jobs - Building bfabc1c

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.InjectQuarkusAppPropertiesDevModeTest.main line 19 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@gsmet gsmet merged commit ce96b00 into quarkusio:main Jan 12, 2022
@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Jan 12, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class PostgreSQL10Dialect not found on Quarkus in Google App Engine java11?
3 participants