You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "clapppp (via GitHub)" <gi...@apache.org> on 2025/12/25 13:23:33 UTC
[PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
clapppp opened a new pull request, #669:
URL: https://github.com/apache/commons-collections/pull/669
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #669:
URL: https://github.com/apache/commons-collections/pull/669#discussion_r2649128105
##########
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##########
@@ -118,6 +118,50 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+ @Test
+ void testGetValuesAsBagIsSafeCopy() {
+ final String[] values = { "v1", "v2", "v3" };
+ final MultiValuedMap<String, String> map = new ArrayListValuedHashMap<>();
+ for (final String val : values) {
+ map.put("key1", val);
+ map.put("key1", val);
+ }
+
+ final Bag<String> bag = MultiMapUtils.getValuesAsBag(map, "key1");
+ bag.add("v4");
+ assertFalse(map.containsMapping("key1", "v4"));
+ assertEquals(6, MultiMapUtils.getValuesAsBag(map, "key1").size());
+ }
+
+ @Test
+ void testGetValuesAsListIsSafeCopy() {
+ final String[] values = { "v1", "v2", "v3" };
+ final MultiValuedMap<String, String> map = new ArrayListValuedHashMap<>();
+ for (final String val : values) {
+ map.put("key1", val);
+ }
+
+ final List<String> list = MultiMapUtils.getValuesAsList(map, "key1");
+ list.add("v4");
+ assertFalse(map.containsMapping("key1", "v4"));
+ assertEquals(Arrays.asList(values), MultiMapUtils.getValuesAsList(map, "key1"));
+ }
+
+ @Test
+ void testGetValuesAsSetIsSafeCopy() {
Review Comment:
This test doesn't prove anything about the changes in `main` because it passes without changes to `main`.
##########
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##########
@@ -118,6 +118,50 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+ @Test
+ void testGetValuesAsBagIsSafeCopy() {
+ final String[] values = { "v1", "v2", "v3" };
+ final MultiValuedMap<String, String> map = new ArrayListValuedHashMap<>();
+ for (final String val : values) {
+ map.put("key1", val);
+ map.put("key1", val);
+ }
+
+ final Bag<String> bag = MultiMapUtils.getValuesAsBag(map, "key1");
+ bag.add("v4");
+ assertFalse(map.containsMapping("key1", "v4"));
+ assertEquals(6, MultiMapUtils.getValuesAsBag(map, "key1").size());
+ }
+
+ @Test
+ void testGetValuesAsListIsSafeCopy() {
Review Comment:
OTOH, this test _does_ prove the changes in `main` matter because it fails without changes to `main`.
##########
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##########
@@ -118,6 +118,50 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+ @Test
+ void testGetValuesAsBagIsSafeCopy() {
Review Comment:
This test doesn't prove anything about the changes in `main` because it passes without changes to `main`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
Posted by "clapppp (via GitHub)" <gi...@apache.org>.
clapppp commented on PR #669:
URL: https://github.com/apache/commons-collections/pull/669#issuecomment-3694138681
Hi @garydgregory
I made the changes as you suggested.
Could you please review the latest changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #669:
URL: https://github.com/apache/commons-collections/pull/669#issuecomment-3694094817
Hi @clapppp
WRT bag you have 2 options:
- add a concrete implementation in the test sources
- mocking as you mentioned
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
Posted by "clapppp (via GitHub)" <gi...@apache.org>.
clapppp commented on code in PR #669:
URL: https://github.com/apache/commons-collections/pull/669#discussion_r2649260248
##########
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##########
@@ -118,6 +123,47 @@ void testGetValuesAsSet() {
assertEquals(new HashSet<>(Arrays.asList(values)), set);
}
+ @Test
+ void testGetValuesAsBagIsSafeCopy() {
Review Comment:
I added the test case using EasyMock, consistent with the project's existing dependencies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #669:
URL: https://github.com/apache/commons-collections/pull/669
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
Posted by "clapppp (via GitHub)" <gi...@apache.org>.
clapppp commented on PR #669:
URL: https://github.com/apache/commons-collections/pull/669#issuecomment-3694087301
@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 `MultiValuedMap`implementation 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy [commons-collections]
Posted by "clapppp (via GitHub)" <gi...@apache.org>.
clapppp commented on code in PR #669:
URL: https://github.com/apache/commons-collections/pull/669#discussion_r2649258987
##########
src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java:
##########
@@ -105,7 +110,7 @@ void testGetValuesAsList() {
@Test
void testGetValuesAsSet() {
- assertNull(MultiMapUtils.getValuesAsList(null, "key1"));
+ assertNull(MultiMapUtils.getValuesAsSet(null, "key1"));
Review Comment:
I corrected a typo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org