DestinationSet should implement java.util.Set #44
Conversation
|
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. |
Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>
kpalang
requested review from
a team,
gibson9583,
kayyagari,
kpalang and
tonygermano
and removed request for
a team
|
Can this be revisited now? @tonygermano |
There was a problem hiding this comment.
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:
DestinationSetnow implementsSet<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
metaDataIdsnull 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.
|
|
||
| @Override | ||
| public boolean retainAll(Collection<?> metaDataIdOrConnectorNames) { | ||
| return removeAllExcept((Collection<Object>)metaDataIdOrConnectorNames); |
There was a problem hiding this comment.
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.
|
|
||
| @Override | ||
| public boolean removeAll(Collection<?> metaDataIdOrConnectorNames) { | ||
| return remove((Collection<Object>)metaDataIdOrConnectorNames); |
There was a problem hiding this comment.
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.
| @Test | ||
| public void testIsEmptyWithoutSourceMapKey() throws Exception { | ||
| DestinationSet ds = getNewTestDestinationSetUsing(new Integer[0]); | ||
|
|
||
| assertTrue(ds.isEmpty()); | ||
| assertEquals(0, ds.size()); | ||
| } |
There was a problem hiding this comment.
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());
}| 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]); | ||
| } |
There was a problem hiding this comment.
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.
| DestinationSet ds = getNewTestDestinationSet(); | ||
|
|
||
| Integer[] arr = new Integer[ds.size()]; | ||
| //the input and response should be the same array |
There was a problem hiding this comment.
Comment formatting should follow convention with a space after //. Should be // the input and response should be the same array.
| //the input and response should be the same array | |
| // the input and response should be the same array |
|
|
||
| assertFalse(ds.contains(0)); | ||
| assertTrue(ds.contains(1)); | ||
| assertTrue(ds.contains("Map to 2")); |
There was a problem hiding this comment.
Actual argument type 'String' is incompatible with expected argument type 'Integer'.
| assertTrue(ds.contains("Map to 2")); | |
| assertTrue(ds.contains(2)); |
| assertTrue(ds.contains("Map to 2")); | ||
| assertTrue(ds.contains(3)); | ||
| assertFalse(ds.contains(4)); | ||
| assertFalse(ds.contains("Invalid")); |
There was a problem hiding this comment.
Actual argument type 'String' is incompatible with expected argument type 'Integer'.
| * @return A boolean indicating whether at least one destination connector was actually removed | ||
| * from processing for this message. | ||
| */ | ||
| public boolean removeAllExcept(Object metaDataIdOrConnectorName) { |
There was a problem hiding this comment.
Method DestinationSet.removeAllExcept(..) could be confused with overloaded method removeAllExcept, since dispatch depends on static types.
| * @return A boolean indicating whether at least one destination connector was actually removed | ||
| * from processing for this message. | ||
| */ | ||
| public boolean remove(Object metaDataIdOrConnectorName) { |
| * destination connector name. | ||
| * @return A boolean indicating whether at least one destination connector was actually removed | ||
| * from processing for this message. | ||
| */ |
There was a problem hiding this comment.
This method overrides Set.remove; it is advisable to add an Override annotation.
| */ | |
| */ | |
| @Override |
6 participants
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.