Sample program for YAML file patching abilities #2206
Conversation
amorton
left a comment
amorton
left a comment
There was a problem hiding this comment.
ok, lets make some changes to this is more a basic JsonMerger.
We will sync on the next ticket to handle the Yaml merging
| } | ||
|
|
||
| /** Core merge logic following the documented semantics. */ | ||
| public JsonNode mergeNodes(JsonNode base, JsonNode patch) { |
There was a problem hiding this comment.
why do we have our own logic and not using something like https://github.com/java-json-tools/json-patch ?
should add some comments.
There was a problem hiding this comment.
I think use JSON Patch would not help here -- it's standard for defining stand-alone patch definitions to use for merging changes (diffs) across documents, bit like unix diff and patch commands. But it would not help here where task is updating one document with another
(not sure I am explaining this well).
Renaming of second document as overrides or something (instead of patch) would help understand the difference.
| } | ||
|
|
||
| /** Merge two YAML strings and return the merged node. */ | ||
| public JsonNode mergeToNode(String baseYaml, String patchYaml) { |
| } | ||
|
|
||
| /** Serialize a node back to a YAML string. */ | ||
| public String toYaml(JsonNode node) { |
| } | ||
|
|
||
| /** Merge two YAML strings and return the merged YAML string. */ | ||
| public String mergeYamlStrings(String baseYaml, String patchYaml) { |
There was a problem hiding this comment.
thinking about this - remove the two mergeYaml* functions and rename to JsonMerger. Can also make the mergeNodes method static.
the caller will be responsble for reading the YAML and putting back to YAML.
the reason is we will want to have better control of errors when reading the yaml, e.g. what if the patch YAML is invalid ?
There was a problem hiding this comment.
Merging functionality is format-agnostic, too, (even if type is called JsonNode) which makes it usable for JSON, YAML and other Jackson-supported formats (XML, Avro, Protobuf...) should those ever be needed.
| if (node.isObject() || node.isArray()) { | ||
| return node.deepCopy(); | ||
| } | ||
| if (node instanceof ValueNode) { |
There was a problem hiding this comment.
this feels like a future bug, when if the caller changed the vlue of the returned ValueNode ? we shoud not return any of the same objects.
There was a problem hiding this comment.
ValueNodes are fully immutable, cannot be changed and meant to be shared/reused.
(I agree with not sharing mutable ArrayNodes and ObjectNodes of course)
| return deepCopy(patch); | ||
| } | ||
|
|
||
| private JsonNode deepCopy(JsonNode node) { |
There was a problem hiding this comment.
this can be cleaner using a modern switch on node.
case for null, case for ValueNode, and default is call deepCopy()
|
|
||
| // If both are arrays, replace entirely with patch (no element-wise merging) | ||
| if (base.isArray() && patch.isArray()) { | ||
| return ((ArrayNode) patch).deepCopy(); |
There was a problem hiding this comment.
for consistency, can we call our deepCopy() below ?
but we may not even need this check, because the code below calls deepCopy() anyway
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class YamlMergerTest { |
There was a problem hiding this comment.
we should simplify these tests to just be about testing the merging without the complexity of the embedding config or other faked examples or reading other config files.
so very simple tests that make it easy to see the yaml changes.
will need to load the yaml into a JsonNode in this test class because of other changes on the YamlMerger - see comments there.
amorton
left a comment
amorton
left a comment
There was a problem hiding this comment.
hit wrong button, we want to make changes before merging
What this PR does:
Sample program for YAML file patching abilities
Which issue(s) this PR fixes:
Fixes #NA
Checklist