add validation to description based on the validation annotations by edewit · Pull Request #47375 · keycloak/keycloak

edewit · 2026-03-23T15:37:43Z

fixes: #47371
Signed-off-by: Erik Jan de Wit erikjan.dewit@gmail.com

fixes: keycloak#47371 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Copilot

Pull request overview

This PR enhances the Admin Client v2 OpenAPI generation so that Jakarta Validation (and a couple of Keycloak custom constraints) are reflected in the published schema, primarily by appending constraint information to field descriptions and additionally by setting some machine-readable schema constraints.

Changes:

Introduces ValidationAnnotationScanner to read validation annotations (field, type-use, and select class-level) and translate them into OpenAPI schema properties and/or description text.
Updates OASModelFilter to invoke the scanner while populating schema property descriptions, appending a Validation: ... suffix and setting basic schema constraints (e.g., minLength, pattern, minimum).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
rest/admin-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java	New scanner that inspects Jandex metadata for validation annotations and produces schema property updates + human-readable validation text.
rest/admin-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/OASModelFilter.java	Integrates the scanner into OpenAPI model filtering, combining field-level and class-level validations into property descriptions and applying schema constraints.

Copilot · 2026-03-23T16:23:52Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+        // @NotEmpty implies minItems: 1 for arrays/collections
+        AnnotationInstance notEmpty = getFieldAnnotation(field, NOT_EMPTY);
+        if (notEmpty != null && propertySchema.getMinItems() == null) {
+            propertySchema.setMinItems(1);


@NotEmpty is translated to minItems=1 unconditionally, but @NotEmpty can also apply to String/CharSequence where the equivalent OpenAPI constraint is minLength=1. Please branch based on the schema/field type (string vs array/collection vs map) to avoid emitting invalid or misleading OpenAPI constraints.

Suggested change

// @NotEmpty implies minItems: 1 for arrays/collections

AnnotationInstance notEmpty = getFieldAnnotation(field, NOT_EMPTY);

if (notEmpty != null && propertySchema.getMinItems() == null) {

propertySchema.setMinItems(1);

// @NotEmpty: behavior depends on schema type

AnnotationInstance notEmpty = getFieldAnnotation(field, NOT_EMPTY);

if (notEmpty != null) {

Schema.SchemaType schemaType = propertySchema.getType();

if (schemaType == Schema.SchemaType.STRING) {

if (propertySchema.getMinLength() == null) {

propertySchema.setMinLength(1);

}

} else if (schemaType == Schema.SchemaType.ARRAY) {

if (propertySchema.getMinItems() == null) {

propertySchema.setMinItems(1);

}

} else if (schemaType == Schema.SchemaType.OBJECT) {

if (propertySchema.getMinProperties() == null) {

propertySchema.setMinProperties(1);

}

}

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

Copilot · 2026-03-23T16:23:53Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+        // Check for @URL
+        AnnotationInstance url = getFieldAnnotation(field, URL);
+        if (url != null) {
+            constraints.add("must be a valid URL");
+        }


buildDescription claims to include validation group context, but group context is only prepended for @NotBlank/@NotNull/@NotEmpty (and class-level constraints). For consistency (and to match the Javadoc), either prepend getGroupContext(...) for the other supported constraints too (@URL, @Size, @Pattern, @Min, @Max, @Valid), or adjust the documentation to reflect the current behavior.

Copilot · 2026-03-23T16:23:53Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+        // @NotBlank implies minLength: 1 for strings (only for field-level, not type args)
+        AnnotationInstance notBlank = getFieldAnnotation(field, NOT_BLANK);
+        if (notBlank != null && propertySchema.getMinLength() == null) {
+            propertySchema.setMinLength(1);
+        }


applySchemaProperties applies machine-readable constraints (e.g., minLength) even when the underlying validation annotation is restricted to specific validation groups. This can make the schema overly strict for operations where the constraint does not apply (e.g., BaseClientRepresentation.clientId is @NotBlank(groups = CreateClient.class) but would still get minLength=1 globally). Consider only applying machine-readable constraints when groups is empty/default, and rely on the human-readable description for group-scoped constraints (or otherwise encode the group scope without tightening the base schema).

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

michalvavrik

I think it would be nice to have this tested. I realize that if something changes (like field is removed or named) then tests would break, but I'd really prefer if for each validation constraint we already use, there was one description test, e.g. added to OpenApiDistTest. Or if you prefer, you can create a unit test and test ValidationAnnotationScanner in isolation with manually created index and test classes.

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

SmallRye handles this Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

michalvavrik · 2026-03-24T18:31:03Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+    }
+
+    private String getOperationContext(String groupSimpleName) {
+        for (Map.Entry<String, String> entry : OPERATION_PREFIXES.entrySet()) {


Do I understand it right that at this place, you cannot reference org.keycloak.representations.admin.v2.validation.PutClient, org.keycloak.representations.admin.v2.validation.PatchClient and org.keycloak.representations.admin.v2.validation.CreateClient directly or are you trying to make this generic?

I don't really have hard opinion on this, but personally, if I were writing it and I could reference org.keycloak.representations.admin.v2.validation, I'd just hardcode known groups and fail build if we hit unknown options. Thus whoever adds a new group, would see failing tests and link group to description like "on patch".

But it is just opinion, if you like that, fine.

I'm not completely sure what you mean by that, but added this error to the scanner in this commit 8bfc3c3

I'm not completely sure what you mean by that, but added this error to the scanner in this commit 8bfc3c3

8bfc3c3 is exactly what I meant. it wasn't there when I reviewed. Thanks

michalvavrik · 2026-03-24T18:34:10Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+    private static final DotName PATTERN = DotName.createSimple("jakarta.validation.constraints.Pattern");
+    private static final DotName MIN = DotName.createSimple("jakarta.validation.constraints.Min");
+    private static final DotName MAX = DotName.createSimple("jakarta.validation.constraints.Max");
+    private static final DotName VALID = DotName.createSimple("jakarta.validation.Valid");


Can you clarify what do you prefer constants over DotName.createSimple(Valid.class)?

I don't have an opinion about it the class overload is new could change it to that

that's just my preference, I'll leave it to you to decide.

michalvavrik

works for me; the only thing I am surprised is that if these changes OpenApi descriptions, I think you didn't build the whole project as if you did, the OpenAPI stored in JS dir would change and we could see clearly effect of these changes.

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

mabartos · 2026-03-25T09:54:49Z

There's already this logic on the SmallRye side - https://github.com/smallrye/smallrye-open-api/blob/main/core/src/main/java/io/smallrye/openapi/runtime/scanner/dataobject/BeanValidationScanner.java#L32

It'd be good if we could leverage it.

I'd like to avoid any maintenance of this custom code.

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

edewit · 2026-03-25T12:41:16Z

Right @mabartos that's why I removed my custom schema property logic for standard Jakarta Validation annotations (@notblank, @notempty, @SiZe, @pattern, @min, @max) -
• Kept @url → format: uri handling (SmallRye doesn't support Hibernate Validator's @url)
• Kept all human-readable description building (SmallRye doesn't do this)

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2026-03-25T15:29:52Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.BruteForceTest#testExceedMaxTemporaryLockouts

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected LoginPage but was localhost (https://localhost:8543/auth/realms/test/login-actions/authenticate?session_code=ZEaHEttdxxO2_1sbCYSu_G8ORCbChBBg8VNB7IBedbw&execution=1750cf17-4ff5-4b9d-ac9b-234632b7b5d8&client_id=test-app&tab_id=UmiXYvUq1JA&client_data=eyJydSI6Imh0dHBzOi8vbG9jYWxob3N0Ojg1NDMvYXV0aC9yZWFsbXMvbWFzdGVyL2FwcC9hdXRoIiwicnQiOiJjb2RlIn0)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:39)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
...

Report flaky test

vmuzikar · 2026-03-25T17:25:04Z

@shawkins @Pepo48 @mabartos Can you please (re-)review as well?

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

.../rest/src/test/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScannerTest.java

Copilot · 2026-03-26T01:07:59Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+        // Check for @URL
+        AnnotationInstance url = getFieldAnnotation(field, URL);
+        if (url != null) {
+            constraints.add("must be a valid URL");
+        }
+
+        // Check for @Size
+        AnnotationInstance size = getFieldAnnotation(field, SIZE);
+        if (size != null) {
+            String sizeDesc = buildSizeDescription(size);
+            if (sizeDesc != null) {
+                constraints.add(sizeDesc);
+            }
+        }


buildDescription adds validation group context only for @NotBlank/@NotNull/@NotEmpty, but not for other constraints like @Size, @Pattern, @Min/@Max, @URL, etc. This makes group-scoped constraints for those annotations lose their "on create/update/patch" context and also contradicts the method Javadoc. Consider prefixing every constraint description with getGroupContext(annotation) consistently.

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

mabartos · 2026-03-26T10:24:58Z

Right @mabartos that's why I removed my custom schema property logic for standard Jakarta Validation annotations (@notblank, @notempty, @SiZe, @pattern, @min, @max)

@edewit I don't understand. Did you remove even more custom logic for the annotations? Because I still see these are still handled in the custom code: https://github.com/keycloak/keycloak/pull/47375/changes#diff-b665b37f69099ad25bfc076ec3551293d93f67e7b724a41a7b374fa29b4aedb8R166

Have you checked if could reuse the https://github.com/smallrye/smallrye-open-api/blob/main/core/src/main/java/io/smallrye/openapi/runtime/scanner/dataobject/BeanValidationScanner.java#L32?

Thanks

edewit · 2026-03-26T10:42:57Z

@mabartos yes, like I said I removed it, it no longer sets the constraints (minLength, maxLength, pattern, minimum, etc.) that's done by SmallRye's BeanValidationScanner. But I've kept the human-readable descriptions (e.g., "must not be blank"). That was part of the issue to have this!

vmuzikar · 2026-03-26T11:12:21Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+        AnnotationInstance notBlank = getFieldAnnotation(field, NOT_BLANK);
+        if (notBlank != null) {
+            String context = getGroupContext(notBlank);
+            constraints.add(context + "must not be blank");


We should not be hardcoding the individual validations in the OAS filter, that would not scale well. The logic needs to be universal. I'd propose scanning the annotations and if we detect it's a validation annotation (based on the presence of @Constraint), we'd extract the message field. We'd just need to figure out how to extract the message as it's sometimes just a placeholder like {org.hibernate.validator.constraints.URL.message}.

This is a blocker from my perspective.

vmuzikar · 2026-03-26T11:14:37Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+            DotName.createSimple(CreateClient.class), "on create",
+            DotName.createSimple(PutClient.class), "on update",
+            DotName.createSimple(PatchClient.class), "on patch"


We should consider renaming the groups to not include Client in the names, again to be universal. But something we can do in a follow-up.

vmuzikar · 2026-03-26T11:16:21Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+ *   <li>Adding validation group context (e.g., "on create:", "on update:")</li>
+ * </ul>
+ */
+public class ValidationAnnotationScanner {


I'd prefer this to be included in the OASModelFilter, I don't think it needs a dedicated class, it just wraps some basically static methods, it doesn't hold a state. But that's not blocking, it might be subjective.

kept special processing for constraints that have parameters Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

vmuzikar

@edewit Thanks for the update. It looks better, there are still a few areas that need reworking.

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

vmuzikar · 2026-03-26T13:13:26Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+            AnnotationValue messageValue = annotation.value("message");
+            String messageTemplate = messageValue != null ? messageValue.asString() : getDefaultMessage(annotation.name());


I think we can safely rely the message is alway present. There's not much point in exposing a default message anyway.

no, the message is null it only returns explicitly declared annotation values, not default values

You mean the index doesn't capture the default field values on annotations? That will be certainly a problem, we use that in custom validations too.

it does, just use the method where you pass the index and you will get the defaults

vmuzikar · 2026-03-26T13:17:25Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+        if (simpleName.contains("uuid")) {
+            return "uuid";
+        }
+        if (simpleName.contains("secret")) {
+            return "secret";
+        }


As mentioned above, the filter should be unaware of specific validators if possible. If the validation needs to be on the class level, we could add a new field to that annotation hinting what field in the representation it belongs to. I.e. the mapping should be captured in the annotation, not hardcoded in the filter.

vmuzikar · 2026-03-26T13:21:35Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+     * @param fieldName the name of the field
+     * @param propertySchema the OpenAPI schema to modify
+     */
+    public void applySchemaProperties(ClassInfo classInfo, String fieldName, Schema propertySchema) {


Why do we need this? Isn't description enough?

This is still valid.

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

vmuzikar · 2026-03-27T16:24:53Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+
+            // Handle @Valid (nested validation) - not a constraint but useful to document
+            if (VALID.equals(annotationName)) {
+                constraints.add("nested fields are validated");


All fields are always implicitly validated, we don't need this message.

vmuzikar · 2026-03-27T16:30:38Z

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java

+            String affectedField = annotation.name().withoutPackagePrefix().toLowerCase();
+            if (affectedField != null) {
+                fieldDescriptions.put(affectedField, context + message);
+            }


As hinted in #47375 (comment), we'll probably need to add some dedicated field to the class level validation annotations to let the filter know which field name it belongs to. Class level annotations are typically our custom ones, so it should be fine. Other validations coming from Jakarta/Hibernate are field level, we don't need it there.

add validation to description based on the validation annotations

a2cadd6

fixes: keycloak#47371 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

edewit requested review from a team as code owners March 23, 2026 15:37

keycloak-github-bot bot added the team/cloud-native label Mar 23, 2026

vmuzikar requested review from Copilot, mabartos, michalvavrik, shawkins and vmuzikar March 23, 2026 16:19

Copilot started reviewing on behalf of vmuzikar March 23, 2026 16:20 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

shawkins reviewed Mar 23, 2026

View reviewed changes

...n-v2/rest/src/main/java/org/keycloak/admin/internal/openapi/ValidationAnnotationScanner.java Outdated Show resolved Hide resolved

michalvavrik reviewed Mar 23, 2026

View reviewed changes

edewit added 2 commits March 24, 2026 11:50

added test and better encapsulation

b5b92e8

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Remove applySchemaProperties() from ValidationAnnotationScanner

70af715

SmallRye handles this Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

michalvavrik reviewed Mar 24, 2026

View reviewed changes

michalvavrik approved these changes Mar 25, 2026

View reviewed changes

edewit added 2 commits March 25, 2026 10:41

change to use class instead of string

c8859ca

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

added error when encountering group annotation that isn't in the map

8bfc3c3

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

fixed test and added openapi.yaml from js folder

1720e53

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

edewit requested a review from a team as a code owner March 25, 2026 10:21

keycloak-github-bot bot added the team/ui label Mar 25, 2026

make it spotless

276f042

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

keycloak-github-bot bot added the flaky-test label Mar 25, 2026

keycloak-github-bot bot reviewed Mar 25, 2026

View reviewed changes

vmuzikar mentioned this pull request Mar 25, 2026

Create more validation tests for PUT/PATCH logic #47282

Merged

vmuzikar requested a review from Pepo48 March 25, 2026 17:24

shawkins requested a review from Copilot March 26, 2026 01:02

Copilot started reviewing on behalf of shawkins March 26, 2026 01:03 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

vmuzikar requested changes Mar 26, 2026

View reviewed changes

edewit added 2 commits March 26, 2026 13:27

made the scanner more generic and use the validation message

9e88c37

kept special processing for constraints that have parameters Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

use interpolation for the other annotations

58e9764

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

vmuzikar requested changes Mar 26, 2026

View reviewed changes

updated more PR reviews

7352124

Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

vmuzikar requested changes Mar 27, 2026

View reviewed changes

-        // @NotEmpty implies minItems: 1 for arrays/collections
-        AnnotationInstance notEmpty = getFieldAnnotation(field, NOT_EMPTY);
-        if (notEmpty != null && propertySchema.getMinItems() == null) {
-            propertySchema.setMinItems(1);
+        // @NotEmpty: behavior depends on schema type
+        AnnotationInstance notEmpty = getFieldAnnotation(field, NOT_EMPTY);
+        if (notEmpty != null) {
+            Schema.SchemaType schemaType = propertySchema.getType();
+            if (schemaType == Schema.SchemaType.STRING) {
+                if (propertySchema.getMinLength() == null) {
+                    propertySchema.setMinLength(1);
+                }
+            } else if (schemaType == Schema.SchemaType.ARRAY) {
+                if (propertySchema.getMinItems() == null) {
+                    propertySchema.setMinItems(1);
+                }
+            } else if (schemaType == Schema.SchemaType.OBJECT) {
+                if (propertySchema.getMinProperties() == null) {
+                    propertySchema.setMinProperties(1);
+                }
+            }

		AnnotationValue messageValue = annotation.value("message");
		String messageTemplate = messageValue != null ? messageValue.asString() : getDefaultMessage(annotation.name());

Conversation

edewit commented Mar 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michalvavrik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalvavrik Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalvavrik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabartos commented Mar 25, 2026

Uh oh!

edewit commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot bot commented Mar 25, 2026

Unreported flaky test detected

org.keycloak.testsuite.forms.BruteForceTest#testExceedMaxTemporaryLockouts

Uh oh!

vmuzikar commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mabartos commented Mar 26, 2026

Uh oh!

edewit commented Mar 26, 2026

Uh oh!

vmuzikar Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmuzikar Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

michalvavrik Mar 25, 2026 •

edited

Loading

michalvavrik left a comment •

edited

Loading

edewit commented Mar 25, 2026 •

edited

Loading

vmuzikar Mar 26, 2026 •

edited

Loading

vmuzikar Mar 26, 2026 •

edited

Loading