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

The contract for StringValueResolver.resolveStringValue should be more explicit [SPR-14842] #19408

Closed
spring-projects-issues opened this issue Oct 24, 2016 · 1 comment
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Oct 24, 2016

Ryan Baxter opened SPR-14842 and commented

The contract is unclear as to what value should be returned if the resolver could not resolve the value. Should it return null? Should it return the original value?


Affects: 4.3.3

Reference URL: https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/util/StringValueResolver.java#L37

Referenced from: commits 20419d7, fe19cfd

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 25, 2016

Juergen Hoeller commented

I've tightened the javadoc a bit to indicate that

  • the original String value will never be null
  • the resolved String may be null (when resolved to a null value)
  • the resolved String may be the original String value itself (in case of no placeholders to resolve or when ignoring unresolvable placeholders)
  • an IllegalArgumentException is to be thrown when rejecting an unresolvable value

So certainly don't return null for unresolvable (since null is treated as a regular value), and only return the original String if you'd like to leniently ignore unresolvable values (which is configurable in e.g. our placeholder configurers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants