Spring Provide a mechanism to configure integration tests with a dynamic property source

In Spring Boot, we'd like to make it easier to set configuration properties from something else that's bootstrapped as part of running an integration test. The main use case that we have for this is setting configuration properties based on the IP address and port on which a Testcontainers container is listening.

The current solution requires using an ApplicationContextInitializer to manipulate the Environment:

@Testcontainers
@ContextConfiguration(initializers = DataRedisTestIntegrationTests.Initializer.class)
@DataRedisTest
public class DataRedisTestIntegrationTests {

    @Container
    public static RedisContainer redis = new RedisContainer();

    // …

    static class Initializer
            implements ApplicationContextInitializer<ConfigurableApplicationContext> {

        @Override
        public void initialize(
                ConfigurableApplicationContext configurableApplicationContext) {
            TestPropertyValues.of(
                "spring.redis.host=" + redis.getContainerIpAddress(),
                "spring.redis.port=" + redis.getMappedPort()
            ).applyTo(configurableApplicationContext.getEnvironment());
        }

    }

}

We'd like to make this more of a first-class concept. One proposal is to achieve that via a method on the test class that can provide a property source:

@Testcontainers
@DataRedisTest
class DataRedisTestIntegrationTests {

   @Container
   static RedisContainer redis = new RedisContainer();

    // …

    @DynamicPropertySource
    static PropertySource redisProperties() {
        return TestPropertyValues.of(
            "spring.redis.host=" + redis.getContainerIpAddress(),
            "spring.redis.port=" + redis.getMappedPort()
        ).asPropertySource();
    }

}

There are some interesting ordering considerations here. It requires the RedisContainer to have been started and assigned to redis before the redisProperties() method is called. To some extent at least, this is already a solved problem as the more cumbersome ApplicationContextInitializer approach shown above works today.

Comment From: philwebb

I'm not sure if it will help with the ordering issue, but perhaps we could use method references in the actual property source itself. That way they wouldn't actually get evaluated until they are read. I also wonder if we can reuse the existing @TestPropertySource annotation?

@DynamicPropertySource
static TestPropertyValues redisProperties() {
    return TestPropertyValues.under("spring.redis.").of((values) -> {
        values.add("host", redis::getContainerIpAddress)
        values.add("port", redis::getMappedPort)
    });
}

Comment From: philwebb

Another thought. We could use an callback interface that's injected into a @TestPropertySource annotated method. That would allow us to extend the functionality of the existing annotation in a relatively logical way:

@TestPropertySource
static void redisProperties(DynamicProperties properties) {
        properties.add("spring.redis.host", redis::getContainerIpAddress)
        properties.add("spring.redis.port", redis::getMappedPort)
}

Comment From: wilkinsona

I thought about reusing @TestPropertySource but decided I didn't like it. None of its attributes made sense to me when I thought about what they'd mean when it was used on a method. inheritLocations and inheritProperties in particular seemed problematic if the annotation was to be permitted on methods.

Comment From: philwebb

Oh yeah. The ordering would also be really complicated to understand. The combination of annotated methods and class level values will be hard to reason about. I still quite like the callback idea, so perhaps:

@DynamicPropertySource
static void redisProperties(DynamicPropertyValues values) {
        values.add("spring.redis.host", redis::getContainerIpAddress)
        values.add("spring.redis.port", redis::getMappedPort)
}

Comment From: wilkinsona

I still quite like the callback idea

Me too.

@sbrannen Any news on whether this might be able to squeeze into a 5.2.x release?

Comment From: philwebb

I can try to work on a PR of it helps?

On Wed, Mar 4, 2020, 8:47 AM Andy Wilkinson notifications@github.com wrote:

I still quite like the callback idea

Me too.

@sbrannen https://github.com/sbrannen Any news on whether this might be able to squeeze into a 5.2.x release?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/spring-projects/spring-framework/issues/24540?email_source=notifications&email_token=AAD64XBRDMW72OIBBUA667DRF2A2PA5CNFSM4KWRYJ62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENY3JLY#issuecomment-594654383, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD64XGZF5QPODQPCG4L35DRF2A2PANCNFSM4KWRYJ6Q .

