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

Update MicroProfile Config to 3.1 #39233

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Mar 6, 2024

@mkouba I've added some validation code to the ConfigStaticInitCheckInterceptor. This was the only was I could find to be able to validate methods injection points for @Initialized events.

For what I could tell, events get injected on Arc initialize, which will throw the error, meaning that our validation with the Recorder comes too late.

Maybe there is a better way to do this?

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file labels Mar 6, 2024
@radcortez radcortez requested a review from mkouba March 6, 2024 16:53

This comment has been minimized.

@mkouba
Copy link
Contributor

mkouba commented Mar 7, 2024

@mkouba I've added some validation code to the ConfigStaticInitCheckInterceptor. This was the only was I could find to be able to validate methods injection points for @Initialized events.

For what I could tell, events get injected on Arc initialize, which will throw the error, meaning that our validation with the Recordes comes too late.

Maybe there is a better way to do this?

Hi @radcortez, which specific events do you have in mind? Could you be more specific pls?

Also note that this validation applies to all @ConfigProperty injection points. Currently we don't fail in static init if there's for example @ConfigProperty String foo injected and the value does not change at runtime. After your change, we would fail.

@radcortez
Copy link
Member Author

Hi @radcortez, which specific events do you have in mind? Could you be more specific pls?

Something like this:

@ApplicationScoped
public class ConfigObserver {
    public void onStartup(@Observes @Initialized(ApplicationScoped.class) final Object event,
            @ConfigProperty(name = "this.property.does.not.exist") final String nonExistentConfigurationPropertyValue) {
    }
}

Also note that this validation applies to all @ConfigProperty injection points. Currently we don't fail in static init if there's for example @ConfigProperty String foo injected and the value does not change at runtime. After your change, we would fail.

It would only fail if there is no value for a non Optional / Instance / Supplier injection point, which would fail anyway in

