Add support for Min, Max, Pattern and Nullable on CRD generator fields

[andreaTP]

Description

Resolves #4224

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[manusa]

Since we're using javax.validation, maybe we should consider jsr305 annotations or support for others (JetBrains, Spring) instead of creating our own.
WDYT?

JSR 305 is definitely dead, but providing yet another annotation for the same purpose doesn't sound great.

[andreaTP]

I proposed this implementation as stated in the relevant Issue, but, I definitely relate with your feelings @manusa .

Here are my considerations:

• Spinning an own annotation to emit a single field in the CRD is great, I agree
• The current implementation of required relies on javax.validation.constraints.NotNull and here I tried to stitch with the original code design
• Adding an additional dependency (on Spring, JetBrains or Micronaut) is something I would very likely avoid as we are, anyhow kind of "abusing" the API
• The implementation of Min/Max is semantically incorrect as we are converting Long to Double effectively prohibiting the user to use floating point numbers where it's actually allowed
• Those encodings will need a corresponding match in the java-generator which is currently relying on Jackson tricks to encode e.g. Nullable
• Parsing those annotations does not guarantee at all that they are respected during deserialization, on the contrary, without the proper Jackson annotations in place ppl are going to be extremely surprised by the behavior

I'm not sure how to solve all of the above, we have a few options with different tradeoffs though:

• continue with the current approach keeping in mind the limitation and possible tech debt we are accepting
• roll out our own annotations for everything constraints to avoid extra dependencies, give a clear semantic to the API (that cannot be mixed with other frameworks etc.), in this case, we should draft a deprecation path for javax.validation.constraints.NotNull

I'm looking for other opinions and ideas here, maybe @OneCricketeer or @fabiobrz wanna chip into this discussion.

[andreaTP]

cc. @csviri too, as this is going to transitively affect JOSDK too.

[manusa]

Let's discuss this point this afternoon. Maybe I wasn't explaining myself correctly, or I misunderstand the implications.

My understanding is that since we use a string to reference the annotation, there's no need for the generator to actually have a dependency on an artifact that provides those annotations.

My proposal was supporting a set of annotations that are equivalent, for example, for Not Null: org.jetbrains.annotations.NotNull, org.springframework.lang.NonNull, javax.annotation.Nonnull, javax.validation.constraints.NotNull, and so on. Same for nullable and other cases. Maybe or maybe not provide our own annotation(s) too.

[andreaTP]

My understanding is that since we use a string to reference the annotation, there's no need for the generator to actually have a dependency on an artifact that provides those annotations.

Correct, in fact, the generator doesn't depend on the validation annotations library

My proposal was supporting a set of annotations that are equivalent

The tricky part about this approach is that every implementation has its own nuances and semantics but we are using it for a very different purpose. We should check for each available annotation semantic and, ideally, properly test both generation and runtime behavior.

[andreaTP]

The agreed approach is to spin out our own annotations, mentioning that they are only relevant for generating the corresponding fields in the CRD (e.g. no runtime guarantees for ser/deser).
The change will be made in a retro-compatible way emitting a warning when javax.validation.constraints.* is used.

[iocanel]

IMHO javax.validation.contraints should be the first thing supported. It's a standard, it's aligned with what we already have in place, it's supported by the builder validation ...

[manusa]

One of the issues we discussed about with these more generic annotations, was their lack of alignment with Kubernetes OpenApi Schema spec.

For example, the @Max annotation expects a long as a value, while the Kubernetes equivalent is a float64.

[fabiobrz]

I'm looking for other opinions and ideas here, maybe @OneCricketeer or @fabiobrz wanna chip into this discussion.

I actually noticed I was mentioned here just now. Sorry for lagging on replying, @andreaTP.
Honestly my initial feelings were about pushing back on this, since well... we're basically giving up on adoptiong a standard spec, i.e. the javax.validation.* one and consequently not leveraging the benefits that these deliver during runtime ser/de process.

That being said, though, I agree as well with some of the considerations you added to your first comment, for instance the one about differences in actual types definition and so on.

In the end, we know already the generated Java model depends on the Fabric8 Kubernetes Client already, i.e. see how default values are generated, and this tradeoff has been accepted.

Here though, we're pushing the limit even forward, by generating annotations that look like standard javax.validation.* ones, allow for correspondent CRD generation, but won't provide anything that looks like ser/de validation at runtime. The original java-generator issue, though, required for this to happen:

Additional context

The objects will fail to be constructed if an invalid value is provided when the annotations are present.

Use-case: Preventing bad user-provided input fields into an API layer.

Bottom line, it seems that the proposed (and currently accepted) solotion - i.e. to roll out own annotations - won't solve the original issue.
In case this is acceptable, I think this should be made very clear to users of both the CRD and the Java generator, e.g.: the following sentence:

The agreed approach is to spin out our own annotations, mentioning that they are only relevant for generating the corresponding fields in the CRD (e.g. no runtime guarantees for ser/dese

should be highlighted in the project APIs documentation so that users won't expect such runtime features from the JOSDK they'd generate.

CC @manusa

[andreaTP]

@fabiobrz I agree with your analysis, please, feel free to submit a PR to improve/make more clear the current docs, it will be really appreciated!

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

80.0% Coverage
6.1% Duplication

[andreaTP]

Superseded #4348