Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 118 additions & 64 deletions server/src/com/mirth/connect/server/userutil/DestinationSet.java
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;

Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: make destinationIdMap and metaDataIds final and initialize both in constructor


/**
Expand All @@ -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<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: when key is not present initialize to Collections.emptySet()

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;
}

}
}

Expand All @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand All @@ -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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand All @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: make this a call to Set operation retainAll

}

/**
Expand All @@ -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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Set<Integer> set = metaDataIdOrConnectorNames.stream()
.map(this::convertToMetaDataId)
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toSet());

return metaDataIds.retainAll(set);
}

/**
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

}

@Override
public Iterator<Integer> iterator() {
return Collections.unmodifiableSet(metaDataIds).iterator();
Copy link
Member

Choose a reason for hiding this comment

The 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 remove which is an allowed operation.

}

@Override
public Object[] toArray() {
return metaDataIds.toArray();
}

@Override
public <T> T[] toArray(T[] a) {
return metaDataIds.toArray(a);
}

@Override
public boolean add(Integer metaDataId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: add should throw UnsupportedOperationException

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; }
Copy link
Member

@tonygermano tonygermano Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: follow copilot recommendation to add space in if (


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


return metaDataIdOrConnectorNames.stream()
.map(this::contains)
.allMatch(Boolean::booleanValue);
}

@Override
public boolean addAll(Collection<? extends Integer> metaDataIdOrConnectorNames) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: addAll should throw UnsupportedOperationException

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return removeAllExcept((Collection<Object>)metaDataIdOrConnectorNames);
}

@Override
public boolean removeAll(Collection<?> metaDataIdOrConnectorNames) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return remove((Collection<Object>)metaDataIdOrConnectorNames);
}

@Override
public void clear() {
metaDataIds.clear();
}
}
Loading
Loading