spring-boot icon indicating copy to clipboard operation
spring-boot copied to clipboard

Add YamlPropertySourceFactory that can be used for loading YAML files through the @TestPropertySource annotation

Open nosan opened this issue 1 year ago • 4 comments

I have prepared two potential solutions for this #33434 enhancement.

The first approach offers a simple and intuitive way to use it:

@SpringJUnitConfig
@TestPropertySource(locations = "classpath:test.yaml", factory = YamlPropertySourceFactory.class)
class SomeTest {

}

YamlPropertySourceFactory loads all properties from the specified YAML file using YamlPropertySourceLoader and returns a CompositePropertySource containing the loaded properties.

As a potential improvement, a dedicated annotation like @TestYamlPropertySource could be introduced to simplify the loading of YAML files in tests.

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@Documented
@Inherited
@TestPropertySource(factory = YamlPropertySourceFactory.class)
@Repeatable(TestYamlPropertySources.class)
public @interface TestYamlPropertySource {

	@AliasFor(attribute = "value", annotation = TestPropertySource.class)
	String[] value() default {};

	@AliasFor(attribute = "locations", annotation = TestPropertySource.class)
	String[] locations() default {};

	@AliasFor(attribute = "inheritLocations", annotation = TestPropertySource.class)
	boolean inheritLocations() default true;

	@AliasFor(attribute = "properties", annotation = TestPropertySource.class)
	String[] properties() default {};

	@AliasFor(attribute = "inheritProperties", annotation = TestPropertySource.class)
	boolean inheritProperties() default true;

}

With a dedicated annotation, the following syntax could be supported:

@SpringJUnitConfig
@TestYamlPropertySource({ "classpath:test.yaml", "classpath:test1.yaml" })
@TestYamlPropertySource(locations = "classpath:test2.yaml", properties = "key:value")
class TestYamlPropertySourceIntegrationTests {

}

An alternative approach is to customize PropertySourceDescriptor in SpringBootTestContextBootstrapper by using the PropertySourceLoaderPropertySourceFactory, which leverages the existing PropertySourceLoader. With that approach, the following syntax would be possible:



@SpringBootTest
@TestPropertySource(locations = "classpath:test.yaml")
class SomeTest {
	
}

But it has several disadvantages:

  • It works only with the @SpringBootTest annotation. For @SpringJUnitConfig, the factory needs to be explicitly used.

  • It does not support EncodedResource for .properties files because PropertySourceLoader only works with Resource objects. As a result, if someone is using @TestPropertySource with the encoding attribute, these changes could potentially impact them.

  • Not obvious.

The first approach is much better and straightforward, and people would not have any problem using it.

nosan avatar Oct 11 '24 17:10 nosan

The third option is similar to the first approach, but instead of supporting only .yaml files, it also supports multi-document .properties.


@SpringJUnitConfig
@TestPropertySource(locations = { "classpath:test.yaml", "classpath:test1.yaml", "classpath:test.properties" },
		factory = PropertySourceLoaderPropertySourceFactory.class)
class PropertySourceLoaderPropertySourceFactoryIntegrationTests {

}

nosan avatar Oct 14 '24 14:10 nosan

I like YamlPropertySourceFactory since it's quite obvious what it does. Once thing that makes me pause is that we don't/can't support spring.config.activate.on-profile for multi-document files. We also can't support spring.config.import. I'm not sure if this is really a problem, but folks using the annotation will need to understand the restrictions.

philwebb avatar Oct 18 '24 20:10 philwebb

Once thing that makes me pause is that we don't/can't support spring.config.activate.on-profile for multi-document files.

I was trying to add this support, but to be honest, I have not found a way.

I'm not sure if this is really a problem

I think most people will use a dedicated YAML file to configure their test context, and I doubt anyone will try to use the spring.config.activate.on-profile feature

but folks using the annotation will need to understand the restrictions.

I completely agree, and it would be beneficial to include this information in the Javadoc and documentation.

nosan avatar Oct 18 '24 20:10 nosan

Perhaps we should make it a hard requirement that only a single document is in the YAML and that no profile restrictions or imports are specified. We'll discuss this on a team call when can.

philwebb avatar Oct 18 '24 20:10 philwebb

I'm wondering if basic YAML support should be something that's added to Spring Framework. Perhaps with some hook point so that we can warn if users accidentally point at YAML files that won't work. What do you think @sbrannen?

philwebb avatar Oct 21 '24 19:10 philwebb

I'm wondering if basic YAML support should be something that's added to Spring Framework.

We actually considered that back when we introduced @TestPropertySource(factory = ...) in Spring Framework 6.1.

I also implemented a custom YamlPropertySourceFactory and an accompanying @YamlTestProperties annotation as a proof of concept.

You can see it in action in YamlTestPropertySourceTests, along with the YAML file used in test.

The reason we didn't make that an official feature of the Spring TestContext Framework is that we doubted that it would meet the needs of enough developers.

The rationale was that Framework's YAML support in YamlPropertiesFactoryBean is very basic and that many developers would likely expect it to be as powerful as Boot's built-in YAML support. So, since we did not want to port Boot's YAML support to Framework and since the implementation is relatively straightforward, we decided it was best not to provide YamlPropertySourceFactory directly in Framework.

Perhaps with some hook point so that we can warn if users accidentally point at YAML files that won't work.

What kind of hook point did you have in mind, and who would hook into that?

sbrannen avatar Dec 04 '24 15:12 sbrannen

What kind of hook point did you have in mind, and who would hook into that?

If you added some kind of YAML support, we could have a spring.factories hook to make sure that the YAML is sensible from a Spring Boot perspective. It seems like perhaps it's best just to keep YamlPropertySourceFactory entirely in Boot based on your previous comments.

philwebb avatar Dec 04 '24 19:12 philwebb

If it's decided to include the proposed support (or a variant thereof) in Spring Boot, instead of @TestYamlPropertySource and @TestYamlPropertySources, I would recommend the names @YamlTestPropertySource and @YamlTestPropertySources in order to align with the existing @TestPropertySource naming convention.

sbrannen avatar Jan 30 '25 17:01 sbrannen

Honestly, the more I think about this PR, the more I want to close it. This YAML support is so limited, and I’m not sure whether it’s worth including in Spring Boot at all.

If support for spring.config.activate.on-profile, spring.config.import, multi-document, etc., were possible, a dedicated annotation might be considered for addition.

As a compromise, YamlPropertySourceFactory could be added, but having a dedicated annotation seems too much.


public class YamlPropertySourceFactory implements PropertySourceFactory {

	private static final YamlPropertySourceLoader loader = new YamlPropertySourceLoader();

	@Override
	public PropertySource<?> createPropertySource(String name, EncodedResource encodedResource) throws IOException {
		Resource resource = encodedResource.getResource();
		String propertySourceName = getPropertySourceName(name, resource);
		List<PropertySource<?>> propertySources = loader.load(propertySourceName, resource);
		CompositePropertySource compositePropertySource = new CompositePropertySource(propertySourceName);
		propertySources.forEach(compositePropertySource::addFirstPropertySource);
		return compositePropertySource;
	}

	private static String getPropertySourceName(String name, Resource resource) {
		if (StringUtils.hasText(name)) {
			return name;
		}
		String description = resource.getDescription();
		if (StringUtils.hasText(description)) {
			return description;
		}
		return resource.getClass().getSimpleName() + "@" + System.identityHashCode(resource);
	}


}

And if the provided YAML is a multi-document such as:

spring:
  bar: bar
  foo: baz
---
spring:
  foo: foo

The values from the later document replace those from the earlier one.

spring.bar=bar
spring.foo=foo

This behaviour will be similar to ResourcePropertySource, which does not support multi-document properties, causing the later values to overwrite the earlier ones.

The usage of this factory will not be verbose:

@TestPropertySource(locations = { "test.yaml", "test1.yaml" }, factory = YamlPropertySourceFactory.class)
class YamlTestPropertySourceIntegrationTests { }

From my perspective, it is the best option.

nosan avatar Jan 30 '25 23:01 nosan

Thanks @nosan, we discussed this one today and we agree that closing it is the best course of action for now.

philwebb avatar May 07 '25 15:05 philwebb