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 unmodifiable collections in DefaultCollectionsTypeConverterFactory #691

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

kilink
Copy link
Member

@kilink kilink commented Nov 19, 2023

DefaultCollectionsTypeConverterFactory already returned an unmodifiable Map, so update the collection converters for List, Set, and SortedSet to also wrap the converted result with the appropriate unmodifiable collection. Additionally, if the value to convert is an empty String, don't allocate a collection and simply return the appropriate empty singleton (e.g. Collections.emptyList()).

Finally, improve type safety in a few places and avoid usage of raw types.

…tory

DefaultCollectionsTypeConverterFactory already returned an unmodifiable Map, so
update the collection converters for List, Set, and SortedSet to also wrap the
converted result with the appropriate unmodifiable collection. Additionally,
if the value to convert is an empty String, don't allocate a collection and
simply return the appropriate empty singleton (e.g. Collections.emptyList()).

Finally, improve type safety in a few places and avoid usage of raw types.

config.setProperty("fubar", "");
Property<List<String>> emptyListProperty = factory.getList("fubar", String.class);
Assert.assertEquals(Collections.emptyList(), emptyListProperty.get());
Copy link
Contributor

@rgallardo-netflix rgallardo-netflix Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all the default empty collections are value types, this assertion is really just equivalent to assertTrue(emptyList.isEmpty()), isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's equivalent.

@rgallardo-netflix rgallardo-netflix merged commit e14dc80 into 2.x Nov 29, 2023
10 checks passed
@rgallardo-netflix rgallardo-netflix deleted the immutable-collections branch May 22, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants