Skip to content

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Jan 22, 2025

  1. It's safe to use immutable Collection object as default value of field.
  2. It's unnecessary to create new Collection object in setter method.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 22, 2025
1. It's safe to use immutable Collection object as default value of field.
2. It's unnecessary to create new Collection object in setter method.

Signed-off-by: Yanming Zhou <[email protected]>
@wilkinsona
Copy link
Member

Thanks for the PR but I don't think we should make this change.

When binding properties, the binder will first try to mutate an existing collection by clearing it and then adding the new contents:

This change stops making use of that capability.

Perhaps more importantly, this may be a breaking change for consumers of the collections as they may no longer be mutable. While we document that the configuration properties classes are not officially public API, we know that people do use them and this change may break their code for reasons that are hard to justify.

@wilkinsona wilkinsona closed this Jan 22, 2025
@wilkinsona wilkinsona 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 labels Jan 22, 2025
@quaff
Copy link
Contributor Author

quaff commented Jan 22, 2025

I see some List are initialized by List.of(), should them be wrapped with new ArrayList<>()?

@wilkinsona
Copy link
Member

I'm not sure that it's worth it as the breaking change reasoning above does not apply.

@quaff
Copy link
Contributor Author

quaff commented Jan 23, 2025

I'm not sure that it's worth it as the breaking change reasoning above does not apply.

I think it's worthy, I created #43933.

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