DestinationSet should implement java.util.Set #44

rogin · 2025-04-19T01:04:01Z

This was an old feature, requested by several people, that NextGen chose to ignore. Prior code review comments are here. I compiled it with Java 8, and the test ran successfully. No further testing on my part.

tonygermano · 2025-04-19T01:08:05Z

Thanks for submitting this! I always meant to give it a more thorough review and suggest a few things. Just a heads up, this probably will not be a top priority until after we get our first release out the door.

NicoPiel · 2025-09-03T11:05:35Z

Can this be revisited now? @tonygermano

Copilot

Pull request overview

This PR implements the java.util.Set<Integer> interface for the DestinationSet class, addressing a long-standing feature request. The implementation adds standard Set methods (add, contains, iterator, etc.) and modernizes the code with Java 8 streams and Optional.

Key Changes:

DestinationSet now implements Set<Integer>, enabling use as a standard Java collection
Refactored existing methods to use streams and Optional for cleaner code
Added comprehensive test suite with 20+ test cases covering Set interface methods

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 21 comments.

File	Description
server/src/com/mirth/connect/server/userutil/DestinationSet.java	Implements Set interface with all required methods, refactors internal logic to use streams and Optional
server/test/com/mirth/connect/userutil/DestinationSetTest.java	Adds comprehensive test suite for Set interface implementation and existing functionality

Critical Issues Found:

NullPointerException bug: The constructor leaves metaDataIds null when the source map doesn't contain the required key, causing NPEs in all Set methods
Test reliability: Multiple tests assume HashSet iteration order, which is not guaranteed and could cause intermittent test failures
Missing test coverage: No test for the scenario where the source map lacks the DESTINATION_SET_KEY

Code Quality Issues:

