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

Return empty LinkedHashMap instead of Collections.emptyMap() in PathVariableMapMethodArgumentResolver #28127

Closed
wants to merge 1 commit into from

Conversation

dugenkui03
Copy link
Contributor

I think it would be better that return empty LinkedHashMap instead of Collections.emptyMap() in PathVariableMapMethodArgumentResolver, because they do't have same support for put operation. And I think that the supported operatioin of Map annotated with @PathVariable should not be changed according the value(empty or not).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 2, 2022
@bclozel
Copy link
Member

bclozel commented Mar 2, 2022

Those are not meant to mutable; in several places we're wrapping the underlying collections with Collections.unmodifiableMap. So using Collections.emptyMap() is consistent with that approach.

I'm declining this PR as a result. Thanks!

@bclozel bclozel closed this Mar 2, 2022
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 2, 2022
@dugenkui03
Copy link
Contributor Author

@bclozel Thanks for your response.

In mvc.method.annotation.PathVariableMapMethodArgumentResolver, the original Map maybe immutable, but the returned value is mutable .
image
Do you think is it nessary that return unmodifiable view of copy even the Map is not empty, for keep same support for put operation.

@bclozel
Copy link
Member

bclozel commented Mar 3, 2022

For consistency we could indeed change that to make it immutable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants