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

CRDGenerator: Add support for exclusiveMinimum/exclusiveMaximum #5868

Closed
baloo42 opened this issue Apr 7, 2024 · 6 comments · Fixed by #6447
Closed

CRDGenerator: Add support for exclusiveMinimum/exclusiveMaximum #5868

baloo42 opened this issue Apr 7, 2024 · 6 comments · Fixed by #6447
Labels
component/crd-generator Related to the CRD generator enhancement
Milestone

Comments

@baloo42
Copy link
Contributor

baloo42 commented Apr 7, 2024

Is your enhancement related to a problem? Please describe

At the moment the CRDGenerator does not support exclusiveMinimum/exclusiveMaximum (boolean):

https://kubernetes.io/docs/reference/kubernetes-api/extend-resources/custom-resource-definition-v1/#JSONSchemaProps

exclusiveMaximum

5.1.2.1. Valid values
The value of "maximum" MUST be a JSON number. The value of "exclusiveMaximum" MUST be a boolean.

If "exclusiveMaximum" is present, "maximum" MUST also be present.

5.1.2.2. Conditions for successful validation
Successful validation depends on the presence and value of "exclusiveMaximum":

if "exclusiveMaximum" is not present, or has boolean value false, then the instance is valid if it is lower than, or equal to, the value of "maximum";
if "exclusiveMaximum" has boolean value true, the instance is valid if it is strictly lower than the value of "maximum".

5.1.2.3. Default value
"exclusiveMaximum", if absent, may be considered as being present with boolean value false.

exclusiveMinimum

5.1.3.1. Valid values
The value of "minimum" MUST be a JSON number. The value of "exclusiveMinimum" MUST be a boolean.

If "exclusiveMinimum" is present, "minimum" MUST also be present.

5.1.3.2. Conditions for successful validation
Successful validation depends on the presence and value of "exclusiveMinimum":

if "exclusiveMinimum" is not present, or has boolean value false, then the instance is valid if it is greater than, or equal to, the value of "minimum";
if "exclusiveMinimum" is present and has boolean value true, the instance is valid if it is strictly greater than the value of "minimum".

5.1.3.3. Default value
"exclusiveMinimum", if absent, may be considered as being present with boolean value false.

Describe the solution you'd like

The existing annotations @Min and @Max allow to define the "inclusiveness":

@Target({ ElementType.ANNOTATION_TYPE, ElementType.FIELD, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
public @interface Min {
  /**
   * @return the element must be higher or equal to
   */
  double value();

  /**
   * Specifies whether the specified minimum is inclusive or exclusive.
   * By default, it is inclusive.
   *
   * @return {@code true} if the value must be higher or equal to the specified minimum,
   *         {@code false} if the value must be higher
   */
  boolean inclusive() default true;
}
@Target({ ElementType.ANNOTATION_TYPE, ElementType.FIELD, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
public @interface Max {
  /**
   * @return value the element must be lower or equal to
   */
  double value();

  /**
   *  Specifies whether the specified maximum is inclusive or exclusive.
   *  By default, it is inclusive.
   *
   * @return {@code true} if the value must be higher or equal to the specified minimum,
   *         {@code false} if the value must be higher
   */
  boolean inclusive() default true;
}

Note that this won't be a breaking change because the above defaults would result in exclusiveMinimum / exclusiveMaximum = false which can be omitted and which is the case at the moment.

Describe alternatives you've considered

We could introduce instead or additionally a new annotation @Schema like in swagger-core, which allows to specify exclusiveMinimum/exclusiveMaximum directly.

But in my opinion it's more important to tie it directly to the value.

Additional context

@andreaTP
Copy link
Member

andreaTP commented Apr 8, 2024

@baloo42 I'll try to catch up with those linked issues in the next few days(I wanna dedicate to this the right amount of attention), feel free to ping me directly if I miss something or I'm not answering somewhere(I might just have missed the notification 🙂 ).

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 8, 2024

No problem - take your time. This issue here has no priority for me. The intention was to get an overview of the missing features and how possible solutions could look like.

Now we have a place where we can discuss solutions to this problem.

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 8, 2024

TBD:

  • At the moment, double is used. Is this a problem in combination with "inclusiveness"? (if we want to do later more...)
  • What validations should be applied during the generation phase?
  • Fail fast or log warnings and omit on wrong datatypes?

@andreaTP
Copy link
Member

Reviewing this one and the only note is that I would use exclusive as opposed to inclusive as it better matches JSONSchema terminology.

Feel free to go ahead and we can cover the details of an implementation, I think 🙂

@manusa manusa added enhancement component/crd-generator Related to the CRD generator labels Apr 10, 2024
Copy link

stale bot commented Jul 9, 2024

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!

Copy link

stale bot commented Oct 19, 2024

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 Oct 19, 2024
baloo42 added a commit to baloo42/kubernetes-client that referenced this issue Oct 22, 2024
baloo42 added a commit to baloo42/kubernetes-client that referenced this issue Nov 4, 2024
@stale stale bot closed this as completed Nov 23, 2024
@manusa manusa reopened this Nov 25, 2024
@stale stale bot removed the status/stale label Nov 25, 2024
baloo42 added a commit to baloo42/kubernetes-client that referenced this issue Nov 28, 2024
@manusa manusa added this to the 7.0.0 milestone Dec 2, 2024 — with automated-tasks
baloo42 added a commit to baloo42/kubernetes-client that referenced this issue Dec 2, 2024
manusa pushed a commit that referenced this issue Dec 3, 2024
Add support for exclusiveMinimum and exclusiveMaximum (#5868)
---
Fix annotation description
---
Add support for minLength and maxLength (#5836)
---
Add support for minItems and maxItems (#5836)
---
Add support for minProperties and maxProperties (#5836)
---
Add tests for type annotations and cleanup
---
Add type support for strings Lists and Maps using `@Size`
---
Fix javadoc in Size annotation
---
Fix approval test
---
Rename group in approvaltest
---
Add default value handling again
---
Updated docs to describe inclusive/exclusive and `@Size` annotation
---
Fix typo
---
Cleanup SpecReplicasPathTest
---
Add unit tests
---
Add changelog entries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/crd-generator Related to the CRD generator enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants