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

CompositePropertySource doesn't fulfil EnumerablePropertySource [SPR-12788] #17385

Closed
spring-issuemaster opened this issue Mar 5, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Mar 5, 2015

Konrad Garus opened SPR-12788 and commented

In #16897 CompositePropertySource became EnumerablePropertySource. It doesn't fully satisfy its contract though and it violates Liskov substitution principle.

For example, Spring Boot assumes that EnumerablePropertySource.getPropertyNames contains names of all properties available on the property source. That is not the case with CompositePropertySource containing non-enumerable property sources. See also spring-projects/spring-boot#2608


Affects: 4.1.5

Issue Links:

  • #16897 CompositePropertySource should extend EnumerablePropertySource

Referenced from: commits 7e8ffc7

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2015

Juergen Hoeller commented

Good point, but I wonder what we can do about this... A CompositePropertySource won't be able to retrieve property names from a contained non-enumerable source. We cannot un-implement EnumerablePropertySource, so the only option seems to be to refine the latter's javadoc and clarify that the exposed property names are not necessarily complete. (And that separately checking for CompositePropertySource may make sense.)

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2015

Konrad Garus commented

If you change the contract on EnumerablePropertySource, what would this class represent? A PropertySource that provides a way to see some of its property names?

If that was the official specification, it would pretty much mean: "Use at your own risk, but in the end you'll have to call getProperty to really see if it contains given property".

EnumerablePropertySource has very clean, but strong specification. I'd say if some property source cannot satisfy it, it shouldn't pretend it does. Maybe it should throw something like IllegalStateException if you call getPropertyNames and it finds itself containing a non-enumerable PropertySource?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2015

Juergen Hoeller commented

I like that idea: an IllegalStateException when it encounters a non-enumerable property source underneath sounds like a fine compromise.

We can easily make this change for 4.2. However, for 4.1.x, it's probably better to preserve the existing behavior, avoiding any disruption at this late maintenance stage of the branch.

Juergen

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