Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spring-issuemaster opened this issue Feb 12, 2015 · 11 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Feb 12, 2015

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:

  • #17318 Open up TestPropertySourceUtils for public consumption

Referenced from: commits e5d41d9, d6a799a, 67934a2, f82c663

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2015

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-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2015

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-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2015

Sam Brannen commented

Thanks for the prompt feedback, Dave!

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2015

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-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2015

Dave Syer commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2015

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-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2015

Sam Brannen commented

Backported to 4.1.5 in GitHub commit e5d41d9.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2015

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-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2015

Dave Syer commented

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

Yes please.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2015

Sam Brannen commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.