public void validateConfigProperties(Set<ConfigValidationMetadata> properties) {

The issue here is that a regular injection point in an @ApplicationScoped bean is delayed until we execute the recorder, but for events, it would try to inject them without us being able to validate at Arc init.

@mkouba
Copy link
Contributor

mkouba commented Mar 7, 2024

Also note that this validation applies to all @ConfigProperty injection points. Currently we don't fail in static init if there's for example @ConfigProperty String foo injected and the value does not change at runtime. After your change, we would fail.

It would only fail if there is no value for a non Optional / Instance / Supplier injection point, which would fail anyway in

If I read the code correctly it's the other way around? For example, if null value and type instance of ConfigValue the log the debug message otherwise throw DeploymentException.

public void validateConfigProperties(Set<ConfigValidationMetadata> properties) {

The issue here is that a regular injection point in an @ApplicationScoped bean is delayed until we execute the recorder, but for events, it would try to inject them without us being able to validate at Arc init.

The goal of the ConfigStaticInitCheck is to detect values injected into any injection point (such as a dependency of a @Singleton bean instantiated during STATIC_INIT or an @Observes @Initialized(ApplicationScoped.class) observer) (which is fired during STATIC_INIT, unlike StartupEvent), record the value and compare it with a value produced at runtime. In your example with @ConfigProperty(name = "this.property.does.not.exist"), if no property exists during STATIC_INIT, we record null and then at runtime if the same property is injected and != null then we fail.

@radcortez
Copy link
Member Author

Let me take a step back.

The goal here is that @ConfigProperty injection points that cannot be satisfied by an existent configuration value must fail with a DeploymentException. In the case of the Observer event, we fail with the NoSuchElementException thrown by SmallRye Config because the configuration value cannot be found.

What we did until now is to record the properties to validate in

void validateConfigValues(ConfigRecorder recorder, List<ConfigPropertyBuildItem> configProperties,
BeanContainerBuildItem beanContainer, BuildProducer<ReflectiveClassBuildItem> reflectiveClass) {
and then use the recorder
public void validateConfigProperties(Set<ConfigValidationMetadata> properties) {
to check for the values and throw the DeploymentException.

In this particular case, with the event and observer, it does not work. I know that the interceptor is not supposed to be used like that, but I couldn't find any other way to validate the injection point at runtime because the injection point for the event calls the producer on Arc.initialize, so before our recorder can execute the validation.

If I read the code correctly it's the other way around? For example, if null value and type instance of ConfigValue the log the debug message otherwise throw DeploymentException.

Yes, ConfigValue is fine, not throwing the DeploymentException because it is a configuration container type (such as Optional, Provider, etc.). The goal is to throw the exception when a mandatory injection point like String cannot be injected.

For these cases, is there a way we can validate the injection point runtime value without using the interceptor?

@mkouba
Copy link
Contributor

mkouba commented Mar 8, 2024

The goal here is that @ConfigProperty injection points that cannot be satisfied by an existent configuration value must fail with a DeploymentException. In the case of the Observer event, we fail with the NoSuchElementException thrown by SmallRye Config because the configuration value cannot be found.

Do I understand it correctly that the exception type is the actual problem? Because ConfigProducer methods throw NoSuchElementException but someone expects a DeploymentException?

In this particular case, with the event and observer, it does not work. I know that the interceptor is not supposed to be used like that, but I couldn't find any other way to validate the injection point at runtime because the injection point for the event calls the producer on Arc.initialize, so before our recorder can execute the validation.

I see.

For these cases, is there a way we can validate the injection point runtime value without using the interceptor?

Directly in the io.smallrye.config.inject.ConfigProducer?

@radcortez
Copy link
Member Author

Do I understand it correctly that the exception type is the actual problem? Because ConfigProducer methods throw NoSuchElementException but someone expects a DeploymentException?

Correct.

Directly in the io.smallrye.config.inject.ConfigProducer?

I guess it may work. We validate the values in AfterDeploymentValidation for the regular CDI Extension, so the producer is always going to work. Let me have a look.

@quarkus-bot quarkus-bot bot added the area/spring Issues relating to the Spring integration label Mar 20, 2024

This comment has been minimized.

This comment has been minimized.

@radcortez radcortez requested review from mkouba and removed request for mkouba March 21, 2024 11:40
@radcortez
Copy link
Member Author

I've ended up following a different approach.

Match the injection points required at static init and have two validation phases, one for static and another for runtime.

@gsmet
Copy link
Member

gsmet commented Mar 25, 2024

I'd like to better understand our strategy regarding MicroProfile updates. Is this update part of a broader set of updates of the whole spec?
Can we merge part of the updates or should we make sure they all land in the same release?

@radcortez
Copy link
Member Author

I'd like to better understand our strategy regarding MicroProfile updates. Is this update part of a broader set of updates of the whole spec?

No, this is an update only for MP Config, still for MP 6.x

Can we merge part of the updates or should we make sure they all land in the same release?

This can be merged separately. All other specs are already at their correct version for MP 6.x.

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Apr 17, 2024

@mkouba all good for you?

@mkouba
Copy link
Contributor

mkouba commented Apr 17, 2024

@mkouba all good for you?

I need to look again ;-)

@gsmet
Copy link
Member

gsmet commented Apr 22, 2024

@mkouba let's try to get this merged this week?

Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Looks good. I've added a few minor comments.

@radcortez
Copy link
Member Author

Comments addressed!

Copy link

quarkus-bot bot commented Apr 23, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6625d56.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

@mkouba mkouba merged commit 20ed823 into quarkusio:main Apr 24, 2024
51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 24, 2024
zZHorizonZz added a commit to zZHorizonZz/quarkus-grpc-transcode that referenced this pull request Apr 24, 2024
commit 4bfd3cc
Merge: 76744d6 cfcd5a3
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Wed Apr 24 14:31:22 2024 +0200

    Merge branch 'quarkusio:main' into grpc-transcoding

commit 76744d6
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Wed Apr 24 14:17:07 2024 +0200

    chore(transcoding): moved some generic methods to util classes

commit cfcd5a3
Merge: 4945eaa 288a54d
Author: Guillaume Smet <guillaume.smet@gmail.com>
Date:   Wed Apr 24 13:31:10 2024 +0200

    Merge pull request quarkusio#40218 from gastaldi/bump_http

    Bump Quarkus HTTP to 5.2.2.Final

commit 4945eaa
Merge: ea41b08 7d56d5d
Author: Sergey Beryozkin <sberyozkin@gmail.com>
Date:   Wed Apr 24 12:28:39 2024 +0100

    Merge pull request quarkusio#40211 from sberyozkin/bc_fips_test

    Add another BouncyCastle FIPS test

commit ea41b08
Merge: fae267b d3db508
Author: Yoann Rodière <yoann@hibernate.org>
Date:   Wed Apr 24 13:13:49 2024 +0200

    Merge pull request quarkusio#40238 from gsmet/run-if-wip

    Run CI when title starts with WIP

commit 82bde60
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Wed Apr 24 12:47:33 2024 +0200

    feat(transcoding): added http method check and code refactor

commit 44ac07c
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Wed Apr 24 12:01:25 2024 +0200

    feat(transcoding): added support for path parameter and query parameters

commit fae267b
Merge: 20ed823 618fbbd
Author: Yoann Rodière <yoann@hibernate.org>
Date:   Wed Apr 24 11:49:06 2024 +0200

    Merge pull request quarkusio#40235 from yrodiere/image-metrics-return

    When verifying image metrics, return all failures instead of just the first one

commit 3fe1e11
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Wed Apr 24 11:48:34 2024 +0200

    feat(transcoding): added support for path parameter and query parameters

commit 20ed823
Merge: fb4963a 6625d56
Author: Martin Kouba <mkouba@redhat.com>
Date:   Wed Apr 24 10:04:40 2024 +0200

    Merge pull request quarkusio#39233 from radcortez/mp-config-3.1

    Update MicroProfile Config to 3.1

commit d3db508
Author: Guillaume Smet <guillaume.smet@gmail.com>
Date:   Tue Apr 23 16:26:16 2024 +0200

    Run CI when title starts with WIP

    We introduced this check when draft PRs weren't a thing but nowadays,
    it's nice to be able to convey the fact that we don't want the PR to get
    merged but still want main CI to run.

commit 618fbbd
Author: Yoann Rodière <yoann@hibernate.org>
Date:   Wed Apr 24 08:58:41 2024 +0200

    When verifying image metrics, return all failures instead of just the first one

    Building a native image takes a lot of time, so it's quite frustrating to iterate on this and solve errors one by one. Better have all the information upfront.

commit fb4963a
Merge: eddc31b 4411438
Author: Yoann Rodière <yoann@hibernate.org>
Date:   Wed Apr 24 08:41:13 2024 +0200

    Merge pull request quarkusio#40230 from quarkusio/dependabot/maven/hibernate-orm.version-6.4.5.Final

    Bump hibernate-orm.version from 6.4.4.Final to 6.4.5.Final

commit eddc31b
Merge: 648de87 2a2bb84
Author: Clement Escoffier <clement@apache.org>
Date:   Wed Apr 24 08:32:59 2024 +0200

    Merge pull request quarkusio#40173 from cescoffier/grpc-concurrent-blocking-call

    Allow concurrent invocation of blocking gRPC services by removing global ordering

commit 648de87
Merge: 0efd81a c97b545
Author: Phillip Krüger <phillip.kruger@gmail.com>
Date:   Wed Apr 24 11:34:33 2024 +1000

    Merge pull request quarkusio#40228 from dgf/main

    Fixed order for default OpenAPI security responses

commit 4411438
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Apr 23 21:40:24 2024 +0000

    Bump hibernate-orm.version from 6.4.4.Final to 6.4.5.Final

    Bumps `hibernate-orm.version` from 6.4.4.Final to 6.4.5.Final.

    Updates `org.hibernate.orm:hibernate-core` from 6.4.4.Final to 6.4.5.Final
    - [Release notes](https://github.com/hibernate/hibernate-orm/releases)
    - [Changelog](https://github.com/hibernate/hibernate-orm/blob/6.4.5/changelog.txt)
    - [Commits](hibernate/hibernate-orm@6.4.4...6.4.5)

    Updates `org.hibernate.orm:hibernate-graalvm` from 6.4.4.Final to 6.4.5.Final
    - [Release notes](https://github.com/hibernate/hibernate-orm/releases)
    - [Changelog](https://github.com/hibernate/hibernate-orm/blob/6.4.5/changelog.txt)
    - [Commits](hibernate/hibernate-orm@6.4.4...6.4.5)

    Updates `org.hibernate.orm:hibernate-envers` from 6.4.4.Final to 6.4.5.Final
    - [Release notes](https://github.com/hibernate/hibernate-orm/releases)
    - [Changelog](https://github.com/hibernate/hibernate-orm/blob/6.4.5/changelog.txt)
    - [Commits](hibernate/hibernate-orm@6.4.4...6.4.5)

    Updates `org.hibernate.orm:hibernate-jpamodelgen` from 6.4.4.Final to 6.4.5.Final
    - [Release notes](https://github.com/hibernate/hibernate-orm/releases)
    - [Changelog](https://github.com/hibernate/hibernate-orm/blob/6.4.5/changelog.txt)
    - [Commits](hibernate/hibernate-orm@6.4.4...6.4.5)

    Updates `org.hibernate:hibernate-jpamodelgen` from 6.4.4.Final to 6.4.5.Final
    - [Release notes](https://github.com/hibernate/hibernate-orm/releases)
    - [Changelog](https://github.com/hibernate/hibernate-orm/blob/6.4.5/changelog.txt)
    - [Commits](hibernate/hibernate-orm@6.4.4...6.4.5)

    Updates `org.hibernate.orm:hibernate-community-dialects` from 6.4.4.Final to 6.4.5.Final
    - [Release notes](https://github.com/hibernate/hibernate-orm/releases)
    - [Changelog](https://github.com/hibernate/hibernate-orm/blob/6.4.5/changelog.txt)
    - [Commits](hibernate/hibernate-orm@6.4.4...6.4.5)

    ---
    updated-dependencies:
    - dependency-name: org.hibernate.orm:hibernate-core
      dependency-type: direct:production
      update-type: version-update:semver-patch
    - dependency-name: org.hibernate.orm:hibernate-graalvm
      dependency-type: direct:production
      update-type: version-update:semver-patch
    - dependency-name: org.hibernate.orm:hibernate-envers
      dependency-type: direct:production
      update-type: version-update:semver-patch
    - dependency-name: org.hibernate.orm:hibernate-jpamodelgen
      dependency-type: direct:production
      update-type: version-update:semver-patch
    - dependency-name: org.hibernate:hibernate-jpamodelgen
      dependency-type: direct:production
      update-type: version-update:semver-patch
    - dependency-name: org.hibernate.orm:hibernate-community-dialects
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit c97b545
Author: Danny Gräf <deep@dagnu.de>
Date:   Tue Apr 23 13:00:05 2024 +0100

    Fixed order for default OpenAPI security responses

commit 6625d56
Author: Roberto Cortez <radcortez@yahoo.com>
Date:   Wed Mar 6 16:53:16 2024 +0000

    Update MicroProfile Config to 3.1

commit 7d56d5d
Author: Sergey Beryozkin <sberyozkin@gmail.com>
Date:   Tue Apr 23 11:18:23 2024 +0100

    Add another BouncyCastle FIPS test

commit 288a54d
Author: George Gastaldi <gegastaldi@gmail.com>
Date:   Tue Apr 23 11:06:25 2024 -0300

    Bump Quarkus HTTP to 5.2.2.Final

commit 0efd81a
Merge: b09853f 8d8dc6e
Author: Yoann Rodière <yoann@hibernate.org>
Date:   Tue Apr 23 18:27:22 2024 +0200

    Merge pull request quarkusio#40214 from zakkak/2024-04-23-fix-webjar-locator-it

    Use new webjars-locator name in integration test

commit b09853f
Merge: af955bd 9e05bd9
Author: George Gastaldi <gegastaldi@gmail.com>
Date:   Tue Apr 23 12:48:55 2024 -0300

    Merge pull request quarkusio#40157 from Dairdevil/feature/fabric8-split-package-resolved

    Remove explicitly ignored split packages

commit 320a63f
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Tue Apr 23 14:30:51 2024 +0200

    feat(transcoding): initial version of complex paths

commit af955bd
Merge: a3d1568 469fd3a
Author: Sanne Grinovero <sanne@hibernate.org>
Date:   Tue Apr 23 13:16:39 2024 +0100

    Merge pull request quarkusio#40203 from stuartwdouglas/eager-classes

    Deprecate 'eager transformers'

commit 8d8dc6e
Author: Foivos Zakkak <fzakkak@redhat.com>
Date:   Tue Apr 23 14:33:39 2024 +0300

    Use new webjars-locator name in integration test

    Closes quarkusio#40213

commit f77df97
Merge: 2af788d a3d1568
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Tue Apr 23 12:49:53 2024 +0200

    Merge branch 'quarkusio:main' into grpc-transcoding

commit 469fd3a
Author: Stuart Douglas <stuart.w.douglas@gmail.com>
Date:   Tue Apr 23 17:29:37 2024 +1000

    Deprecate 'eager transformers'

    They have not done anything for a while and no longer make sense.

commit 2af788d
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Sun Apr 21 22:42:11 2024 +0200

    feat(transcoding): removed unnecessary code

commit 2a2bb84
Author: Clement Escoffier <clement.escoffier@gmail.com>
Date:   Sun Apr 21 16:17:25 2024 +0200

    Allow concurrent invocation of blocking gRPC services by removing global ordering

    Fix quarkusio#40155

    Previously, the code utilized `executeBlocking` with `ordered=true` to maintain event order. However, this approach enforced global order instead of per-call order. This commit corrects the behavior, ensuring per-call order preservation using the BlockingExecutionHandler lock.

commit 3e9ce41
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Sat Apr 20 12:56:40 2024 +0200

    feat(transcoding): removed unnecessary encoding

commit 817d9a0
Merge: 89fdbd5 914ae38
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Sat Apr 20 12:16:22 2024 +0200

    Merge branch 'refs/heads/main' into grpc-transcoding

commit 9e05bd9
Author: Alasdair Preston <apreston@redhat.com>
Date:   Fri Apr 19 14:35:59 2024 +0100

    Remove split package ignored list as no entries remain

commit 89fdbd5
Author: Daniel Fiala <danfiala23@gmail.com>
Date:   Fri Apr 19 09:48:15 2024 +0200

    chore(transcoding): cleanup
@gsmet
Copy link
Member

gsmet commented May 16, 2024

@radcortez what's the rationale for backporting it to 3.8? It looks a bit of a stretch.

@radcortez
Copy link
Member Author

It was to make it part of the LTS version.

@gsmet
Copy link
Member

gsmet commented May 20, 2024

I can see that but it's a new minor so that seems odd to push in a micro. Maybe ping me tomorrow so that we discuss it (and we will have to convince @rsvoboda :)).

@radcortez
Copy link
Member Author

@rsvoboda was asking for it :))

@radcortez
Copy link
Member Author

The version bump was to make the TCK compatible with CDI 4. There are no API changes.

@rsvoboda
Copy link
Member

I remember now, there is QUARKUS-4213. Your "There are no API changes between both versions; the changes are in the TCK." comment implied to me there would be just component version update.

@radcortez changes in the code are a bit concerning, but we can discuss here what the motivation for them. Especially changes in ConfigBuildStep.

@radcortez
Copy link
Member Author

The changes were required to pass a change in one of the TCK tests:
eclipse/microprofile-config#543

We validate config injection points at runtime, but in the case of @Observes @Initialized(ApplicationScoped.class), these must be performed at static init. The code changes are only for that case.

I'm okay with it if we do not backport this one.

@rsvoboda
Copy link
Member

Thanks for the details, I would be in favor of some bake time + include this in the round for 3.8.6.

@gsmet gsmet modified the milestones: 3.11.0.CR1, 3.8.6 Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/spring Issues relating to the Spring integration triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants