[Enhancement] CrdGenerator validate @JsonPropertyOrder

[tombentley]

Can we improve the CrdGenerator to:

1. validate that the @JsonPropertyOrder is present
2. validate that the properties given are all properties of the class
3. validate that no properties of the class (including inherited properties) are missing

[samuel-hawker]

@tombentley this is a great idea.

A question of implementation, would you want this to fail a build if any of the above validations fail, or print them out as warnings?
I am guessing the prior, but just thought it might be useful to clarify in this issue.

[tombentley]

Fail the build. People usually overlook warnings.

[samuel-hawker]

I haven't had time to finish this.
The core new logic for validating the order property is present was added here:
https://github.com/strimzi/strimzi-kafka-operator/pull/2820/files#diff-52292ed5de154b1a03087dc5c1d64bceR447-R458
However in a larger test restructure I got pretty lost and a lot of the tests no longer work due to the complications that errors collect in an array and many pre-existing tests actually print out many errors, meaning it fairly difficult or unreliable to test.

If someone wants to pick this up, feel free to look at my above closed PR #2820

[scholzj]

Where does the property order actually matter? When working on the api-conversion, it in theory should be used. But in practice, for example when using kubectl, it anyway reorders them. So the question is ... does this really matter?

[scholzj]

Triaged on 7.6.2022: We should either validate it or remove it completely. But right now it is not clear what is it actually useful for.

@tombentley why do you think we should have this and define the order?

[tombentley]

It mostly doesn't matter, because whatever order we use it's only honoured when it's us serializing the JSON. That's why kubectl reorders them. But for things like our tests it allows us to compare strings, which can be useful. It is also be useful for any tooling we (or others, since this is part of the api module) might write that displays YAMLs (e.g. strimzictl). I think this is worth keeping.

[scholzj]

Triaged on 21st June: We should validate that all fields are present at build time.

[steffen-karlsson]

Hello @scholzj

Is this still relevant? If so, I'd like to give it a go.
If possible can you list expected outcome after the solution is successfully implemented?
Is it still expected to be implemented in the CrdGenerator?

[scholzj]

I think the expectation is that it will check if:

• The JsonPropertyOrder annotation is set
• If it has all the relevant fields (not sure how easy or hard the definition of relevant field is -> we want those generated into the CRD spec to be included. But not the additionalProperties, constants etc.)

If the annotation or some fields is missing, it should raise the error and fail. The PR adding this feature should also update any of the missing fields so that we can merge it without failing the main build.

And then once it is merged, when new field is added tot he API but not to the property order or when a new class is added without the property order, it would simply fail the build and force the user to fix it.

[steffen-karlsson]

Sounds good @scholzj.

Just to clarify, when you say all the relevant fields, you mean the ones represented in JsonPropertyOrder?
And they all need to have a value and default is acceptable as well?

Secondly, how can we identify the classes requiring JsonPropertyOrder? Is it just all in io.strimzi.api.kafka.model or all being a property of a class inheriting from CustomResource?

Ill take a look at this one :)

[scholzj]

No, what the check should do is:

• Get all the fields from the class that will be added to the custom resource definition (I think it identifies them through the get methods)
• Validate that all of them are listed in the JsonPropertyOrder annotation

it would apply to all the POJO classes in the api module basically. The whole https://github.com/strimzi/strimzi-kafka-operator/tree/main/api/src/main/java/io/strimzi/api/kafka/model tree. But as you would validate it in the CRD Generator, it basically goes through these classes from the Custom Resource implementations, so you do not need to browse the tree, you can just follow the classes as processed in the CRD Generator.

[steffen-karlsson]

Makes sense, thanks @scholzj :)

This is already what I did, so makes sense.
All left is to do the comparison of the fields with the JsonPropertyOrder and fix all the classes.