Maintain consistent element order in exports#45
Maintain consistent element order in exports#45rogin wants to merge 2 commits intoOpenIntegrationEngine:mainfrom
Conversation
Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>
rogin
force-pushed
the
maint-element-order-exports
branch
from
c944e16 to
8630dde
Compare
| package com.mirth.connect.donkey.model.message; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.TreeMap; |
There was a problem hiding this comment.
Since a TreeMap is sorted and a HashMap is not, I'm guessing there could be considerable changes compared to a prior export the first time an export is done with the new code?
There was a problem hiding this comment.
Correct, then subsequent exports will benefit.
There was a problem hiding this comment.
Is there a purpose for having MapContent as part of this PR? I think it only shows up in a message export, and not part of the server configuration?
There was a problem hiding this comment.
I'll revert it and verify.
server/src/com/mirth/connect/model/converters/MapContentConverter.java
Outdated
Show resolved
Hide resolved
rogin
dismissed stale reviews from kpalang, gibson9583, and jonbartels
via
7162f41
|
Can you describe your testing? I'm sure this does work when creating and exporting new code templates and libraries. Do we know for certain that when xstream creates the objects from xml that it isn't recreating them as HashSets? |
jonbartels
added
enhancement
|
@rogin would you be able to complete the DSO and rebase this so we can include it in our next release? |
There was a problem hiding this comment.
Pull request overview
This PR addresses non-deterministic ordering in exported artifacts by switching key collection types to sorted implementations so serialized output is stable across runs, improving diffability and reducing noisy changes during import/export.
Changes:
- Use
TreeSetfor code template library channel ID sets to stabilize export ordering. - Use
TreeSetforCodeTemplateContextSetto stabilize context ordering. - Use
TreeMap/TreeMap-backed defaults in map serialization/content to stabilize map entry ordering in serialized output.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/src/com/mirth/connect/model/codetemplates/CodeTemplateLibrary.java | Switch enabled/disabled channel ID sets to sorted sets for stable export order. |
| server/src/com/mirth/connect/model/codetemplates/CodeTemplateContextSet.java | Use a sorted set delegate to ensure consistent iteration/serialization order of contexts. |
| donkey/src/main/java/com/mirth/connect/donkey/util/MapUtil.java | Use TreeMap in recovery/deserialization paths to produce deterministic serialized map output. |
| donkey/src/main/java/com/mirth/connect/donkey/model/message/MapContent.java | Default map-backed content to TreeMap to encourage stable map iteration/serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public CodeTemplateLibrary() { | ||
| id = UUID.randomUUID().toString(); | ||
| enabledChannelIds = new HashSet<String>(); | ||
| disabledChannelIds = new HashSet<String>(); | ||
| enabledChannelIds = new TreeSet<String>(); | ||
| disabledChannelIds = new TreeSet<String>(); | ||
| codeTemplates = new ArrayList<CodeTemplate>(); | ||
| } |
There was a problem hiding this comment.
The constructor now initializes enabled/disabledChannelIds as TreeSet for deterministic ordering, but the setters still accept and assign arbitrary Set implementations. There are call sites that pass HashSet (e.g., client CodeTemplatePanel, server ChannelServletTest), which will reintroduce non-deterministic export order and undermine the purpose of this change. Consider defensively copying in the setters (e.g., wrap in a new TreeSet) so CodeTemplateLibrary always holds a sorted set regardless of how it’s populated (including after deserialization or UI updates).
| return serializer.serialize(map); | ||
| } catch (Exception e) { | ||
| Map<String, Object> newMap = new HashMap<String, Object>(); | ||
| Map<String, Object> newMap = new TreeMap<String, Object>(); | ||
|
|
||
| for (Entry<String, Object> entry : map.entrySet()) { |
There was a problem hiding this comment.
These changes aim to make map serialization deterministic by using TreeMap, but existing MapUtil tests don’t assert ordering. To prevent regressions (and to validate the PR’s main goal), add a unit test that serializes a map with out-of-order keys and verifies the resulting XML entry order is consistent/sorted for the TreeMap-backed paths (both the fallback re-serialization path and deserializeMapWithInvalidValues).
Fixes an old and common complaint. Previous test results here. I've only verified that it built successfully using Java 8.
Testing points:
diff) are improved with consistent order