Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java-gen] Support Min/Max/Pattern javax.validation.constraints #3870

Closed
OneCricketeer opened this issue Feb 16, 2022 · 25 comments
Closed

[java-gen] Support Min/Max/Pattern javax.validation.constraints #3870

OneCricketeer opened this issue Feb 16, 2022 · 25 comments

Comments

@OneCricketeer
Copy link

OneCricketeer commented Feb 16, 2022

Is your enhancement related to a problem? Please describe

I noticed that required fields get a @NotNull annotation applied, but there are other supported validation patterns that can be applied to generated Java models as well

https://javaee.github.io/tutorial/bean-validation002.html

Describe the solution you'd like

Examples

Mapped to

@Min(0)
@Max(1000)

type: string
pattern: '^(off)$|^(102[4-9]|10[3-9][0-9]|1[1-9][0-9]{2}|[2-9][0-9]{3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$'

Mapped to

@Pattern(...)

Describe alternatives you've considered

Manually adding these where they would be needed.

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.

@andreaTP
Copy link
Member

Just as a reference the complete list (as of today) of available annotations generated but the crd-generator is here:
https://github.com/fabric8io/kubernetes-client/blob/master/doc/CRD-generator.md

@OneCricketeer
Copy link
Author

Ah. Would be nice to have a round-trip test as well

@andreaTP
Copy link
Member

Pretty confident they will come soon (I have some POCs for this already).

@stale
Copy link

stale bot commented May 18, 2022

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@stale stale bot added the status/stale label May 18, 2022
@stale stale bot closed this as completed May 25, 2022
@manusa manusa reopened this May 26, 2022
@stale stale bot removed the status/stale label May 26, 2022
@OneCricketeer OneCricketeer changed the title [java-gen] Support javax.validation.constraints [java-gen] Support Min/Max/Pattern javax.validation.constraints May 26, 2022
@leoandrea7
Copy link
Contributor

Hi, any news about this annotations support?

@andreaTP
Copy link
Member

Hi @leoandrea7 , this is not supported yet, although it might be relatively straightforward to implement it.

Let's see if @fabiobrz gets intrigued enough 😏 , but, in general, would you mind sharing your use case?

@leoandrea7
Copy link
Contributor

I have a CRD with openAPIV3Schema like this:

....
    kafka:
      type: object
      properties:
        port:
          type: integer
          minimum: 0
          maximum: 65535
          nullable: false
        hostname:
          type: string
          pattern: '^([A-Za-z0-9-]{1,63}\.)+[[A-Za-z0-9-]{1,63}$'
          nullable: false
      nullable: false
.....

The problem is that crd-generator does not generate the fields minimum, maximum and pattern.
I see that nullable will be introduced in the next release. Issue-3975

Thanks a lot @andreaTP!

@andreaTP
Copy link
Member

@leoandrea7 quick sanity check, you mentioned crd-generator but you meant instead the Java generator, is it correct?

Also nullable is already integrated in the current implementation (i.e. 6.0.0-RC1) when you are experiencing any issue with it, please, file an issue 🙏

@OneCricketeer
Copy link
Author

I have a CRD with openAPIV3Schema like this:

....
    kafka:
      type: object
      properties:

I assume that comes from Strimzi? Last I heard, they do use Java operator SDK, so might already have models you would need instead of generating them?

@andreaTP
Copy link
Member

AFAIK Strimzi doesn't use JOSDK (so far) but straight client

@leoandrea7
Copy link
Contributor

No it is a custom CRD. The "kafka" field has nothing to do with Strimzi.

@leoandrea7
Copy link
Contributor

Anyway

@leoandrea7 quick sanity check, you mentioned crd-generator but you meant instead the Java generator, is it correct?

Also nullable is already integrated in the current implementation (i.e. 6.0.0-RC1) when you are experiencing any issue with it, please, file an issue 🙏

Actually I need it on the crd-generator, but I thought they were a single tool..

@leoandrea7
Copy link
Contributor

There is need of a new issue for crd-gen? Thanks

@andreaTP
Copy link
Member

@leoandrea7 the CRD and Java generators should possibly eventually converge, but they are, ATM two separate tools, so please, file a separate issue (and, possibly, the one on Nullable)

@leoandrea7
Copy link
Contributor

@andreaTP new issue opened

@OneCricketeer
Copy link
Author

OneCricketeer commented Jun 23, 2022

That seems to be written as a duplicate. I think the ask was to make a ticket only for any issues you're having with the nullable fields. The schemas linked in the original post are openAPIV3Schema

Edit: ignore. I see you're asking for crd-gen ; since you showed yaml already, I'd assumed you already had that

@fabiobrz
Copy link
Contributor

Hi @leoandrea7 , this is not supported yet, although it might be relatively straightforward to implement it.

Let's see if @fabiobrz gets intrigued enough smirk , but, in general, would you mind sharing your use case?

Hi @OneCricketeer, @andreaTP - IIUC the scope for this issue is just about OpenAPI v3 Schema based validation, right? i.e. what described in here - specifically the resourcedefinition.yaml example, can you confirm?

@OneCricketeer
Copy link
Author

@fabiobrz

IIUC the scope for this issue is just about OpenAPI v3 Schema based validation, right

No, this issue was requesting to add POJO annotations, unrelated to YAML/OpenAPI spec/schema validation, but POJO's created from those YAML files.

@fabiobrz
Copy link
Contributor

fabiobrz commented Sep 8, 2022

No, this issue was requesting to add POJO annotations, unrelated to YAML/OpenAPI spec/schema validation, but POJO's created from those YAML files.

Thanks @OneCricketeer - I think I just worded my question badly, sorry about that.
Yes, what I meant is this issue is about generating validation related annotations on generated POJOs. I added a reference to a YAML as an example for the related OpenAPI schema properties.

@OneCricketeer
Copy link
Author

@fabiobrz Sorry, I still don't understand.

Look at the links in the original issue description. The min max and pattern fields are part of the openapiv3 schema section of those crds

@fabiobrz
Copy link
Contributor

fabiobrz commented Sep 8, 2022

Hi @OneCricketeer - I think we're on the same page. I've looked at the links in the description already. This issue is for tracking the work needed to make java-gen support the generation of "validation" annotations, like for instance @Pattern.

Solving the two issues that you linked above (#4348 and #4384) will not solve this very issue, because they will support the generation of Fabric8 annotations - i.e. those belonging defined by io.fabric8.generator.annotation, not the ones defined by javax.validation.constraints - see #4406

@OneCricketeer
Copy link
Author

@fabiobrz One of the linked tickets discussed doing away with JavaEE annotation dependency (I recall someone called them deprecated or abandoned, or bloat...?) and thus why the new annotation module was created to keep external dependencies at a minimum.

@fabiobrz
Copy link
Contributor

fabiobrz commented Sep 8, 2022

Hi @OneCricketeer

@fabiobrz One of the linked tickets discussed doing away with JavaEE annotation dependency (I recall someone called them deprecated or abandoned, or bloat...?)

Yes, I as well have read such discussion in one of the related issues.

and thus why the new annotation module was created to keep external dependencies at a minimum.

Exactly. #4384 is for generating the new annotations, not the javax.validation.constraints that this very issue (#3870) is about.

Hence, given what we say here, it seems this can be closed or put on-hold. WDYT?

@OneCricketeer
Copy link
Author

Fine by me. Moving discussion to #4384

@OneCricketeer OneCricketeer closed this as not planned Won't fix, can't repro, duplicate, stale Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants