-
Notifications
You must be signed in to change notification settings - Fork 41
DestinationSet should implement java.util.Set #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,19 +1,17 @@ | ||||||||
| /* | ||||||||
| * Copyright (c) Mirth Corporation. All rights reserved. | ||||||||
| * | ||||||||
| * http://www.mirthcorp.com | ||||||||
| * | ||||||||
| * The software in this package is published under the terms of the MPL license a copy of which has | ||||||||
| * been included with this distribution in the LICENSE.txt file. | ||||||||
| */ | ||||||||
| // SPDX-License-Identifier: MPL-2.0 | ||||||||
| // SPDX-FileCopyrightText: Mirth Corporation | ||||||||
| // SPDX-FileCopyrightText: 2025 Richard Ogin | ||||||||
|
|
||||||||
| package com.mirth.connect.server.userutil; | ||||||||
|
|
||||||||
| import java.util.Collection; | ||||||||
| import java.util.Collections; | ||||||||
| import java.util.HashSet; | ||||||||
| import java.util.Iterator; | ||||||||
| import java.util.Map; | ||||||||
| import java.util.Optional; | ||||||||
| import java.util.Set; | ||||||||
| import java.util.stream.Collectors; | ||||||||
|
|
||||||||
| import org.mozilla.javascript.Context; | ||||||||
|
|
||||||||
|
|
@@ -24,9 +22,9 @@ | |||||||
| * Utility class used in the preprocessor or source filter/transformer to prevent the message from | ||||||||
| * being sent to specific destinations. | ||||||||
| */ | ||||||||
| public class DestinationSet { | ||||||||
| public class DestinationSet implements Set<Integer> { | ||||||||
|
|
||||||||
| private Map<String, Integer> destinationIdMap; | ||||||||
| private Map<String, Integer> destinationIdMap = Collections.emptyMap(); | ||||||||
| private Set<Integer> metaDataIds; | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -43,6 +41,7 @@ public DestinationSet(ImmutableConnectorMessage connectorMessage) { | |||||||
| this.metaDataIds = (Set<Integer>) connectorMessage.getSourceMap().get(Constants.DESTINATION_SET_KEY); | ||||||||
| } | ||||||||
| } catch (Exception e) { | ||||||||
| metaDataIds = new HashSet<>(); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: when key is not present initialize to Consider making fields final and modifying the constructor like this. public DestinationSet(ImmutableConnectorMessage connectorMessage) {
Map<String, Integer> tempMap = Collections.emptyMap();
Set<Integer> tempSet = Collections.emptySet();
if (connectorMessage != null) {
try {
Object rawValue = connectorMessage.getSourceMap().get(Constants.DESTINATION_SET_KEY);
if (rawValue instanceof Set<?> set) {
tempMap = connectorMessage.getDestinationIdMap();
@SuppressWarnings("unchecked")
Set<Integer> castedSet = (Set<Integer>) set;
tempSet = castedSet;
}
} catch (Exception e) {
// consider logging an error, but logging is not currently set up for this class
// logger.error("Failed to extract destination metadata from connector message", e);
}
}
this.destinationIdMap = tempMap;
this.metaDataIds = tempSet;
} |
||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -56,15 +55,7 @@ public DestinationSet(ImmutableConnectorMessage connectorMessage) { | |||||||
| * from processing for this message. | ||||||||
| */ | ||||||||
| public boolean remove(Object metaDataIdOrConnectorName) { | ||||||||
| if (metaDataIds != null) { | ||||||||
| Integer metaDataId = convertToMetaDataId(metaDataIdOrConnectorName); | ||||||||
|
|
||||||||
| if (metaDataId != null) { | ||||||||
| return metaDataIds.remove(metaDataId); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| return false; | ||||||||
| return remove(Collections.singleton(metaDataIdOrConnectorName)); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: annotate with This is a Set operation suggestion: keep this method simple and don't wrap and delegate If |
||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -77,15 +68,15 @@ public boolean remove(Object metaDataIdOrConnectorName) { | |||||||
| * from processing for this message. | ||||||||
| */ | ||||||||
| public boolean remove(Collection<Object> metaDataIdOrConnectorNames) { | ||||||||
| boolean removed = false; | ||||||||
|
|
||||||||
| for (Object metaDataIdOrConnectorName : metaDataIdOrConnectorNames) { | ||||||||
| if (remove(metaDataIdOrConnectorName)) { | ||||||||
| removed = true; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| return removed; | ||||||||
| if(metaDataIdOrConnectorNames == null) { return false; } | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: follow copilot recommendation to add space in suggestion: move this implementation to Set operation This encourages the use of the Set operations issue: should throw a NPE instead of return false when Collection is null This matches the previous behavior and is specified by |
||||||||
|
|
||||||||
| return metaDataIdOrConnectorNames.stream() | ||||||||
| .map(this::convertToMetaDataId) | ||||||||
| .filter(Optional::isPresent) | ||||||||
| .map(Optional::get) | ||||||||
| .map(metaDataIds::remove) | ||||||||
| .filter(Boolean::booleanValue) | ||||||||
| .count() > 0; | ||||||||
|
Comment on lines
+78
to
+79
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -98,15 +89,7 @@ public boolean remove(Collection<Object> metaDataIdOrConnectorNames) { | |||||||
| * from processing for this message. | ||||||||
| */ | ||||||||
| public boolean removeAllExcept(Object metaDataIdOrConnectorName) { | ||||||||
| if (metaDataIds != null) { | ||||||||
| Integer metaDataId = convertToMetaDataId(metaDataIdOrConnectorName); | ||||||||
|
|
||||||||
| Set<Integer> set = (metaDataId != null) ? Collections.singleton(metaDataId) : Collections.emptySet(); | ||||||||
|
|
||||||||
| return metaDataIds.retainAll(set); | ||||||||
| } | ||||||||
|
|
||||||||
| return false; | ||||||||
| return removeAllExcept(Collections.singleton(metaDataIdOrConnectorName)); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: make this a call to Set operation |
||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -119,21 +102,15 @@ public boolean removeAllExcept(Object metaDataIdOrConnectorName) { | |||||||
| * from processing for this message. | ||||||||
| */ | ||||||||
| public boolean removeAllExcept(Collection<Object> metaDataIdOrConnectorNames) { | ||||||||
| if (metaDataIds != null) { | ||||||||
| Set<Integer> set = new HashSet<Integer>(); | ||||||||
|
|
||||||||
| for (Object metaDataIdOrConnectorName : metaDataIdOrConnectorNames) { | ||||||||
| Integer metaDataId = convertToMetaDataId(metaDataIdOrConnectorName); | ||||||||
|
|
||||||||
| if (metaDataId != null) { | ||||||||
| set.add(metaDataId); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| return metaDataIds.retainAll(set); | ||||||||
| } | ||||||||
|
|
||||||||
| return false; | ||||||||
| if(metaDataIdOrConnectorNames == null) { return false; } | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: follow copilot recommendation to add space in suggestion: move this implementation to Set operation This encourages the use of the Set operations and reduces confusion about two methods which do the same thing. I think issue: should throw a NPE instead of return false when Collection is null This matches the previous behavior and is specified by Suggestion: remove null check for metaDataIds It should not be null if following other recommendations in this review |
||||||||
|
|
||||||||
| Set<Integer> set = metaDataIdOrConnectorNames.stream() | ||||||||
| .map(this::convertToMetaDataId) | ||||||||
| .filter(Optional::isPresent) | ||||||||
| .map(Optional::get) | ||||||||
| .collect(Collectors.toSet()); | ||||||||
|
|
||||||||
| return metaDataIds.retainAll(set); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -144,25 +121,102 @@ public boolean removeAllExcept(Collection<Object> metaDataIdOrConnectorNames) { | |||||||
| * from processing for this message. | ||||||||
| */ | ||||||||
| public boolean removeAll() { | ||||||||
| if (metaDataIds != null && metaDataIds.size() > 0) { | ||||||||
| metaDataIds.clear(); | ||||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| return false; | ||||||||
| int origSize = size(); | ||||||||
| clear(); | ||||||||
| return origSize > 0; | ||||||||
| } | ||||||||
|
|
||||||||
| private Integer convertToMetaDataId(Object metaDataIdOrConnectorName) { | ||||||||
| private Optional<Integer> convertToMetaDataId(Object metaDataIdOrConnectorName) { | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: keep return value for this method as it is Since this method is private, it's not really necessary to wrap and unwrap with an Optional in the same class. The nulls are cleaner. In your streams you could do +import java.util.Objects;
- .filter(Optional::isPresent)
- .map(Optional::get)
+ .filter(Objects::nonNull)I don't think it is necessary to replace the multiple returns either, but it is a good change to remove the destinationIdMap null check if we always initialize it to be non-null. It could be helpful to add a javadoc for this method to describe its behavior. |
||||||||
| Integer result = null; | ||||||||
|
|
||||||||
| if (metaDataIdOrConnectorName != null) { | ||||||||
| if (metaDataIdOrConnectorName instanceof Number) { | ||||||||
| return ((Number) metaDataIdOrConnectorName).intValue(); | ||||||||
| result = Integer.valueOf(((Number) metaDataIdOrConnectorName).intValue()); | ||||||||
| } else if (metaDataIdOrConnectorName.getClass().getName().equals("org.mozilla.javascript.NativeNumber")) { | ||||||||
| return (Integer) Context.jsToJava(metaDataIdOrConnectorName, int.class); | ||||||||
| } else if (destinationIdMap != null) { | ||||||||
| return destinationIdMap.get(metaDataIdOrConnectorName.toString()); | ||||||||
| result = (Integer) Context.jsToJava(metaDataIdOrConnectorName, int.class); | ||||||||
| } else { | ||||||||
| result = destinationIdMap.get(metaDataIdOrConnectorName.toString()); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| return null; | ||||||||
| return Optional.ofNullable(result); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public int size() { | ||||||||
| return metaDataIds.size(); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public boolean isEmpty() { | ||||||||
| return metaDataIds.isEmpty(); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public boolean contains(Object metaDataIdOrConnectorName) { | ||||||||
| Optional<Integer> m = convertToMetaDataId(metaDataIdOrConnectorName); | ||||||||
|
|
||||||||
| return m.isPresent() && metaDataIds.contains(m.get()); | ||||||||
|
Comment on lines
+157
to
+159
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if return metaDataIds.contains(convertToMetaDataId(metaDataIdOrConnectorName)); |
||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public Iterator<Integer> iterator() { | ||||||||
| return Collections.unmodifiableSet(metaDataIds).iterator(); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: iterator should be modifiable The only method on the iterator which allows modifying the Set is |
||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public Object[] toArray() { | ||||||||
| return metaDataIds.toArray(); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public <T> T[] toArray(T[] a) { | ||||||||
| return metaDataIds.toArray(a); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public boolean add(Integer metaDataId) { | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: DestinationSets do not support adding to the Set |
||||||||
| return metaDataId != null && metaDataIds.add(metaDataId); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public boolean containsAll(Collection<?> metaDataIdOrConnectorNames) { | ||||||||
| if(metaDataIdOrConnectorNames == null) { return false; } | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: follow copilot recommendation to add space in issue: |
||||||||
|
|
||||||||
| return metaDataIdOrConnectorNames.stream() | ||||||||
| .map(this::contains) | ||||||||
| .allMatch(Boolean::booleanValue); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public boolean addAll(Collection<? extends Integer> metaDataIdOrConnectorNames) { | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: DestinationSets do not support adding to the Set |
||||||||
| boolean changed = false; | ||||||||
|
|
||||||||
| if(metaDataIdOrConnectorNames != null) { | ||||||||
| for(Object item : metaDataIdOrConnectorNames) { | ||||||||
| Optional<Integer> m = convertToMetaDataId(item); | ||||||||
|
|
||||||||
| if(m.isPresent() && metaDataIds.add(m.get())) { | ||||||||
| changed = true; | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| return changed; | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public boolean retainAll(Collection<?> metaDataIdOrConnectorNames) { | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: deprecate Also mentioned above. The Set operations should be the primary methods to reduce confusion and we can eventually remove the old methods. |
||||||||
| return removeAllExcept((Collection<Object>)metaDataIdOrConnectorNames); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public boolean removeAll(Collection<?> metaDataIdOrConnectorNames) { | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: deprecate Also mentioned above. The Set operations should be the primary methods to reduce confusion and we can eventually remove the old methods. |
||||||||
| return remove((Collection<Object>)metaDataIdOrConnectorNames); | ||||||||
| } | ||||||||
|
|
||||||||
| @Override | ||||||||
| public void clear() { | ||||||||
| metaDataIds.clear(); | ||||||||
| } | ||||||||
| } | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: make
destinationIdMapandmetaDataIdsfinal and initialize both in constructor