-
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
Config: injection verification during the native image build #36169
Conversation
9b28d2a
to
af8517b
Compare
af8517b
to
07bcc67
Compare
This comment has been minimized.
This comment has been minimized.
07bcc67
to
fa88efe
Compare
This comment has been minimized.
This comment has been minimized.
fa88efe
to
97e8a56
Compare
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.
Thanks a lot! It looks like a nice improvement. I added a few comments below, hopefully not all of them are silly questions :)
...ions/arc/runtime/src/main/java/io/quarkus/arc/runtime/NativeBuildConfigCheckInterceptor.java
Outdated
Show resolved
Hide resolved
...ions/arc/runtime/src/main/java/io/quarkus/arc/runtime/NativeBuildConfigCheckInterceptor.java
Outdated
Show resolved
Hide resolved
...ions/arc/runtime/src/main/java/io/quarkus/arc/runtime/NativeBuildConfigCheckInterceptor.java
Outdated
Show resolved
Hide resolved
...ions/arc/runtime/src/main/java/io/quarkus/arc/runtime/NativeBuildConfigCheckInterceptor.java
Show resolved
Hide resolved
...ions/arc/runtime/src/main/java/io/quarkus/arc/runtime/NativeBuildConfigCheckInterceptor.java
Outdated
Show resolved
Hide resolved
@@ -23,6 +24,10 @@ public Object create(SyntheticCreationalContext<Object> context) { | |||
throw new IllegalStateException("No current injection point found"); | |||
} | |||
|
|||
if ((boolean) context.getParams().get("nativeBuild")) { | |||
NativeBuildConfigCheckInterceptor.verifyCurrentImageMode(Set.of()); |
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.
IIRC the Set is empty here because we assume none of the properties being injected are "build and runtime fixed"... but can't a config mapping be completely "build and runtime fixed"? If so we'd want to skip this check, no?
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.
Yes, a ConfigRoot
defined as a config mapping is something I need to check.
Because we do register a synthetic bean for any BUILD_AND_RUN_TIME_FIXED
/RUN_TIME
config root here and my guess would be that an injected config root with a config mapping would result in an ambiguous dependency error...
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.
FYI it's not a problem because a config root implemented as a config mapping is not considered a RootDefinition
...
import java.lang.annotation.Target; | ||
|
||
/** | ||
* A config property injected during the static initialization phase of a native image build may result in unexpected errors |
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.
* A config property injected during the static initialization phase of a native image build may result in unexpected errors | |
* Allows a configuration property injection point to retrieve values during static initialization. | |
* <p> | |
* A config property injected during the static initialization phase of a native image build may result in unexpected errors |
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.
We should improve the wording here, as it's not only about injecting properties.
...t/src/test/java/io/quarkus/arc/test/config/nativebuild/NativeBuildConfigInjectionOkTest.java
Show resolved
Hide resolved
declaringClassCandidate + | ||
" may lead to unexpected results. To ensure proper results, please " + | ||
"change the type of the field to " + | ||
ParameterizedType.create(DotNames.INSTANCE, new Type[] { type }, null) + |
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 see this earlier version of the check took into account the type being injected in the error message (i.e. it would suggest Instance<MyConfig>
instead of Instance<String>
, IIUC).
Would it make sense to still do that with the new approach, or is it too complex to be worth it?
If we remove the JVM mode check, you may still get unexpected results. The issue is that the available |
So you think the check should happen in JVM mode too, and the annotation should be renamed to something like |
Well, yes if we are trying to prevent this. The issue is that it may or may not be acceptable to the user to get such configuration. We have no way of knowing. For instance REST Providers are initialized during static init (like Validators), so the available config is the one at |
But in JVM mode, static init config is apparently influenced by what developers consider "runtime" config (e.g. env variables or system properties I suppose, or perhaps config files on the filesystem). At least that seems to be what triggered all the fuss in #36125 . If that's not the case, then I don't understand #36125 anymore, because the problem experienced in native mode should have happened in JVM mode as well. In that light, the fact that Vault or Database config sources don't affect static init config in JVM mode is... confusing? I suppose there are technical reasons (e.g. the DB connection pool is not available during static init), but it's definitely not just "the same". Now, the fact that env variables or system properties influence static init in JVM mode is good, and we probably don't want to change that. I'd suggest:
WDYT @mkouba? @radcortez? |
Correct. Both JVM and Native go through a
Correct, DB connection pool is not available during
What we can probably do, is record the injected configuration in |
That's interesting... And I think there is such a feature somewhere already, about build time config being overridden later, right? @mkouba WDYT? |
Yes, we do it here: quarkus/core/runtime/src/main/java/io/quarkus/runtime/configuration/ConfigRecorder.java Lines 28 to 76 in ebe995a
For build time, we generate a MAX ordinal source to prevent users to override the values, but we have to discard it to search for values that may have been overridden in other sources by matching the recorded build time time properties against the runtime properties. |
- verify the current ImageMode when a config object is injected - fail if injected during the static initialization phase of a native image build
- deprecate ConfigInjectionStaticInitBuildItem
Hm, but this would mean that we would eventually fail at Also it's probably easy to record injected properties, but I'm not sure about config mappings... |
02b4703
to
db78fb2
Compare
Failing Jobs - Building db78fb2
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/undertow/deployment
! Skipped: devtools/cli extensions/agroal/deployment extensions/avro/deployment and 346 more 📦 extensions/undertow/deployment✖
⚙️ JVM Tests - JDK 17 #- Failing: extensions/undertow/deployment
! Skipped: devtools/cli extensions/agroal/deployment extensions/avro/deployment and 347 more 📦 extensions/undertow/deployment✖
⚙️ JVM Tests - JDK 17 Windows #- Failing: extensions/arc/deployment
! Skipped: devtools/cli extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 469 more 📦 extensions/arc/deployment✖
⚙️ JVM Tests - JDK 20 #- Failing: extensions/undertow/deployment
! Skipped: devtools/cli extensions/agroal/deployment extensions/avro/deployment and 347 more 📦 extensions/undertow/deployment✖
⚙️ Native Tests - HTTP #- Failing: integration-tests/resteasy-reactive-kotlin/standard
📦 integration-tests/resteasy-reactive-kotlin/standard✖ ⚙️ Native Tests - Messaging1 #- Failing: integration-tests/kafka-oauth-keycloak integration-tests/reactive-messaging-kafka
📦 integration-tests/kafka-oauth-keycloak✖ 📦 integration-tests/reactive-messaging-kafka✖ ⚙️ Native Tests - Messaging2 #- Failing: integration-tests/reactive-messaging-amqp integration-tests/reactive-messaging-pulsar integration-tests/reactive-messaging-rabbitmq and 1 more
📦 integration-tests/reactive-messaging-amqp✖ 📦 integration-tests/reactive-messaging-pulsar✖ 📦 integration-tests/reactive-messaging-rabbitmq✖ 📦 integration-tests/reactive-messaging-rabbitmq-dyn✖ ⚙️ Native Tests - Misc3 #- Failing: integration-tests/smallrye-config
📦 integration-tests/smallrye-config✖ |
By the way, this results in a relatively large generated In theory, we could record the values for all |
We shouldn't need this. Only the properties that were injected in the static init classes. If we do need them, they are already recorded in a
Config mappings are not registered by default for |
Yes, that's actually good news. So injecting a config mapping that is not annotated with In other words, if a CDI bean that is created during Does it make sense? @radcortez @yrodiere |
It does to me. The one downside compared to your current solution is that application developers might end up in a situation where their config gets injected without error, and they end up using it during static init, triggering other, less explicit errors down the line because the property has no value during static init. But I suppose it has more to do with other libraries (e.g. Hibernate Validator) at this point, and at least in such a scenario the developer will get a stacktrace showing them where the problem happens,
By "programmatic API", do you mean the use of |
Yes, on the other hand we would not fail if the injected value did not change which is good ;-)
I mean the "retrieval of config properties through Smallrye's |
Nice! Well I don't even know if Smallrye's |
Correct.
|
This PR was superseded by #36281. |
See also discussion in #36125
The error message looks like: