application-test.properties leaks into downstream module tests even though only application.properties is included in the test-jar

[famod]

Describe the bug

Imagine module A providing a src/test/resources/application.properties to downstream modules by building a test-jar.
Now also image an application-test.properties (next to that application.properties) that is not included in the test-jar (because it is only needed for tests in A but not for any downstream modules).

In such a setup, even though application-test.properties does not end up in the test-jar, downstream modules adding a dependency to that test-jar still receive config properties from that application-test.properties.

Expected behavior

Only config that is actually in the test-jar is applied to downstream modules.

Actual behavior

application-test.properties leaks into downstream modules

How to Reproduce?

q_testjar-properties.zip

• ./mvnw clean install
• see how the foo property in dist is overridden by the foo property in the application-test.properties of upstream module core

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.13.2

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

No response

Additional information

I went back all the way to 2.7.0.Final (see #22336) and it seems it has always been like that.

[famod]

/cc @aloubyansky @radcortez

[aloubyansky]

So it's about prioritizing application.properties from the current module over application-test.properties coming from a dependency.

[famod]

So it's about prioritizing application.properties from the current module over application-test.properties coming from a dependency.

I wouldn't say so, because application-test.properties doesn't even exist physically in the resulting test-jar.
It does exist physically in the resources dir, but not in the resulting test-jar.

In other words, if (in this specific test-jar setup) application-test.properties contains a property x and the downstream module that is consuming the test-jar does not set x (in whichever of its config sources), then I do not expect property x to show up in any way in that downstream module.

[aloubyansky]

So it's about prioritizing application.properties from the current module over application-test.properties coming from a dependency.

I wouldn't say so, because application-test.properties doesn't even exist physically in the resulting test-jar. It does exist physically in the resources dir, but not in the resulting test-jar.

Ok, that makes sense.

[radcortez]

I believe the issue is that we use the path of the build directory instead of the compiled jar as one of the entries in the test classloader (most likely to support hot reload of multi-module projects).

We would need to look into the dependency configuration to apply the necessary filters for include/exclude. Might be a bit tricky.

[aloubyansky]

We actually do apply content filtering. I am going to look into it.

[radcortez]

Yes, I know. I was thinking that the tricky part is the surefire configuration. Not sure if we have that implemented.

[aloubyansky]

Surefire config isn't involved in this. It's artifact content. I'll have a look.

[radcortez]

Ah, I was thinking that we would need to read surefire configuration to exclude / include stuff, but I guess that we can just pick up the test jar artifact and match the content, right?

[aloubyansky]

The jars might not exist at this phase but we do consult jar plugin configurations already, so it looks like something that should be supported

[aloubyansky]

The classpath element representing org.acme:code-with-quarkus-core:tests:1.0.0-SNAPSHOT contains

application.properties
org/acme/core/SomeAnnotation.class

which is what's expected. So the application-test.properties is picked up through something else.

[aloubyansky]

It's coming from jdk.internal.loader.ClassLoaders$AppClassLoader which is the parent of the QuarkusClassLoader

[famod]

@aloubyansky does that mean it's beyond Quarkus' control? Is the surefire classpath setup to be blamed?

[aloubyansky]

It's a quarkus issue. Surefire does what it's supposed to do. I need to spend more time on this.

[aloubyansky]

@radcortez SR Config will need a fix. When it tries to check whether profile-specific config sources (properties) are available, it creates specific URI and checks whether they happen to be loadable. The issue is that it doesn't check whether they are going beyond the classpath scope.

Here is where it happens https://github.com/smallrye/smallrye-config/blob/main/implementation/src/main/java/io/smallrye/config/AbstractLocationConfigSourceLoader.java#L213

The reproducer has both core/target/test-classes/application.properties and core/target/test-classes/application-test.properties. So given core/target/test-classes/application.properties is discovered and we are in test mode, that line attempts to check whether core/target/test-classes/application-test.properties exists. It does but this resource is not on the classpath and should not be picked up.

Somehow, the code shouldn't create paths that are crossing boundaries of classpath elements.

If core/target/test-classes/application.properties is removed though, the test passes, since application.properties appears to act as a seed for profile-specific variations of properties.

Another interesting point, not related to the above issue is that the classpath resources in the dist module test classloader are different in case of mvn clean test and mvn clean package when executed from the root.

In case of mvn clean test, JARs aren't created for the core, so the filtering doesn't apply and application-test.properties is visible on the test classpath in the dist. With mvn clean package, however, the core JARs are created with filtering applied and application-test.properties isn't visible anymore on the test classpath in the dist module. So that we could say a Maven surefire "feature".

[aloubyansky]

Another interesting point, not related to the above issue is that the classpath resources in the dist module test classloader are different in case of mvn clean test and mvn clean package when executed from the root.

In case of mvn clean test, JARs aren't created for the core, so the filtering doesn't apply and application-test.properties is visible on the test classpath in the dist. With mvn clean package, however, the core JARs are created with filtering applied and application-test.properties isn't visible anymore on the test classpath in the dist module. So that we could say a Maven surefire "feature".

Just to make it clear, the above is not a Quarkus issue.

[radcortez]

Yes, we require the main file to be present to ensure a consistent ordering when loading the files, and each of the profile files is queried from the main location manually to keep that order.

When a resource is in the jar, that is fine, because if the main file is coming from a jar, we generate the equivalent location for a possible profile resource to load from that jar location.

It seems it is indeed a problem, if the resource is loaded from a file location, where we need to perform an additional check to make sure that that file is also part of the classloader. It should be fine to perform that check.

I'm just not sure how can we do it, because as you said, application-test.properties may be visiable in the classpath, so even if we add the check, it will pass and we end up in the same situation.

[aloubyansky]

A classpath check would be enough. If maven/surefire/whatever doesn't setup the classpath consistently there is not much we can do besides completely isolating quarkus classloaders.
If we keep the current class loading setup, the behavior would be consistent with the underlying build tool.
And if/when we decide to isolate quarkus classloading from surefire/failsafe, config loading would still be based on a classpath check.

There's not much else we can do. And given we actually pass a class loader to the config builder, that's what a user should expect.

[radcortez]

Ok, if we assume that case, I'm fine adding the check.

[radcortez]

@famod, are you okay with the resolution? Even if I fix the issue discovered in SmallRye Config, you will probably still have the original problem.

Maybe you need to add some Maven filtering to control how you want the file to be available / or not.

[aloubyansky]

Just to clarify, it will work with mvn clean package or install run from the root.
mvn clean test will be picking up the application-test.properties.

Note, quarkus properly applies resource filtering in both cases. But surefire doesn't, so it will be loaded from the classpath created by surefire.

[famod]

I'd say it's a step in the right direction to add that check in SR Config.

I'll see if I can report a surefire issue with a reproducer.

[famod]

Just to clarify, it will work with mvn clean package or install run from the root. mvn clean test will be picking up the application-test.properties.

Confirmed. I'd like to add that mvn clean test -f dist does not put application-test.properties on the test classpath but mvn clean test does.

[famod]

Reported: https://issues.apache.org/jira/browse/SUREFIRE-2259