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

feat(crd-generator): Add support for more validation constraints #6447

Merged
merged 16 commits into from
Dec 3, 2024

Conversation

baloo42
Copy link
Contributor

@baloo42 baloo42 commented Oct 13, 2024

Description

Fixes #5836 (Add support for size constraints)
Fixes #5868 (Add support for exclusiveMinimum/exclusiveMaximum)

Add support for the following validation constraints:

  • exclusiveMinimum / exclusiveMaximum by extending @Min and @Max annotations
  • minLength / maxLength for strings with new @Size annotation
  • minItems / maxItems for array like containers with new @Size annotation
  • minProperties / maxProperties for map like containers with @Size annotation

Notes:

  • @Size will be detected on container types similar to @Pattern and @Min/@Max.
  • The use of @Min/@Max and @Size is now restricted to their intended types,
    e.g. @Min/@Max is only intended to be used on number types and not on strings.
  • This implementation uses regarding "exclusivness" the same strategy as described in JSR-303:
    A boolean inclusive flag in the annotation, defaulting to true.

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

@baloo42 baloo42 changed the title feat(crd-generator): Add support for more validation constratins feat(crd-generator): Add support for more validation constraints Oct 13, 2024
@baloo42 baloo42 marked this pull request as ready for review October 13, 2024 15:39
@baloo42 baloo42 marked this pull request as draft November 3, 2024 13:30
Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.

@baloo42
Copy link
Contributor Author

baloo42 commented Nov 20, 2024

@andreaTP can you have a look on this?

You said here:

I would use exclusive as opposed to inclusive as it better matches JSONSchema terminology.

Are you fine with this implementation (which follows common annotations) or should we follow the JSONSchema terminology?

@manusa Do you have an opinion on this?

Other open discussion topics are:

  • Should @Size be detected on container types, if the type itself is a container?
    e.g. List<@Size(min=1) List<@Size(min=2) String>>
  • I think our @Min/@Max annotation has the wrong type for the value. It feels wrong that we are using double. Are there any plans or thoughts to tackle this? Should we create a new issue for this? (would be a breaking change)

@manusa manusa added this to the 7.0.0 milestone Dec 2, 2024 — with automated-tasks
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx!

@manusa Do you have an opinion on this?

I really don't care as long as it's intuitive for users.

Since this doesn't involve any breaking changes I think it's fine if we delay it to 7.1.

What I'd really like is to release v7.0 this week. I'm not sure if there's anything else left.

@baloo42 baloo42 marked this pull request as ready for review December 2, 2024 22:35
@baloo42
Copy link
Contributor Author

baloo42 commented Dec 2, 2024

@manusa Do you have an opinion on this?
I really don't care as long as it's intuitive for users.

We are following Jakarta Bean Validation here, so this PR should be fine in terms of your goal.

Since this doesn't involve any breaking changes I think it's fine if we delay it to 7.1.

If the following change is considered as bugfix, then we can postpone it:

The use of @Min/@Max and @Size is now restricted to their intended types,
e.g. @Min/@Max is only intended to be used on number types and not on strings.


What I'd really like is to release v7.0 this week. I'm not sure if there's anything else left.

I have not found anything else breaking, so feel free to create the release with or without this PR.

@manusa
Copy link
Member

manusa commented Dec 3, 2024

If the following change is considered as bugfix, then we can postpone it:

OK; I forgot about that change. I'm merging it now.

Maybe you want to add something in the MIGRATIOM-v7.md section for the CRD Generator.

@manusa manusa merged commit b3a5fea into fabric8io:main Dec 3, 2024
21 checks passed
@baloo42 baloo42 deleted the feature/iss_5868 branch December 3, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants