[rogin]

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.

Release Note

destinationSet is now a java.util.Set

The destinationSet object in the preprocessor and source filter/transformer is now a java.util.Set<Integer> of the metadata IDs of the destinations the message will be sent to. Scripts can use the standard Set methods, such as size(), isEmpty(), contains(...), removeAll([...]), retainAll([...]), removeIf(...), iterator() and forEach(...). As with the existing methods, the methods that look up or remove destinations accept a metadata ID or a destination connector name:

if (destinationSet.contains('Dest C')) {
    destinationSet.removeAll(['Dest A', 3]);
}
destinationSet.removeIf(function (id) { return id > 1; });

As before, destinations can't be added. add and addAll throw UnsupportedOperationException.

Existing scripts don't need any changes.

Upgrade considerations:

• Converting destinationSet to a string, or logging it, now gives the remaining metadata IDs, for example [1, 3], instead of a class name and hash such as com.mirth.connect.server.userutil.DestinationSet@1b2c3d.

[tonygermano]

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]

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:

1. NullPointerException bug: The constructor leaves metaDataIds null when the source map doesn't contain the required key, causing NPEs in all Set methods
2. Test reliability: Multiple tests assume HashSet iteration order, which is not guaranteed and could cause intermittent test failures
3. 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.

[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]

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

[david-labs-ca]

Impressive work.

[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.

[ssrowe]

@tonygermano it doesn't look like there are any objections to your comments/improvements above. Would you be able to make the changes and rebase? We should then be able to get this merged.

[github-actions]

Test Results

129 files  ± 0  129 suites  ±0   4m 15s ⏱️ + 1m 18s
787 tests +50  787 ✅ +50  0 💤 ±0  0 ❌ ±0 
913 runs  +86  907 ✅ +86  6 💤 ±0  0 ❌ ±0 

Results for commit 8f49f8c. ± Comparison against base commit 6248cdd.

♻️ This comment has been updated with latest results.

[tonygermano]

@rogin I rebased this onto main and made the changes from my review, so it should be ready to merge. Since main moved to the Gradle layout, I rebuilt the branch as three commits instead of rebasing the old ones. You are still the author of the main commit.

1. Add more DestinationSet tests for the current behavior. These tests pass on the old code and pin down what must not change: metadata ids still work when there is no destination id map, a null collection throws an NPE, and JavaScript calls (numbers, names, and arrays) resolve to the expected overloads.
2. Make DestinationSet implement java.util.Set. Your change, with the review comments addressed:
   • It extends AbstractSet<Integer>, so equals, hashCode, and toString follow the Set contract.
   • Both fields are final and never null. A missing destination set or destination id map falls back to an empty collection. The old branch threw an NPE in both cases.
   • remove(Object) has @Override and no longer wraps and delegates. convertToMetaDataId returns null again instead of an Optional, and has Javadoc.
   • removeAll(Collection) and retainAll(Collection) hold the implementations. remove(Collection) and removeAllExcept(Collection) delegate to them, and removeAllExcept(Object) calls retainAll.
   • I changed my mind about deprecating remove(Collection) and removeAllExcept(Collection). They stay as supported aliases. If they were ever removed, Rhino would send a JavaScript array to the Object overloads instead, and the array doesn't match any destination. destinationSet.remove([...]) would silently remove nothing, and destinationSet.removeAllExcept([...]) would silently remove every destination.
   • A null collection throws an NPE in removeAll, retainAll, and containsAll.
   • add and addAll throw UnsupportedOperationException, as @pacmano1 asked.
   • The iterator is modifiable, so iterator().remove() and removeIf work.
   • removeAll(Collection) uses a loop rather than .anyMatch(...). anyMatch stops at the first match, so it would only remove the first destination that matched.
   • Your tests are merged into the existing DestinationSetTest, without the assumptions about iteration order.
3. Generate destinationSet calls with the Set operations. The Destination Set Filter step and the reference list now generate removeAll([...]) and retainAll([...]) instead of the aliases, so new scripts use the Set operations.

A full ./gradlew clean build passes at each commit.

---

Drafted with Claude Code

[mgaffigan]

@tonygermano, can this be tested in CI?

[tonygermano]

@tonygermano, can this be tested in CI?

@mgaffigan is that a question or a request? 🙂 It probably can be, but the unit tests are pretty thorough and test calling from javascript.

[mgaffigan]

@mgaffigan is that a question or a request? 🙂 It probably can be, but the unit tests are pretty thorough and test calling from javascript.

It's a question. I'm not sure how this is mean to be used in reality and am thus unwilling to approve - but I'm assuming it is from some source transformer or from the destination selector step. Unit tests are great - unit tests plus real-world exercise is better.

For example: without spelunking, it's hard for me to know if something subclasses DestinationSet between here and message processing that would break real-world, but allow unit tests to pass.

[tonygermano]

@mgaffigan is that a question or a request? 🙂 It probably can be, but the unit tests are pretty thorough and test calling from javascript.

It's a question. I'm not sure how this is mean to be used in reality and am thus unwilling to approve - but I'm assuming it is from some source transformer or from the destination selector step. Unit tests are great - unit tests plus real-world exercise is better.

For example: without spelunking, it's hard for me to know if something subclasses DestinationSet between here and message processing that would break real-world, but allow unit tests to pass.

@mgaffigan it's a class that is part of the User API. The engine presents an instance to the user in the javascript context for modification, but never accepts one, so there isn't really a chance for a subclass to break something.

[tonygermano]

@mgaffigan these changes are meant to make it behave more like a normal Set, since the original class did not allow inspection or iteration. It retains the original property of the class that additions are not allowed. I think any potential issue that I'm not aware of with subclassing would be an existing issue before these changes.

[pacmano1]

Ran it on a real engine next to main, same channels, same messages: existing destinationSet calls return and route exactly the same in the source filter and transformer, both preprocessors and the Destination Set Filter step, including partial destination lists, router.routeMessageByChannelId and reprocess. @mgaffigan, that's the real-world run.

Release notes: logging it changes. String(destinationSet) was ...DestinationSet@2efe6d42, now [1, 2, 3, 4].

Worth a line of Javadoc, not blocking:

• iterator().next() and toArray() give java.lang.Integer objects, so it.next() === 2 is false. The forEach/removeIf callbacks get JS numbers.
• destinationSet.remove(id) inside forEach or an iterator loop throws ConcurrentModificationException; removeIf or iterator().remove() is the way.
• for each (var id in destinationSet) loops over the methods, and for (var id of destinationSet) says it isn't iterable (same on main).

Co-authored with Claude Code.

[pacmano1]

Pushed a CI fixture, ci/tests/130-destination-set: one channel, six messages, covering the filter step's generated calls, the existing calls, and size/contains and removeIf. Green in all 7 configurations; run locally against main's server, the two new-method cases fail, as they should.

On the Integer vs number thing from my review: that's Rhino 1.7.13, not this PR. A value a Java method returns as Object (Iterator.next(), Object[] elements) goes through WrapFactory.wrap, which wraps boxed numbers as Java objects because java primitive wrap is on (Rhino's default; OIE never changes it). Arguments passed into a JS callback go through InterfaceAdapter, which skips the wrap for String, Number and Boolean. So forEach/removeIf see numbers and iterator().next() doesn't.

Co-authored with Claude Code.

[tonygermano]

• destinationSet.remove(id) inside forEach or an iterator loop throws ConcurrentModificationException; removeIf or iterator().remove() is the way.

This behavior is from the iterator of metaDataIds. That's how most Sets work.

• for each (var id in destinationSet) loops over the methods, and for (var id of destinationSet) says it isn't iterable (same on main).

for each behavior on anything except for an e4x object should be considered undefined as it's not part of the javascript spec.

I believe the for (var id of destinationSet) case will work after we update the Rhino library. I'm pretty sure Java Iterables are now considered ES6 iterables.

[mgaffigan]

@NicoPiel, are your requested changes complete?

If we don't hear back by EOD Friday, I'm going to dismiss them since this is otherwise ready to merge.

[NicoPiel]

@mgaffigan My conversations seem to be resolved? I don't know what else to dismiss.

[mgaffigan]

@NicoPiel, it shows you're no longer requesting changes now. Thanks. To unblock without approval, you use the "Dismiss" under your name at the top right - otherwise requesting changes blocks until approval whether conversations are open or not.