Inconsistent spacing in control flow statements (if, for)
Unchecked casts that will generate compiler warnings
Comment formatting inconsistencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-10T15:17:10Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+
+     @Override
+     public boolean retainAll(Collection<?> metaDataIdOrConnectorNames) {
+         return removeAllExcept((Collection<Object>)metaDataIdOrConnectorNames);


Unchecked cast from Collection<?> to Collection<Object>. While this works, it will generate a compiler warning. Consider adding @SuppressWarnings("unchecked") to this method or refactoring to avoid the cast.

Copilot · 2025-12-10T15:17:11Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+
+     @Override
+     public boolean removeAll(Collection<?> metaDataIdOrConnectorNames) {
+         return remove((Collection<Object>)metaDataIdOrConnectorNames);


Unchecked cast from Collection<?> to Collection<Object>. While this works, it will generate a compiler warning. Consider adding @SuppressWarnings("unchecked") to this method or refactoring to avoid the cast.

Copilot · 2025-12-10T15:17:11Z

server/test/com/mirth/connect/userutil/DestinationSetTest.java

+    @Test
+    public void testIsEmptyWithoutSourceMapKey() throws Exception {
+        DestinationSet ds = getNewTestDestinationSetUsing(new Integer[0]);
+
+        assertTrue(ds.isEmpty());
+        assertEquals(0, ds.size());
+    }


The test name testIsEmptyWithoutSourceMapKey is misleading. The test actually creates a source map WITH the key (line 47 in getNewTestDestinationSetUsing), just with an empty set.

There's no test coverage for the scenario where the source map does NOT contain Constants.DESTINATION_SET_KEY at all (i.e., when line 45 in DestinationSet.java returns false). This is a critical gap because that scenario will cause a NullPointerException with the current implementation.

Add a test like:

@Test public void testWithoutSourceMapKey() throws Exception { ConnectorMessage cm = new ConnectorMessage(); Map<String, Object> sm = new HashMap<>(); // Don't put DESTINATION_SET_KEY in the map cm.setSourceMap(sm); ImmutableConnectorMessage icm = new ImmutableConnectorMessage(cm); DestinationSet ds = new DestinationSet(icm); assertTrue(ds.isEmpty()); assertEquals(0, ds.size()); }

Copilot · 2025-12-10T15:17:11Z

server/test/com/mirth/connect/userutil/DestinationSetTest.java

+    public void testToArrayGenericProperlySized() throws Exception {
+        DestinationSet ds = getNewTestDestinationSet();
+
+        Integer[] arr = new Integer[ds.size()];
+        //the input and response should be the same array
+        Integer[] resp = ds.toArray(arr);
+        assertEquals(3, resp.length);
+        assertTrue(Arrays.equals(arr, resp));
+        assertEquals(Integer.valueOf(1), arr[0]);
+        assertEquals(Integer.valueOf(2), arr[1]);
+        assertEquals(Integer.valueOf(3), arr[2]);
+    }


This test assumes a specific array order (1, 2, 3), but DestinationSet uses a HashSet internally, which does not guarantee any specific order. The test is flawed and could fail intermittently.

The test should verify that all expected elements are present without assuming order.

Copilot · 2025-12-10T15:17:12Z

server/test/com/mirth/connect/userutil/DestinationSetTest.java

+        DestinationSet ds = getNewTestDestinationSet();
+
+        Integer[] arr = new Integer[ds.size()];
+        //the input and response should be the same array


Comment formatting should follow convention with a space after //. Should be // the input and response should be the same array.

Suggested change

//the input and response should be the same array

// the input and response should be the same array

Copilot · 2025-12-10T15:17:16Z

server/test/com/mirth/connect/userutil/DestinationSetTest.java

+
+        assertFalse(ds.contains(0));
+        assertTrue(ds.contains(1));
+        assertTrue(ds.contains("Map to 2"));


Actual argument type 'String' is incompatible with expected argument type 'Integer'.

Suggested change

assertTrue(ds.contains("Map to 2"));

assertTrue(ds.contains(2));

Copilot · 2025-12-10T15:17:17Z

server/test/com/mirth/connect/userutil/DestinationSetTest.java

+        assertTrue(ds.contains("Map to 2"));
+        assertTrue(ds.contains(3));
+        assertFalse(ds.contains(4));
+        assertFalse(ds.contains("Invalid"));


Actual argument type 'String' is incompatible with expected argument type 'Integer'.

Copilot · 2025-12-10T15:17:17Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+      * @return A boolean indicating whether at least one destination connector was actually removed
+      *         from processing for this message.
+      */
+     public boolean removeAllExcept(Object metaDataIdOrConnectorName) {


Method DestinationSet.removeAllExcept(..) could be confused with overloaded method removeAllExcept, since dispatch depends on static types.

Copilot · 2025-12-10T15:17:17Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+      * @return A boolean indicating whether at least one destination connector was actually removed
+      *         from processing for this message.
+      */
+     public boolean remove(Object metaDataIdOrConnectorName) {


Method DestinationSet.remove(..) could be confused with overloaded method remove, since dispatch depends on static types.

Copilot · 2025-12-10T15:17:18Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+      *            destination connector name.
+      * @return A boolean indicating whether at least one destination connector was actually removed
+      *         from processing for this message.
+      */


This method overrides Set.remove; it is advisable to add an Override annotation.

Suggested change

*/

*/

@Override

NicoPiel · 2025-12-16T13:55:03Z

server/test/com/mirth/connect/userutil/DestinationSetTest.java

+    public void testToArray() throws Exception {
+        DestinationSet ds = getNewTestDestinationSet();
+
+        Object[] arr = ds.toArray();
+        assertEquals(3, arr.length);
+        assertEquals(Integer.valueOf(1), arr[0]);
+        assertEquals(Integer.valueOf(2), arr[1]);
+        assertEquals(Integer.valueOf(3), arr[2]);
+    }


This is true. Tests that assume toArray to be deterministic could be flaky.

NicoPiel · 2025-12-16T13:55:53Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+         try {
+             if (connectorMessage.getSourceMap().containsKey(Constants.DESTINATION_SET_KEY)) {
+                 this.destinationIdMap = connectorMessage.getDestinationIdMap();
+                 this.metaDataIds = (Set<Integer>) connectorMessage.getSourceMap().get(Constants.DESTINATION_SET_KEY);
+             }
+         } catch (Exception e) {
+             metaDataIds = new HashSet<>();


This is also a good point and probable cause for future NPEs.

pacmano1

Needs more review.

"add" should be in it own PR. We should try to avoid adding features in the context of a limited scope issue.

rogin · 2025-12-17T15:54:52Z

I'm impressed by the copilot results. I'd like to resolve the findings this weekend.

david-labs-ca · 2025-12-17T23:13:19Z

Impressive work.

Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>

tonygermano

In addition to the comments below, I pushed a change to your branch with the following:

I fixed the line endings on DestinationSet so that you were no longer rewriting the entire file.
I changed the header on DestinationSet and added one to DestinationSetTest in SPDX format and adding your name to the copyright.
I rebased to resolve the merge conflict with previous PR #133 (keeps the changes in this PR, which negate the changes from the previous PR.)

The previous PR also added tests, but to file server/test/com/mirth/connect/server/userutil/DestinationSetTest.java instead of server/test/com/mirth/connect/userutil/DestinationSetTest.java. I think the other PR was correct, and this PR should add to the other tests. I think this is also what is currently causing the build to fail.

tonygermano · 2025-12-20T08:33:09Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

-        }
-
-        return false;
+        return remove(Collections.singleton(metaDataIdOrConnectorName));


suggestion: annotate with @Override

This is a Set operation

suggestion: keep this method simple and don't wrap and delegate

If metaDataIds is made final and never null above, then the first null check can be removed.

tonygermano · 2025-12-20T08:44:44Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

-        }
-
-        return removed;
+        if(metaDataIdOrConnectorNames == null) { return false; }


nitpick: follow copilot recommendation to add space in if (

suggestion: move this implementation to Set operation removeAll and deprecate this method

This encourages the use of the Set operations remove for a single object and removeAll for collections. The overloaded remove has been a source of confusion in the past.

issue: should throw a NPE instead of return false when Collection is null

This matches the previous behavior and is specified by Set.removeAll

tonygermano · 2025-12-20T08:45:58Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+            .filter(Boolean::booleanValue)
+            .count() > 0;


Suggested change

.filter(Boolean::booleanValue)

.count() > 0;

.anyMatch(Boolean::booleanValue)

tonygermano · 2025-12-20T08:47:50Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

-        }
-
-        return false;
+        return removeAllExcept(Collections.singleton(metaDataIdOrConnectorName));


suggestion: make this a call to Set operation retainAll

tonygermano · 2025-12-20T08:54:53Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

-        }
-
-        return false;
+        if(metaDataIdOrConnectorNames == null) { return false; }


nitpick: follow copilot recommendation to add space in if (

suggestion: move this implementation to Set operation retainAll and deprecate this method

This encourages the use of the Set operations and reduces confusion about two methods which do the same thing. I think removeAllExcept(Object) can remain since Set does not provide a retainOnly method for a single Object.

issue: should throw a NPE instead of return false when Collection is null

This matches the previous behavior and is specified by Set.retainAll

Suggestion: remove null check for metaDataIds

It should not be null if following other recommendations in this review

tonygermano · 2025-12-20T16:43:15Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+    }
+
+    @Override
+    public boolean addAll(Collection<? extends Integer> metaDataIdOrConnectorNames) {


issue: addAll should throw UnsupportedOperationException

DestinationSets do not support adding to the Set

tonygermano · 2025-12-20T16:45:57Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+    }
+
+    @Override
+    public boolean retainAll(Collection<?> metaDataIdOrConnectorNames) {


suggestion: deprecate removeAllExcept(Collection<Object>) in favor of this method

Also mentioned above. The Set operations should be the primary methods to reduce confusion and we can eventually remove the old methods.

tonygermano · 2025-12-20T16:46:29Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+    }
+
+    @Override
+    public boolean removeAll(Collection<?> metaDataIdOrConnectorNames) {


suggestion: deprecate remove(Collection<Object>) in favor of this method

Also mentioned above. The Set operations should be the primary methods to reduce confusion and we can eventually remove the old methods.

tonygermano · 2025-12-20T18:59:20Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+        Optional<Integer> m = convertToMetaDataId(metaDataIdOrConnectorName);
+
+        return m.isPresent() && metaDataIds.contains(m.get());


if convertToMetaDataId is changed back to returning nulls and metaDataIds is always non-null as suggested elsewhere, I think this can be simplified to just

return metaDataIds.contains(convertToMetaDataId(metaDataIdOrConnectorName));

tonygermano · 2025-12-20T19:01:38Z

server/src/com/mirth/connect/server/userutil/DestinationSet.java

+
+    @Override
+    public boolean containsAll(Collection<?> metaDataIdOrConnectorNames) {
+        if(metaDataIdOrConnectorNames == null) { return false; }


nitpick: follow copilot recommendation to add space in if (

issue: Set.containsAll should throw a NPE instead of returning false when Collection is null

jonbartels previously approved these changes Apr 20, 2025

View reviewed changes

rogin force-pushed the implement-set branch from fd1c91a to 2b23e77 Compare April 21, 2025 02:34

jonbartels added this to the Technical Preview 2 milestone May 6, 2025

ppazos mentioned this pull request Aug 5, 2025

[IDEA] Extend the API of Destination set to be able to query it Innovar-Healthcare/BridgeLink#42

Open

kpalang requested review from a team, gibson9583, kayyagari, kpalang and tonygermano and removed request for a team August 17, 2025 10:22

kayyagari previously approved these changes Aug 18, 2025

View reviewed changes

pacmano1 requested a review from Copilot December 10, 2025 15:08

Copilot started reviewing on behalf of pacmano1 December 10, 2025 15:08 View session

gibson9583 previously approved these changes Dec 10, 2025

View reviewed changes

Copilot AI reviewed Dec 10, 2025

View reviewed changes

NicoPiel suggested changes Dec 16, 2025

View reviewed changes

pacmano1 requested changes Dec 17, 2025

View reviewed changes

DestinationSet should implement java.util.Set

3e60899

Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>

tonygermano dismissed stale reviews from gibson9583, kayyagari, and jonbartels via 3e60899 December 20, 2025 08:11

tonygermano force-pushed the implement-set branch from 2b23e77 to 3e60899 Compare December 20, 2025 08:11

tonygermano requested changes Dec 20, 2025

View reviewed changes

Merge branch 'OpenIntegrationEngine:main' into implement-set

3c5b47d

	//the input and response should be the same array
	// the input and response should be the same array

	assertTrue(ds.contains("Map to 2"));
	assertTrue(ds.contains(2));

	.filter(Boolean::booleanValue)
	.count() > 0;
	.anyMatch(Boolean::booleanValue)

		Optional<Integer> m = convertToMetaDataId(metaDataIdOrConnectorName);

		return m.isPresent() && metaDataIds.contains(m.get());

DestinationSet should implement java.util.Set #44

Are you sure you want to change the base?

DestinationSet should implement java.util.Set #44

Uh oh!

Conversation

rogin commented Apr 19, 2025

Uh oh!

tonygermano commented Apr 19, 2025

Uh oh!

NicoPiel commented Sep 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacmano1 left a comment

Choose a reason for hiding this comment

Uh oh!

rogin commented Dec 17, 2025

Uh oh!

david-labs-ca commented Dec 17, 2025

Uh oh!

tonygermano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonygermano Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tonygermano Dec 20, 2025 •

edited

Loading