SED-4912 yaml formatting improve ordering of fields - #699
Conversation
…rove-ordering-of-fields
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to control the serialization order of fields in YAML models using a new @YamlFieldOrder annotation and YamlFieldPriority enum. It updates several YAML model classes and configures a custom Jackson serializer modifier to enforce this ordering. The review feedback highlights a critical compilation error in SimpleYamlArtefact.java due to a non-existent Jackson method, suggests a performance optimization in YamlPlanReader.java to avoid redundant reflection calls, and recommends renaming the enum constants in YamlFieldPriority to adhere to standard Java naming conventions.
…-ordering-of-fields' into SED-4912-yaml-formatting-improve-ordering-of-fields
| protected List<NamedYamlArtefact> children = new ArrayList<>(); | ||
|
|
||
| @YamlFieldCustomCopy | ||
| @YamlFieldOrder(YamlFieldPriority.LOW) |
There was a problem hiding this comment.
I haven't tested it, but according to the modify JSON schema on this branch the order is now children -> before -> after instead of before > children > after. Please check it and also if we already have unit tests that validates it.
There was a problem hiding this comment.
The json schema does not seem to enforce ordering, and from what I saw in the generated json the order is completely reverse. Rudimentary covarage is given by the ap collection unit tests but you are right we should add some more. Will do.
There was a problem hiding this comment.
Thanks, for the test. I guess the order in the schema may impact the auto-completion in intellij, but that's another topic.
| @@ -71,7 +74,18 @@ protected void fillYamlArtefactFields(T artefact) { | |||
|
|
|||
| public ObjectNode toFullJson() { | |||
| ObjectNode jsonNode = yamlObjectMapper.valueToTree(this); | |||
There was a problem hiding this comment.
This seems relatively complex also in term of perforamnce, and previously the setAll would override any existing properties values with the one of the fieldValues object. Not verified, but it seems that @JsonAnyGetter would be more adapted
There was a problem hiding this comment.
JsonAnyGetter won't control for field ordering, tried that. My honest opinion is that SimpleYAMLArtefact as a whole introduces a lot of complexity and breaks many of the rich features and conventions of the Jackson library, with all the conversions to and from object to JsonObject. Even the avoidance of printing default values in the yaml is perfectly well covered by Jackson annotations and does not require any copying back and forth.
This complexity with the SimpleYamlArtefact is also the primary reason why all my ideas for the diff engine fell flat at the beginning of the year. I'd love to get rid of this altogether, but I always deemed it too much of a can of worms. But maybe with the knowledge I have now it would be of great benefit down the road to simplify it. Let me know if I should tackle this.
There was a problem hiding this comment.
We know that we have to review and refactor the whole implementation around reading/writing YAML for AP entities in order to algin it, simplify it, make it more pluggable, but not something we achieve for the next major release.
| @@ -71,7 +74,18 @@ protected void fillYamlArtefactFields(T artefact) { | |||
|
|
|||
| public ObjectNode toFullJson() { | |||
| ObjectNode jsonNode = yamlObjectMapper.valueToTree(this); | |||
There was a problem hiding this comment.
We know that we have to review and refactor the whole implementation around reading/writing YAML for AP entities in order to algin it, simplify it, make it more pluggable, but not something we achieve for the next major release.
| protected List<NamedYamlArtefact> children = new ArrayList<>(); | ||
|
|
||
| @YamlFieldCustomCopy | ||
| @YamlFieldOrder(YamlFieldPriority.LOW) |
There was a problem hiding this comment.
Thanks, for the test. I guess the order in the schema may impact the auto-completion in intellij, but that's another topic.
2 participants
No description provided.