-
Notifications
You must be signed in to change notification settings - Fork 41
Add message tracking information in VMRouter #49
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?
Conversation
|
Revised nextgenhealthcare/connect#5293, closed issue and renamed appending |
Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>
99a0e5e to
feeeb74
Compare
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.
Pull request overview
This PR enhances the VMRouter class to automatically track message routing chains, similar to how Channel Writer connectors work. The enhancement adds source channel IDs and message IDs to the sourceMap of routed messages, making it easier to trace message flow through multiple channels when using VMRouter programmatically instead of Channel Writer connectors.
Key Changes:
- Added a new VMRouter constructor that accepts tracking information (channelId, messageId, sourceMap)
- Introduced new overloaded routing methods that accept sourceMap and destinationSet parameters for enhanced control
- Created a TrackingEnhancer inner class that enriches source maps with routing chain information
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| server/src/com/mirth/connect/server/util/javascript/JavaScriptScopeUtil.java | Adds addRouterEnhancement method to create VMRouter instances with tracking capabilities and integrates them into all JavaScript execution scopes (preprocessor, postprocessor, filter/transformer, etc.). Also adds an unused doGetContext method. |
| server/src/com/mirth/connect/server/userutil/VMRouter.java | Extends VMRouter with new constructor, overloaded routing methods for enhanced control, TrackingEnhancer inner class for automatic source tracking, createRawMessage helper method, and improved Javadoc with @link references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public RawMessage createRawMessage(Object message, Map<String, Object> sourceMap, | ||
| Collection<Number> destinationSet) { | ||
| if (trackingEnhancer != null) { | ||
| sourceMap = trackingEnhancer.enrich(sourceMap); | ||
| } | ||
|
|
||
| if (message instanceof byte[]) { | ||
| return new RawMessage((byte[]) message, destinationSet, sourceMap); | ||
| } else { | ||
| return new com.mirth.connect.donkey.model.message.RawMessage(message.getRawData(), message.getDestinationMetaDataIds(), message.getSourceMap()); | ||
| return new RawMessage(message.toString(), destinationSet, sourceMap); | ||
| } | ||
| } |
Copilot
AI
Dec 12, 2025
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.
The createRawMessage method doesn't validate that the message parameter is non-null before calling message.toString() on line 282. If a null message is passed, this will throw a NullPointerException. Consider adding validation at the beginning of this method to throw a more meaningful exception or handle the null case appropriately.
| * Adds additional message tracking data. | ||
| * | ||
| * TrackingEnhancer |
Copilot
AI
Dec 12, 2025
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.
The Javadoc comment "TrackingEnhancer" on line 289 is redundant as it simply restates the class name. Consider either removing this line or expanding it to provide a more meaningful description of what the TrackingEnhancer does, such as "Inner class that enriches source maps with message tracking information for routing chains."
| * Adds additional message tracking data. | |
| * | |
| * TrackingEnhancer | |
| * Inner class that enriches source maps with message tracking information for routing chains. | |
| * Used to add channel and message identifiers to the source map for tracking purposes. |
| List<String> sourceChannelIds = lookupAsList("sourceChannelIds", "sourceChannelId"); | ||
| List<String> sourceMessageIds = lookupAsList("sourceMessageIds", "sourceMessageId"); | ||
|
|
||
| HashMap<String, Object> newSourceMap = new HashMap<String, Object>(messageSourceMap); |
Copilot
AI
Dec 12, 2025
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.
The HashMap is constructed with an initial capacity based on messageSourceMap size, but then 4 additional entries are added. To avoid potential resizing, consider using new HashMap<>(messageSourceMap.size() + 4) or similar to ensure adequate initial capacity.
| HashMap<String, Object> newSourceMap = new HashMap<String, Object>(messageSourceMap); | |
| HashMap<String, Object> newSourceMap = new HashMap<String, Object>(messageSourceMap.size() + 4); | |
| newSourceMap.putAll(messageSourceMap); |
| ((Collection<?>) primaryValue).stream().map(i -> i.toString()).forEach(result::add); | ||
| } else if (primaryValue instanceof Object[]) { | ||
| Arrays.stream((Object[]) primaryValue).map(i -> i.toString()).forEach(result::add); |
Copilot
AI
Dec 12, 2025
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.
The lambda expression i -> i.toString() can be simplified to a method reference: Object::toString. Similarly on line 357. This improves code readability and is a common Java best practice.
| ((Collection<?>) primaryValue).stream().map(i -> i.toString()).forEach(result::add); | |
| } else if (primaryValue instanceof Object[]) { | |
| Arrays.stream((Object[]) primaryValue).map(i -> i.toString()).forEach(result::add); | |
| ((Collection<?>) primaryValue).stream().map(Object::toString).forEach(result::add); | |
| } else if (primaryValue instanceof Object[]) { | |
| Arrays.stream((Object[]) primaryValue).map(Object::toString).forEach(result::add); |
| /** | ||
| * Enrich the given source map with additional message tracking data. | ||
| * | ||
| * @param messageSourceMap |
Copilot
AI
Dec 12, 2025
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.
The @param documentation for messageSourceMap is missing. While parameters in private methods don't strictly require documentation, it would improve maintainability to document what this parameter represents (e.g., "the source map provided by the caller, may be null").
| * @param messageSourceMap | |
| * @param messageSourceMap the source map provided by the caller, may be null |
| /** | ||
| * Look up a value from the environment's {@link SourceMap} | ||
| * | ||
| * @param key |
Copilot
AI
Dec 12, 2025
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.
The @param documentation for key is missing. Adding a brief description (e.g., "the key to look up in the source map") would improve code documentation consistency.
| * @param key | |
| * @param key the key to look up in the source map |
| public Context doGetContext(ContextFactory contextFactory) { | ||
| return JavaScriptScopeUtil.getContext(contextFactory); | ||
| } | ||
|
|
Copilot
AI
Dec 12, 2025
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.
This public method doGetContext appears to be unused throughout the codebase. It's a non-static wrapper around the static getContext method but serves no clear purpose in this PR. Consider removing it unless there's a specific planned use case.
| public Context doGetContext(ContextFactory contextFactory) { | |
| return JavaScriptScopeUtil.getContext(contextFactory); | |
| } | |
| * | ||
| * TrackingEnhancer | ||
| */ | ||
| private class TrackingEnhancer { |
Copilot
AI
Dec 12, 2025
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.
TrackingEnhancer should be made static, since the enclosing instance is not used.
| private class TrackingEnhancer { | |
| private static class TrackingEnhancer { |
| * | ||
| * @see #routeMessageByChannelId(String, Object, Map, Collection) | ||
| */ | ||
| public Response routeMessageByChannelId(String channelId, Object message) { |
Copilot
AI
Dec 12, 2025
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.
Method VMRouter.routeMessageByChannelId(..) could be confused with overloaded method routeMessageByChannelId, since dispatch depends on static types.
| * @return The {@link Response} object returned by the channel, if its source | ||
| * connector is configured to return one. | ||
| */ | ||
| public Response routeMessageByChannelId(String channelId, RawMessage rawMessage) { |
Copilot
AI
Dec 12, 2025
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.
Method VMRouter.routeMessageByChannelId(..) could be confused with overloaded method routeMessageByChannelId, since dispatch depends on static types.
Enhance the VMRouter using @tonygermano's existing code template.
Initial PR with @jonbartels feedback is here.
Note that I removed the boolean property that toggled its functionality, so it's always on now.