search

spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.24k stars 37.98k forks source link

Preserve ordering of inlined properties in @TestPropertySource [SPR-12710] #17307

Closed spring-projects-issues closed 9 years ago

spring-projects-issues commented 9 years ago

Dave Syer opened SPR-12710 and commented

Status Quo

@TestPropertySource currently does not preserve the order of its inlined properties. Since it uses java.util.Properties internally to parse the property values, the order is lost.

Spring's Environment PropertySource is not restricted in the same way (for instance a Spring Boot app using YAML has ordered properties), so there is no way for a test to mimic the behavior of the configuration for the production application.

Proposal

Using an ordered Map as the source of the PropertySource would work.

Further Resources

Affects: 4.1 GA

Issue Links:

Referenced from: commits https://github.com/spring-projects/spring-framework/commit/e5d41d91d53873c55d2ba6c196916f7dde7ae84d, https://github.com/spring-projects/spring-framework/commit/d6a799ad4af81da8840fbd5536efd49b9225ef8a, https://github.com/spring-projects/spring-framework/commit/67934a22e2f77449dc1fd8e614028e3d42e20e81, https://github.com/spring-projects/spring-framework/commit/f82c6635d7d677eacf07c3b5e809b64c5e23a715

spring-projects-issues commented 9 years ago

Sam Brannen commented

While it is true that Spring's PropertySource abstraction is not necessarily restricted with regard to the ordering of property names, the EnumerablePropertySource API makes no guarantees about the ordering of names returned from its getPropertyNames() method. Thus, concrete implementations of EnumerablePropertySource may optionally choose to guarantee ordering but are in no way required to.

Note that Spring Framework's PropertiesPropertySource and ResourcePropertySource both rely on the JDK's standard support for loading Properties objects from input streams (which relies on the semantics of Java's Hashtable implementation which does not preserve ordering of property names), and Spring's implementations for supporting @PropertySource and @TestPropertySource rely on the same standard support in the JDK. In that regard, the behavior supported for @TestPropertySource in tests is a one-to-one copy of the behavior provided to production code via @PropertySource. Thus, for properties files and inlined properties, Spring's testing support does provide a means for mimicking the behavior of the configuration for the production application.

If Spring Boot's support for YAML-based PropertySources preserves the ordering of property names, that is something specific to Spring Boot. Furthermore, the YAML support provided by YamlPropertiesFactoryBean in the Spring Framework does not preserve ordering of property names. Thus, there is a disconnect at this level between the support for YAML in Core Spring and Spring Boot.

Now, having said that, although it would be possible to refactor the implementation of AbstractContextLoader.extractEnvironmentProperties() so that it no longer uses java.util.Properties for parsing inlined properties declared via @TestPropertySource, I question the need for this.

The source of application configuration should be transparent to application code that consumes the configuration. Thus, if ordering of property names can never be guaranteed in Spring configuration, why would one wish to write production code that relies on the ordering? For example, if a development team switches from YAML-based configuration to Properties-based configuration, any production code relying on the ordering would then fail.

Long story, short: based on the above analysis and reasoning, I am tempted to resolve this issue as Won't Fix, but I am open to any convincing use cases that demonstrate why an application would need to rely on the ordering of property names in a production deployment.

Do you have any such concrete use cases for production deployments?

Thanks,

Sam

spring-projects-issues commented 9 years ago

Dave Syer commented

Yes actually I do. Spring Cloud Configserver needs ordered maps, so does Spring Yarn and also Grails. Obviously those use cases only work with YAML but I don't see that as a reason not to fix this (especially as you probably could have written the parser and tested it in the time it took to write your comment :-)).

spring-projects-issues commented 9 years ago

Sam Brannen commented

Thanks for the prompt feedback, Dave!

OK... I'll come up with a solution. ;)

spring-projects-issues commented 9 years ago

Sam Brannen commented

Is a fix in 4.2 timely enough? Or will you be needing a fix in the 4.1.x timeline?

spring-projects-issues commented 9 years ago

Dave Syer commented

It's not supercritical (there's always a workaround using profiles).

spring-projects-issues commented 9 years ago

Sam Brannen commented

Completed as described in GitHub commit d6a799a:

Preserve ordering of inlined props in @TestPropertySource

The initial implementation for adding inlined properties configured via @TestPropertySource to the context's environment did not preserve the order in which the properties were physically declared. This makes @TestPropertySource a poor testing facility for mimicking the production environment's configuration if the property source mechanism used in production preserves ordering of property names -- which is the case for YAML-based property sources used in Spring Boot, Spring Yarn, etc.

This commit addresses this issue by ensuring that the ordering of inlined properties declared via @TestPropertySource is preserved. Specifically, the original functionality has been refactored. extracted from AbstractContextLoader, and moved to TestPropertySourceUtils where it may later be made public for general purpose use in other frameworks.

spring-projects-issues commented 9 years ago

Sam Brannen commented

Backported to 4.1.5 in GitHub commit e5d41d9.

spring-projects-issues commented 9 years ago

Sam Brannen commented

Dave Syer, I've made the requested change in spring-test 4.2 and 4.1.5; however, please note that Spring Boot's own SpringApplicationContextLoader still contains the original logic in its extractEnvironmentProperties() method.

Therefore, ordering of inlined properties is still not preserved when declared via @IntegrationTest.

Furthermore, and perhaps more importantly for users of Spring Boot, inlined properties declared via @TestPropertySource used in conjunction with SpringApplicationContextLoader will not have their ordering preserved.

The reason is that SpringApplicationContextLoader does not delegate to AbstractContextLoader.prepareContext(ConfigurableApplicationContext, MergedContextConfiguration).

Thus, if you wish to address this issue within Spring Boot, you will either have to perform a similar refactoring (as was done to resolve this issue), or as an alternative we can make the extractEnvironmentProperties() method in TestPropertySourceUtils public so that Spring Boot and other frameworks can reuse that logic.

Please let me know if you'd like for us to make extractEnvironmentProperties() public.

Cheers,

Sam

spring-projects-issues commented 9 years ago

Dave Syer commented

Please let me know if you'd like for us to make extractEnvironmentProperties() public.

Yes please.

spring-projects-issues commented 9 years ago

Sam Brannen commented

OK. I created #17318 for opening up those utility methods.

spring-projects-issues commented 9 years ago

Sam Brannen commented

Dave Syer, Phil Webb, & Stéphane Nicoll,

Since #17307 and #17318 have now been resolved, Spring Boot's SpringApplicationContextLoader can now delegate to TestPropertySourceUtils.convertInlinedPropertiesToMap() to ensure that inlined properties configured via either @IntegrationTest or @TestPropertySource have the ordering of their property names preserved in the resulting map (which can then be used to create a MapPropertySource that can be added to the Environment).

Similarly, Spring Boot can now fully support Properties files configured via @TestPropertySource (i.e., via the value or locations attribute) by delegating to TestPropertySourceUtils.addPropertiesFilesToEnvironment().

Related Spring Boot Issues

Let me know if you have any questions.

Regards,

Sam