Register properly OpenApiConfigMapping at runtime

[essobedo]

Motivation

While working on #29886, we realized that OpenApiConfigMapping was located in the deployment module instead of runtime which prevented the mapping to work.

Modifications:

• Move OpenApiConfigMapping from deployment to runtime
• Register OpenApiConfigMapping using ConfigBuilder instead of relying on the ServiceLoader for more flexibility

Result

The mapping defined in OpenApiConfigMapping is properly taken into account.

cc @zakkak @radcortez

[radcortez]

My recommendation is to use StaticInitConfigBuilderBuildItem RunTimeConfigBuilderBuildItem, which allow you to register a custom io.quarkus.runtime.configuration.ConfigBuilder, and gives you access to the current SmallRyeConfigBuilder instance.

This is way more flexible since then you can register everything that you want and you don't need separate build steps for each type of registration. I intend to deprecate and remove the specific build steps and keep only these ones.

[essobedo]

Hi @radcortez, thank you for your feedback.

Unless I miss something obvious I don't see any services or dynamic data that are currently registered using a ConfigBuilder. AFAIU, since you can only provide the FQN of the ConfigBuilder to call so If I want to register dynamically some services, either the code of the ConfigBuilder is static so the dynamic part (service discovery) would be done at runtime which is what we want to avoid or the code of the ConfigBuilder is generated dynamically, is it what you expect? or maybe I totally missed the idea?

Or maybe within the context of this PR, I only move OpenApiConfigMapping to runtime like I did here to make the mapping work.

WDYT?

[radcortez]

The ConfigBuilder instance is executed with each build step (either static or runtime). So for instance if you want to register an interceptor only for runtime you could do:

@BuildStep
void runtimeOnly(BuildProducer<RunTimeConfigBuilderBuildItem> runTimeConfigBuilder) {
    runTimeConfigBuilder.produce(new RunTimeConfigBuilderBuildItem(RuntimeOnlyBuilder.class.getName()));
}

and

public class RuntimeOnlyBuilder implements ConfigBuilder {
    @Override
    public SmallRyeConfigBuilder configBuilder(final SmallRyeConfigBuilder builder) {
        return return builder.withInterceptors(new OpenApiConfigMapping());
    }
}

[radcortez]

This would be done directly in the openapi extension.

If you have conditions to apply for the registration, these could be checked at the build step level. If you need to check something on runtime, then you can always register an interceptor factory instead and check it with the config context.

[essobedo]

The ConfigBuilder instance is executed with each build step (either static or runtime). So for instance if you want to register an interceptor only for runtime you could do:

@BuildStep
void runtimeOnly(BuildProducer<RunTimeConfigBuilderBuildItem> runTimeConfigBuilder) {
    runTimeConfigBuilder.produce(new RunTimeConfigBuilderBuildItem(RuntimeOnlyBuilder.class.getName()));
}

and

public class RuntimeOnlyBuilder implements ConfigBuilder {
    @Override
    public SmallRyeConfigBuilder configBuilder(final SmallRyeConfigBuilder builder) {
        return return builder.withInterceptors(new OpenApiConfigMapping());
    }
}

That sounds a little bit cumbersome to create a new class for each service to register but ok, it is clear now, thx

[essobedo]

In practice, it is a nice approach, thx for sharing 🙏

[essobedo]

"First-time contributors need a maintainer to approve running workflows.", I should not be a "first-time contributor", that's weird?!

[radcortez]

That sounds a little bit cumbersome to create a new class for each service to register but ok, it is clear now, thx

It is by extension. Remember you can write like:

public class RuntimeOnlyBuilder implements ConfigBuilder {
    @Override
    public SmallRyeConfigBuilder configBuilder(final SmallRyeConfigBuilder builder) {
        return return builder
                                   .withSources(...)
                                   .withMapping(...)
                                   .withInterceptors(...);
    }
}

So you can customize everything related to the configuration for the extension.

[radcortez]

"First-time contributors need a maintainer to approve running workflows.", I should not be a "first-time contributor", that's weird?!

No idea... but it does show you as an author and contributor. Maybe some issue with GH? It is building now.

[essobedo]

We have build failures with this approach because all over the place in the code base we get the config thanks to ConfigProvider.getConfig() before building the config with ConfigBuilders (runtime and static init), so it is initialized with ConfigUtils.emptyConfigBuilder().addDefaultSources().addDiscoveredSources().build() by QuarkusConfigFactory. With the previous code since OpenApiConfigMapping was defined as a service, it could be found by the ServiceLoader.

Any ideas about how to workaround/fix this new problem?

I'm wondering if my initial fix is not the best compromise?

[geoand]

It seems like CI is not happy with the fix :)

[radcortez]

We have build failures with this approach because all over the place in the code base we get the config thanks to ConfigProvider.getConfig() before building the config with ConfigBuilders (runtime and static init), so it is initialized with ConfigUtils.emptyConfigBuilder().addDefaultSources().addDiscoveredSources().build() by QuarkusConfigFactory. With the previous code since OpenApiConfigMapping was defined as a service, it could be found by the ServiceLoader.

Any ideas about how to workaround/fix this new problem?

I'm wondering if my initial fix is not the best compromise?

I had a look, and the issue is that the interceptor is relocating properties that are build only and runtime only, meaning that if you have the interceptor in deployment (as of now), it will only work for the build time properties. If you move it to runtime, it will work for runtime properties but will stop working for the build time properties.

The relocate interceptor will need to be split into one that handles build time only and another that handles runtime only.

[essobedo]

TBH, I'm a bit lost with your comment, indeed according to my investigations, the test ConfigMappingTest fails because the open API annotation model is built from a Config instance (retrieved from ConfigProvider.getConfig()) that is itself created during the Augmentation phase by https://github.com/quarkusio/quarkus/blob/main/core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java#L184 (called by QuarkusAugmentor) which happens before the static and the runtime initializations so it cannot be aware of the relocate interceptors defined with ConfigBuilder, so far, the only way to define an interceptor during this phase is by using the SPI approach like before.

Could you please clarify a bit more what you have in mind? Any example to share?

[radcortez]

the only way to define an interceptor during this phase is by using the SPI approach like before.

Correct, because some properties are build time only, so they are needed during Quarkus augmentation. I never added a way to register Config builds for the build phase because we never needed them before, so the only way to do it is by using the ServiceLoader.

I recommend splitting the relocation rules, adding an interceptor relocator (deployment module) with the build time properties rules, and registering it with the ServiceLoader. Add another interceptor (runtime module) for the runtime properties rules and use the ConfigBuilder registration approach.

[quarkus-bot]

Failing Jobs - Building 2d4772c

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 MacOS M1	Set up runner	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 18

[essobedo]

FYI, the build fails because of "JDK 17 MacOS M1"

[geoand]

Don't worry about that check, it's very flaky

[essobedo]

@radcortez Does my last commit match with what you had in mind?

[radcortez]

In this case, I believe the configuration relocate between quarkus.smallrye-openapi.info-title and mp.openapi.extensions.smallrye.info.title will not work (because it is a build time only property - other info properties too). Can you add a test for it so we can be sure? Thanks!

[essobedo]

In this case, I believe the configuration relocate between quarkus.smallrye-openapi.info-title and mp.openapi.extensions.smallrye.info.title will not work (because it is a build time only property - other info properties too). Can you add a test for it so we can be sure? Thanks!

It is already tested by https://github.com/quarkusio/quarkus/blob/main/extensions/smallrye-openapi/deployment/src/test/java/io/quarkus/smallrye/openapi/test/jaxrs/ConfigMappingTest.java#L21 and https://github.com/quarkusio/quarkus/blob/main/extensions/smallrye-openapi/deployment/src/test/java/io/quarkus/smallrye/openapi/test/jaxrs/OpenApiWithConfigTestCase.java#L21, and both tests pass so it seems to work properly, doesn't it?

[radcortez]

Ah, yes, and the interceptor is also applied to deployment. Thank you for your patience.