Comment From: bsideup

I think the callback approach will work very well for the Testcontainers use case 👍

Also, keep in mind that sometimes the containers will be manually started, and the initializer was a good place to call .start() on them because SF calls it when the context starts / being initialized. The expectation would be that @DynamicPropertySource annotated methods are called during the context initialization as well.

Comment From: sbrannen

I still quite like the callback idea

Seems plausible.

Thanks for all of the brainstorming! 👍

@sbrannen Any news on whether this might be able to squeeze into a 5.2.x release?

I'll see if I can put together a spike in the coming days.

As for whether it will land in 5.2.x or 5.3, I'll discuss that with @jhoeller once I have something working.

Comment From: philwebb

I've pushed some code here for review.

Comment From: sbrannen

@philwebb, I think your spike looks very promising!

Do you want to submit a PR that can be reviewed?

Or shall I pull down your branch, polish on top of it as necessary, and go from there?

Comment From: sbrannen

Update: I've reviewed and slightly revised the work from @philwebb (Thanks!). The diff can be viewed here.

@wilkinsona and @bsideup, if you could give a 👍 or 👎 on the current approach/API/implementation, that would be greatly appreciated.

Comment From: bsideup

@sbrannen looks good 👍 Just maybe one minor thought - since it is for testing, I would replace Supplier with Callable because it allows throwing from it.

Comment From: wilkinsona

👍 LGTM too.

+1 for Callable rather than Supplier.

Comment From: sbrannen

Thanks for the feedback, @bsideup and @wilkinsona.

Just maybe one minor thought - since it is for testing, I would replace Supplier with Callable because it allows throwing from it.

Indeed. Good catch! I'll address that.

Comment From: sbrannen

Original proposal

public @interface DynamicPropertySource {
}

public interface DynamicPropertyValues {

    void add(String name, Supplier<Object> value);
}

Usage:

    @Container
    static GenericContainer<?> redis = new GenericContainer<>(
            "redis:5.0.3-alpine").withExposedPorts(6379);

    @DynamicPropertySource
    static void redisProperties(DynamicPropertyValues values) {
        values.add("test.redis.ip", redis::getContainerIpAddress);
        values.add("test.redis.port", redis::getFirstMappedPort);
    }

We later switched from Supplier<Object> to Callable<Object> to allow the supplier to throw Exception, but the usage looks identical.

New proposal based on the notion of a registry

@FunctionalInterface
public interface DynamicPropertyRegistar {

    void register(String name, DynamicPropertyResolver dynamicPropertyResolver);

    @FunctionalInterface
    interface DynamicPropertyResolver {

        Object resolve() throws Exception;
    }
}

Usage:

    static DemoContainer container = new DemoContainer();

    @DynamicPropertySource
    static void containerProperties(DynamicPropertyRegistar registrar) {
        registrar.register("test.container.ip", container::getIpAddress);
        registrar.register("test.container.port", container::getPort);
    }

Issues

Should it be called DynamicPropertyResolverRegistar instead of DynamicPropertyRegistar?
Should it be a "registrar" or "registry"?
Spring already has org.springframework.core.env.PropertyResolver which serves an entirely different purpose than the proposed DynamicPropertyResolver functional interface above. In other words, you cannot say a DynamicPropertyResolver is-a PropertyResolver.
So, should this new functional interface rather be something like a DynamicPropertySupplier?

Feedback welcome!

Comment From: sbrannen

Regarding the switch from Supplier<Object> to Callable<Object>, that resulted in the following reflective hack to make it work.

    @Override
    public Object getProperty(String name) {
        DynamicPropertyResolver dynamicPropertyResolver = this.source.get(name);
        if (dynamicPropertyResolver != null) {
            try {
                return dynamicPropertyResolver.resolve();
            }
            catch (Exception ex) {
                maskAsUncheckedException(ex);
            }
        }
        return null;
    }

    private static void maskAsUncheckedException(Exception ex) {
        DynamicValuesPropertySource.throwAsUncheckedException(ex);
    }

    @SuppressWarnings("unchecked")
    private static <T extends Throwable> void throwAsUncheckedException(Throwable throwable) throws T {
        throw (T) throwable;
    }

In light of that, @jhoeller and I are strongly considering switching back to Supplier<Object> for simplicity. Please note that functional bean registration is also implemented via Supplier<Object>.

@wilkinsona and @bsideup, do you really foresee the need to throw checked exceptions from one of these property value suppliers?

Comment From: jhoeller

Since we'd have to wrap checked exceptions in some kind of runtime exceptions ourselves in order to get rid of that hack, without an obvious wrapper candidate to use, and since the existing PropertySource interface does not expose checked exceptions, I'd rather ask for wrapping checked exceptions in your Supplier implementations wherever necessary...

Comment From: philwebb

It looks like test containers doesn't generally use checked exceptions for Container implementations, so I'd be +1 on moving back to a Supplier.

Comment From: bsideup

@sbrannen when I was suggesting Callable I thought it would be trivial to change, but if it will require some more work (including introducing a new concept of DynamicPropertyResolver and the hack), it is okay by me to have it as Supplier 👍

We (Testcontainers) indeed don't have many methods that throw checked exceptions, but I was thinking about some other, non-Testcontainers use cases.

Looking forward to having it merged, looks great!

Comment From: wilkinsona

Me too. I have no problem with Supplier, particularly now I understand what the alternative looks like.

Comment From: wilkinsona

FWIW, I like DynamicPropertyRegistry and DynamicPropertySupplier.

Comment From: philwebb

Should it be a "registrar" or "registry"?

This is an interesting question, and it's making me wonder if the registry change is worthwhile or not. It really feels like the method itself is the registrar and it should be passed a registry. The problem is, that doesn't really feel very natural:

static DemoContainer container = new DemoContainer();

@DynamicPropertyRegistrar
static void containerProperties(DynamicPropertyRegistry registry) {
    registry.register("test.container.ip", container::getIpAddress);
    registry.register("test.container.port", container::getPort);
}

I guess my current preference would be

@DynamicPropertySource
static void redisProperties(DynamicPropertyRegistry registry) {
        registry.addDynamicProperty("spring.redis.host", redis::getContainerIpAddress)
        registry.addDynamicProperty("spring.redis.port", redis::getMappedPort)
}

Comment From: sbrannen

Thanks everybody for the feedback!

Status quo in the feature branch:

@DynamicPropertySource
static void containerProperties(DynamicPropertyRegistry registry) {
    registry.register("test.container.ip", container::getIpAddress);
    registry.register("test.container.port", container::getPort);
}

registry.register(...) aligns with existing things like BeanDefinitionRegistry.registerBeanDefinition(...). So that was my inspiration.

So I guess the debate is between the following:

registry.register(...)
registry.registerDynamicProperty(...)
registry.addDynamicProperty(...)

Comment From: philwebb

FormatterRegistry has add.. methods.

Comment From: wilkinsona

I prefer something short, and given that only one type is being registered or added, I think add or register would be ok. I think there's sufficient context from the DynamicPropertyRegistry that's passed in to avoid the need for the method name to reference dynamic properties as well. If someone wants more context they could name the variable dynamicPropertyRegistry rather than just registry.

If we think there may be multiples kinds of registration in the future (as FormatterRegistry has with printers and formatters) then a longer method name would provide some future proofing.

Comment From: sbrannen

Team Decision: go with the following simplified API.

public interface DynamicPropertyRegistry {

    /**
     * Add a {@link Supplier} for the given property name to this registry.
     * @param name the name of the property for which the supplier should be added
     * @param valueSupplier a supplier that will provide the property value on demand
     */
    void add(String name, Supplier<Object> valueSupplier);

}