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

Take into account that configuration properties can contain a dot in RunTimeConfigurationGenerator #26960

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Jul 27, 2022

Otherwise we sometimes get a bug where Quarkus would get to the wrong index
when parsing a configuration property, and would end up mistakenly
reporting missing configuration properties (in a really puzzling error
message...).

You can witness this bug in the test introduced in #26961 in particular. Essentially, any runtime property that contains a dot and has a Map parent will trigger the bug; in my case the property was quarkus.hibernate-search-orm."namedpu".elasticsearch.version-check.enabled (version-check.enabled is a single property, not two, see io.quarkus.hibernate.search.orm.elasticsearch.runtime.HibernateSearchElasticsearchRuntimeConfigPersistenceUnit.ElasticsearchBackendRuntimeConfig#versionCheck).

I initially thought "well, maybe I shouldn't be using dots in my property names", but then I checked, and we already do that all over the place (e.g. metrics.enabled, serializer-autodetection.enabled, handler.console, config.locations, ...).

I suspect this bug may have caused some configuration being ignored, but I'm not entirely sure.

@quarkus-bot

This comment has been minimized.

@radcortez
Copy link
Member

It seems that these keys:

quarkus.log.console.json.additional-field.foo.value=42
quarkus.log.console.json.additional-field.foo.type=int
quarkus.log.console.json.additional-field.bar.value=baz
quarkus.log.console.json.additional-field.bar.type=string

Are not being picked up correctly now. They are missing their map key:

  - SRCFG00014: The config property quarkus.log.console.json.additional-field.value is required but it could not be found in any config source
  - SRCFG00014: The config property quarkus.log.console.json.additional-field.type is required but it could not be found in any config source
  - SRCFG00014: The config property quarkus.log.console.json.additional-field.value is required but it could not be found in any config source
  - SRCFG00014: The config property quarkus.log.console.json.additional-field.type is required but it could not be found in any config source

@yrodiere
Copy link
Member Author

Having a look...

…RunTimeConfigurationGenerator

Otherwise we sometimes get a bug where Quarkus would get to the wrong index
when parsing a configuration property, and would end up mistakenly
reporting missing configuration properties (in a really puzzling error
message...).
@yrodiere yrodiere force-pushed the property-multiple-dots branch from 888267d to ed782c9 Compare July 28, 2022 09:07
@yrodiere
Copy link
Member Author

There was a symmetric operation restoring the initial state of the NameIterator, which I hadn't noticed and thus had not updated.

It should be fixed now.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 28, 2022

Failing Jobs - Building ed782c9

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

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment 
! Skipped: extensions/avro/deployment extensions/grpc/deployment extensions/hibernate-reactive/deployment and 106 more

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

io.quarkus.resteasy.reactive.server.test.multipart.TooLargeFormAttributeMultipartFormInputTest.test - More details - Source on GitHub

java.util.concurrent.RejectedExecutionException
	at org.jboss.threads.RejectingExecutor.execute(RejectingExecutor.java:38)
	at org.jboss.threads.EnhancedQueueExecutor.rejectShutdown(EnhancedQueueExecutor.java:2076)

@yrodiere
Copy link
Member Author

Note the failure is unrelated to this PR: it also happens (randomly) on the main branch: https://github.com/quarkusio/quarkus/actions/runs/2751756082

@yrodiere yrodiere merged commit e7e423f into quarkusio:main Aug 1, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Aug 1, 2022
@yrodiere yrodiere deleted the property-multiple-dots 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
Development

Successfully merging this pull request may close these issues.

2 participants