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

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 scann 8000 er into OpenAPI model filtering, combining field-level and class-level validations into property descriptions and applying schema constraints.

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

+        // @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);

}

}

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

+        // 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

+        // @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).

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.

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

+    }
+
+    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

+    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.

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.

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)

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.

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

+        // 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.

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!

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

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>

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

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>

Jandex returns null for annotation values that use the default, it only returns values that were explicitly specified. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

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

Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>

keycloak-github-bot · 2026-04-29T12:57:05Z

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.MultipleTabsLoginTest#testEmptyBaseUrl

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: Expected LoginPage but was localhost (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?scope=openid&response_type=code&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&client_id=empty-baseurl-client) ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

…ycloak#47375) * add validation to description based on the validation annotations fixes: keycloak#47371 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * added test and better encapsulation Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Remove applySchemaProperties() from ValidationAnnotationScanner SmallRye handles this Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * change to use class instead of string Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * added error when encountering group annotation that isn't in the map Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fixed test and added openapi.yaml from js folder Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * make it spotless Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * made the scanner more generic and use the validation message 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 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * updated more PR reviews Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * more PR review comments Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fixed merge Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * moved class level validator descriptions Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * added field name to annotation, that we can then use in the schema Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * don't put validations that have an affectedField on the class Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * jandex default Jandex returns null for annotation values that use the default, it only returns values that were explicitly specified. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fixed test Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * even more PR review comments Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * small fix to get more validations in the description Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * better handle on parmeterised types Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Update generated spec Signed-off-by: Václav Muzikář <vmuzikar@ibm.com> --------- Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> Signed-off-by: Václav Muzikář <vmuzikar@ibm.com> Co-authored-by: Václav Muzikář <vmuzikar@ibm.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

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

michalvavrik reviewed Mar 23, 2026

View reviewed changes

michalvavrik reviewed Mar 24, 2026

View reviewed changes

michalvavrik approved these changes Mar 25, 2026

View reviewed changes

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

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

edewit and others added 21 commits April 29, 2026 13:56

add validation to description based on the validation annotations

56a45aa

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

added test and better encapsulation

21640a8

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

Remove applySchemaProperties() from ValidationAnnotationScanner

2039fc9

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

change to use class instead of string

2dd694f

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

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

39f88c0

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

fixed test and added openapi.yaml from js folder

44f3c81

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

make it spotless

546284c

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

made the scanner more generic and use the validation message

46f035e

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

2a08d9d

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

updated more PR reviews

12da2ef

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

more PR review comments

bc80408

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

fixed merge

b96a0b2

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

moved class level validator descriptions

0433e99

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

added field name to annotation, that we can then use in the schema

b8c18e2

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

don't put validations that have an affectedField on the class

dfe5976

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

jandex default

3e6cc2d

Jandex returns null for annotation values that use the default, it only returns values that were explicitly specified. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

fixed test

4a50426

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

even more PR review comments

c637b1e

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

small fix to get more validations in the description

199b2ee

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

better handle on parmeterised types

ec3ef97

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

Update generated spec

7ac6531

Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>

vmuzikar force-pushed the issue-47371 branch from 0f89295 to 7ac6531 Compare April 29, 2026 11:56

vmuzikar merged commit ccacca5 into keycloak:main Apr 29, 2026
90 checks passed

This was referenced Apr 29, 2026

10BC8 Flaky test: org.keycloak.testsuite.model.singleUseObject.SingleUseObjectModelTest#testCluster #48441

Closed

Flaky test: org.keycloak.testsuite.model.user.UserModelTest#testAddRemoveUserConcurrent #48442

Closed

keycloak-github-bot Bot reviewed Apr 29, 2026

View reviewed changes

stianst mentioned this pull request Jun 1, 2026

js deps uuid 26.6 #49539

Closed

edewit deleted the issue-47371 branch June 11, 2026 09:03

-        // @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);
+                }
+            }

Conversation

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.forms.BruteForceTest#testExceedMaxTemporaryLockouts

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.forms.MultipleTabsLoginTest#testEmptyBaseUrl

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants