Skip to content

Conversation

@clapppp
Copy link

@clapppp clapppp commented Dec 25, 2025

This PR modifies getXXX method to return new Collection.

Updated getValuesAsSet, getValuesAsList, and getValuesAsBag to always return a new copy.
Added unit tests to verify that returned collections are independent of the backing map.
Updated Javadocs to reflect that a new collection is returned.

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

@clapppp

Thank you for your PR.

Please see my comments.

}

@Test
void testGetValuesAsBagIsSafeCopy() {
Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't prove anything about the changes in main because it passes without changes to main.

}

@Test
void testGetValuesAsListIsSafeCopy() {
Copy link
Member

Choose a reason for hiding this comment

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

OTOH, this test does prove the changes in main matter because it fails without changes to main.

}

@Test
void testGetValuesAsSetIsSafeCopy() {
Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't prove anything about the changes in main because it passes without changes to main.

@clapppp clapppp force-pushed the COLLECTIONS-881-MultiMapUtils.getXXX branch from 2d5dff4 to 5a11203 Compare December 27, 2025 16:12
@clapppp
Copy link
Author

clapppp commented Dec 27, 2025

@garydgregory

Thank you for the feedback!

I reviewed my test cases and found the issues.
I updated the test for getValuesAsSet accordingly.

However, I couldn't find any MultiValuedMapimplementation whose get(key)returns a Bag(Only Collections/List/Set).
So I'm not sure how to write a failing test case.
If it's okay to make test case with Mockito I'll make it!

@garydgregory
Copy link
Member

Hi @clapppp
WRT bag you have 2 options:

  • add a concrete implementation in the test sources
  • mocking as you mentioned

@clapppp clapppp force-pushed the COLLECTIONS-881-MultiMapUtils.getXXX branch from 5a11203 to 75200cf Compare December 27, 2025 18:04
@Test
void testGetValuesAsSet() {
assertNull(MultiMapUtils.getValuesAsList(null, "key1"));
assertNull(MultiMapUtils.getValuesAsSet(null, "key1"));
Copy link
Author

Choose a reason for hiding this comment

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

I corrected a typo.

}

@Test
void testGetValuesAsBagIsSafeCopy() {
Copy link
Author

Choose a reason for hiding this comment

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

I added the test case using EasyMock, consistent with the project's existing dependencies.

@clapppp
Copy link
Author

clapppp commented Dec 27, 2025

Hi @garydgregory

I made the changes as you suggested.
Could you please review the latest changes?

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