Named Flyway config assumes values from adjusted default Flyway config

[michalvavrik]

Describe the bug

#32925 migrated Flyway to @ConfigMapping. I have app that uses 2+ datasources. Default datasource PG has Flyway configured via default quarkus.flyway.* while Oracle is configured via quarkus.flyway.oracle.*. In past when I changed quarkus.flyway.migrate-at-start at runtime, it only affected PG, now it also affects Oracle unless I override it with quarkus.flyway.oracle.migrate-at-start=false. In past, Flyway extension simply created new default instance of FlywayDataSourceRuntimeConfig instead of loading it from Config.

Expected behavior

https://quarkus.io/guides/all-config#quarkus-flyway_quarkus.flyway.-named-data-sources-.migrate-at-start says default is false, so I expect one of:

• I'd expect it to stay false unless I override value with quarkus.flyway.oracle
• (worse solution) default should be set to quarkus.flyway.migrate-at-start in documentation so that my expectations are corrected

Actual behavior

quarkus.flyway.migrate-at-start changes default of quarkus.flyway.oracle.migrate-at-start

How to Reproduce?

Reproducer

1. git clone [email protected]:michalvavrik/quarkus-test-suite.git
2. cd quarkus-test-suite/sql-db/vertx-sql
3. git checkout -b repro-flyway-defaults
4. mvn clean test

Output of uname -a or ver

Linux Fedora

Output of java -version

openjdk version "11.0.18" 2023-01-17

GraalVM version (if different from Java)

22.3

Quarkus version or git rev

999-SNAPSHOT

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.6

Additional information

No response

[quarkus-bot]

/cc @cristhiank (flyway), @gastaldi (flyway), @geoand (flyway), @gsmet (flyway)

[michalvavrik]

/cc @radcortez

[gastaldi]

I believe that if you modify the default configuration (eg. quarkus.flyway.migrate-at-start=true) that should reflect in any other named configurations, as the actual behavior indicates. WDYT @gsmet @geoand @maxandersen ?

[gsmet]

@gastaldi I don't think so, at least that's not what we are doing in other extensions. Named config are independent from the unnamed one. They are configuring different instances.

[michalvavrik]

I believe this is (deeper) config issue, not a Flyway issue (PR is completely fine), that's why I cc Roberto before.

[geoand]

I agree with @gsmet , the previous behavior is the correct one

[radcortez]

This is not a behavior provided by Config. There is no relationship between defaults for unnamed and named configs. The issue here is that if a named config is not found, then the default is returned (but this is coded manually), here:

quarkus/extensions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/FlywayRuntimeConfig.java

Lines 19 to 28 in 6cf647c

	default FlywayDataSourceRuntimeConfig getConfigForDataSourceName(String dataSourceName) {
	if (DataSourceUtil.isDefault(dataSourceName)) {
	return defaultDataSource();
	}
	FlywayDataSourceRuntimeConfig config = namedDataSources().get(dataSourceName);
	if (config == null) {
	config = defaultDataSource();
	}
	return config;
	}

And because the app config contains oracle related properties:
https://github.com/michalvavrik/quarkus-test-suite/blob/f3b40ba7d115899f4658fda773884c3876d748cd/sql-db/vertx-sql/src/main/resources/application.properties#L55-L60

It triggers the Flyway container creation for that named datasource (which doesn't exist and fallbacks to the default).

[michalvavrik]

ah, you are right, my bad, thank you

[gastaldi]

The issue here is that if a named config is not found, then the default is returned (but this is coded manually)

So the solution is to create a new ConfigMapping explicitly?

eg.

   default FlywayDataSourceRuntimeConfig getConfigForDataSourceName(String dataSourceName) {
        if (DataSourceUtil.isDefault(dataSourceName)) {
            return defaultDataSource();
        }
        FlywayDataSourceRuntimeConfig config = namedDataSources().get(dataSourceName);
        if (config == null) {
            SmallRyeConfig smallRyeConfig = ConfigProvider.getConfig().unwrap(SmallRyeConfig.class);
            config = smallRyeConfig.getConfigMapping(FlywayDataSourceRuntimeConfig.class, "quarkus.flyway." + dataSourceName);
        }
        return config;
    }

[gsmet]

Ideally, we should have SmallRye Config return a config with the default values when doing namedDataSources().get(dataSourceName).

@radcortez does the newly introduced @WithUnnamedKey handle that for us?

@gastaldi FYI, there is some work in progress here to prototype it with Hibernate Search: #32854 . Let's wait for it to be sorted out before moving other extensions to it.

@yrodiere what's the status?

[radcortez]

@radcortez does the newly introduced @WithUnnamedKey handle that for us?

Not yet. The plan was to support it. We first added support for the unnamed key to keep everything in a single Map, but we still need to adjust (and make decisions) on how are we going to show the docs in this case.

Regardless, I'll try to add that support ASAP.

[gastaldi]

I'll revert my commit meanwhile and reapply it once the @WithUnamedKey fix is sorted out

[yrodiere]

Ideally, we should have SmallRye Config return a config with the default values when doing namedDataSources().get(dataSourceName).

Neither @WithUnamedKey nor my PR cover that. But they could be a first step.

@yrodiere what's the status?

My PR (#32854) is ready, but we need a Smallrye config dependency upgrade before we merge it.

---

As to the feature you're talking about (default instances of config interfaces), as Roberto said we first need to agree on a solution. I personally would be in favor of switching from Map to a different, or at least extending, interface, but others would prefer to keep the simplicity of Map even if that means breaking some contracts (e.g. get(...) returning a value for keys not in .keySet()). I guess Roberto will have the final word anyway :)

The discussion is there: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Config.20mapping.20take.202