kubernetes-client icon indicating copy to clipboard operation
kubernetes-client copied to clipboard

CRDGenerator: Add support for exclusiveMinimum/exclusiveMaximum

Open baloo42 opened this issue 10 months ago • 6 comments

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

  • https://kubernetes.io/docs/reference/kubernetes-api/extend-resources/custom-resource-definition-v1/#JSONSchemaProps
  • https://json-schema.org/draft-04/draft-zyp-json-schema-04
  • https://docs.swagger.io/swagger-core/v2.2.21/apidocs/io/swagger/v3/oas/annotations/media/Schema.html
  • https://jakarta.ee/specifications/bean-validation/3.0/jakarta-bean-validation-spec-3.0.html#builtinconstraints-decimalmin

baloo42 avatar Apr 07 '24 19:04 baloo42

@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 🙂 ).

andreaTP avatar Apr 08 '24 17:04 andreaTP

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 avatar Apr 08 '24 18:04 baloo42

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?

baloo42 avatar Apr 08 '24 18:04 baloo42

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 🙂

andreaTP avatar Apr 10 '24 10:04 andreaTP

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[bot] avatar Jul 09 '24 22:07 stale[bot]