Querying MVP in Client v2 by Pepo48 · Pull Request #48342 · keycloak/keycloak

Pepo48 · 2026-04-21T20:00:13Z

The query string is parsed using the SCIM filter grammar from scim/core, reusing FilterUtils.parseFilter() and the ANTLR4-generated ScimFilterParserBaseVisitor. This avoids maintaining a separate grammar - the same parse tree that SCIM uses for user/group filtering is now used for client filtering, with a thin ClientQueryEvaluator visitor that evaluates expressions in-memory against BaseClientRepresentation objects.

Supported operators: eq, ne, co, sw, ew, pr with and, or, not, and parenthesized grouping (SCIM gave us this for free, comparison operators gt, ge, lt, le rejected during validation for now, until type handling is designed). extractCompValue() and unescapeJsonString() were extracted to FilterUtils as shared utilities - ScimJPAPredicateEvaluator now delegates to them as well.

Queryable fields are restricted to a whitelist in FieldResolver, which delegates to the projection PR's ClientModelMappers field registry as the single source of truth - unknown fields return 400. This mirrors the JpaClientProviderFactory.searchableAttributes pattern where the set of searchable attributes is explicitly controlled. The only manually maintained entry is auth.method (a dot-notation nested accessor with no direct mapper equivalent).

Filtering happens in-memory after all clients are loaded. The pipeline was reordered to filter before projection (fromModel() -> filter -> applyProjection()), so ?fields=clientId&q=enabled eq true works correctly - the filtered field doesn't need to be in the projection set. The query is validated upfront (QueryParseUtils.validate()) so errors surface before the stream is consumed.

shawkins

This looks like a good start @Pepo48.

I'm hoping there will eventually be re-usable logic with the scim predicate pushdown.

If we want to give clients the ability to directly support the second option from #48286 (comment) we'll need to account for other comparison operators (COLON means equality at this point right?). If not, we'd need pagination control parameters for that.

Do we know that all of field names conform to BAREWORD? I guess we can just make the grammar more json like and allow for the fieldpath to contain quoted strings if needed.

And to confirm, the application of these filters will always just be against the root object in a conjunctive way? There won't for example be a way to filter children. e.g. give me all clients that have a role with description foo vs. give me all clients with only roles that have description foo

shawkins · 2026-04-21T20:28:48Z

+            // TODO .toList() materializes all results in memory
+            return stream
+                    .filter(client -> ClientQueryEvaluator.matches(queryCtx, client))
+                    .toList()
+                    .stream();


Suggested change

// TODO .toList() materializes all results in memory

return stream

.filter(client -> ClientQueryEvaluator.matches(queryCtx, client))

.toList()

.stream();

return stream

.filter(client -> ClientQueryEvaluator.matches(queryCtx, client));

I deliberately kept toList() here because the validation lives in the visitor - following the same pattern used by SCIM and workflow evaluators that keep logic in visitors rather than in separate validation steps. Since the visitor runs inside stream.filter(), without toList() the exception escapes the try-catch and the user gets a misleading "Cannot parse the JSON" instead of "Unknown query field: unknownField". The toList() forces the evaluation so the exception appears.

If we want to remove it, the validation would need to move out of the visitor into a separate pre-validation step. I tried it earlier but decided to keep the pattern consistent.

the user gets a misleading "Cannot parse the JSON"

Relates to #34857 in particular the most recent comments

If we want to remove it, the validation would need to move out of the visitor into a separate pre-validation step. I tried it earlier but decided to keep the pattern consistent.

We definitely want to keep Streaming where possible. The validation doesn't seem necessary against every single element of the stream as you can validate the accessors and match lists upfront - this is typically logic that "compiles" the actual visitor to use for matching.

Ah, that's a different approach from what I tried. I tried to address your suggestion. PTAL. Thanks!

Thanks @Pepo48 that looks good.

Pepo48 · 2026-04-21T22:33:45Z

I'm hoping there will eventually be re-usable logic with the scim predicate pushdown.

Actually, this was the reason, why I studied how core team uses SCIM filter parser to implement it as close as possible. From what I understand, the main advantage of the visitor pattern is that it gives us an opportunity to be as generic by design - in our case the grammar and parse infrastructure are generic. The ClientQueryEvaluator is a visitor, so a second visitor producing JPA predicates (like ScimJPAPredicateEvaluator does) can be added without changing the grammar. Only FieldResolver is client-specific. So the answer is - I hope so too 😄 because that was my goal - all we would need is a new visitor with its own field to column mapping.

COLON means equality at this point right?

Yes, currently it means strict equality. So correct, the comparison operators would be needed for a cursor-based pagination. The proposal mention them under "key differences" compared to GH syntax - we considered it as a possible improvement. I don't see a problem with it - specifically for the comparison operators the grammar could be just extended (without breaking the existing queries).

Do we know that all of field names conform to BAREWORD?

Currently yes, all 12 whitelisted fields.

guess we can just make the grammar more json like and allow for the fieldpath to contain quoted strings if needed.

Sure, the same as the comparison operators.

And to confirm, the application of these filters will always just be against the root object in a conjunctive way? There won't for example be a way to filter children.

Correct and also correct. Per my current understanding SCIM solves this with bracket syntax on the field path, e.g. roles[description eq "Full access"]. Our brackets are on the value side, e.g. roles:[admin,user], not the field side.

vmuzikar · 2026-05-06T15:38:42Z

@Pepo48 Can you please update your PR now that #48460 was merged?

vmuzikar · 2026-05-06T15:46:43Z

@Pepo48 Also, can you please re-link the PR to #48765.

Pepo48 · 2026-05-11T18:56:24Z

@shawkins @vmuzikar the PR is rebased. As we discussed previously, I removed 12 hardcoded mappings from FieldResolver, as it was duplicating projection's BaseClientModelMapper. FieldResolver is a thin adapter that delegates to ClientModelMappers. The only thing that stayed in FieldResolver is auth.method. I considered to add it as a read-only mapping in OIDCClientModelMapper (with null setters, like protocol), but since the dot-notation is a query grammar concept, not a mapper concept, I guess it's fine not to move it. Also, I assume it will be up to the #48604 spike to decide the approach.

Thanks!

Pepo48 · 2026-05-15T18:53:47Z

@shawkins @vmuzikar jfyi, I tried to adopt SCIM syntax, see the updated PR description.

Compared to the initial MVP I could remove the custom ANTLR4 grammar (lexer, parser, error listener) and all the code that it was supported by. We also lost list/map bracket syntax, but that's expected and acceptable trade-off, right? So we will use standard SCIM expressions with and, e.g. roles eq "admin" and roles eq "user" instead of roles eq [admin,user].

shawkins

LGTM, just a couple of things of notes.

shawkins · 2026-05-16T17:44:22Z

+    private static void validateExpression(ScimFilterParser.ExpressionContext ctx) {
+        if (ctx.OR() != null) {
+            validateExpression(ctx.expression());
+            validateAndExpression(ctx.andExpression());


nit - here and in the validate method below, the if/else share a common validate statement that could be moved below the if/else.

Thanks @shawkins, addressed. Thanks to that I also noticed that isKnownField guard in FieldResolver.resolve is a dead code (validation in QueryParseUtils.validate already rejects unknown fields) so I cleaned that up as well.

shawkins · 2026-05-16T17:50:18Z

                    .filter(client -> canView || permissions.clients().canView(client))
                    .filter(client -> client.getProtocol() != null)
-                    .map(client -> getMapper(client.getProtocol()).fromModel(client, projectionOptions.getFields()))
+                    .map(client -> getMapper(client.getProtocol()).fromModel(client))


We are focused on the contract, not the implementation, so this is not something that needs addressed - but I wanted to mention that fully mapping, then preforming the projection on top of that will cause all representations to be fully materialized.

With the filtering pushed further down, either to the Model layer or all the way to JPA, the projection will work more as expected.

shawkins · 2026-05-21T12:34:50Z

Still looks good, just a conflict to resolve.

Pepo48 · 2026-05-22T16:50:32Z

Conflicts resolved and rebased.

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

Pepo48 · 2026-05-25T11:07:41Z

Another set of conflicts resolved and the PR is rebased. I also fixed the integration test that was failing due to the recent changes that made the validation stricter.

michalvavrik · 2026-05-25T10:37:48Z

@@ -16,7 +16,15 @@
 public interface ClientService extends Service {

    class ClientSearchOptions {


looks like a record candidate

Clearly it does. Addressed. Thanks!

michalvavrik · 2026-05-25T10:47:24Z

+        }
+
+        String operator = ctx.compareOp().getText().toLowerCase();
+        String queryValue = FilterUtils.extractCompValue(ctx.compValue());


queryValue can be null, so when users does "contains", "startsWith" or "endsWith" null, it will result in NPE In matchScalar, because java.lang.String#contains, java.lang.String#startsWith(java.lang.String) and java.lang.String#endsWith require non null parameter. I makes no sense to start/end/contain null, but I can imagine that some programmatically generated expression can do description sw null by mistake. Maybe you can guard against NPE and throw something like IllegalArgumentException?

Turns out to be deeper - the NPE exists in ScimJPAPredicateProvider too, so I fixed it at the source. FilterUtils.parseFilter() now rejects null for operators other than eq/ne ("contains null" or "starts with null" is imo meaningless, pr is the right way to test presence).

Can you double check with the spec on whether it uses three valued boolean logic like SQL?

For filtered attributes that are not part of a particular resource type, the service provider SHALL treat the attribute as if there is no attribute value. For example, a presence or equality filter for an undefined attribute evaluates to false.

Which I read as no "unknown" state compared to SQL, although the "For example" claim only covers pr and eq. Spec does not specify how ne behaves on absent attributes (at least I don't see it).

Right, it's not very specific.

For filtered attributes that are not part of a particular resource type

From the current implementation this is interpretted as an attribute that does not exist for the given schemas - ScimJPAPredicateProvider.createPredicate

As far as I can see from our current SCIM implementation, we're handing predicates (potentially with explicit nulls) directly mapped to JPA. So my expectation is that three valued boolean logic will be used underneath on attributes that are defined for the schema, but may be null.

Right, just for the record, we have two distinct cases:

undefined attribute - resolve() returns null -> cb.disjunction(), always false per RFC 7644.

defined but null-valued - predicates go straight to SQL. Both ne "x" and not (eq "x") hit three-valued logic on a NULL column and exclude the row.

This MVP in-memory evaluator returns true in case 2 (a missing value isn't equal to anything). This is another task for JPA filtering follow-up (#48766), as we may need explicit IS NULL handling if we want to preserve that.

michalvavrik · 2026-05-25T11:09:27Z

+
+        return switch (operator) {
+            case "eq" -> matchScalar(fieldValue, queryValue, Objects::equals);
+            case "ne" -> !matchScalar(fieldValue, queryValue, Objects::equals);


frankly, this was found by Claude but I looked into it and it seems relevant:

Consider situation when field like description is null. Then not description eq "something" is true and description ne "something" is false.

I have tested it:

cat <<'PATCH' | git apply - && mvn -pl rest/admin-v2/services test -Dtest=ClientQueryEvaluatorTest#neAndNotEqInconsistencyOnNullField diff --git a/rest/admin-v2/services/src/test/java/org/keycloak/services/client/query/ClientQueryEvaluatorTest.java b/rest/admin-v2/services/src/test/java/org/keycloak/services/client/query/ClientQueryEvaluatorTest.java index 14abb744b8..a1b3083196 100644 --- a/rest/admin-v2/services/src/test/java/org/keycloak/services/client/query/ClientQueryEvaluatorTest.java +++ b/rest/admin-v2/services/src/test/java/org/keycloak/services/client/query/ClientQueryEvaluatorTest.java @@ -241,6 +241,23 @@ class ClientQueryEvaluatorTest { assertFalse(matches("not (description eq \"hello\")", client)); } + @Test + void neAndNotEqInconsistencyOnNullField() { + var client = createClient("test", true); + client.setDescription(null); + + assertTrue(matches("not description eq \"something\"", client)); + + assertFalse(matches("description ne \"something\"", client)); + } + private static boolean matches(String query, OIDCClientRepresentation client) { var filterCtx = QueryParseUtils.parse(query); return ClientQueryEvaluator.matches(filterCtx, client); PATCH

and it seems that unlike in boolean algebra, not is applied on the (description eq "something") rather than on description. https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2.2 see valFilter

Thanks, fixed. The early return false for null fields fired before the operator was checked, so ne could never return true. Now eq/ne are handled before the null guard.

michalvavrik · 2026-05-25T11:15:04Z

+    }
+
+    @Test
+    void caseSensitiveMatch() {


https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2.2 says says that whether equality is case sensitive or not is determined by caseExact which is false by default https://datatracker.ietf.org/doc/html/rfc7643#section-2.2. Should we clarify what is expected to users?

I think the JPA follow-up (#48766) can address this - ScimJPAPredicateProvider already implements the pattern via attrInfo.isCaseExact() + cb.lower(). The current in-memory evaluator doesn't have Attribute schema metadata based on which the decision could be made, therefore I opted for case-sensitive matching for all fields for the needs of MVP.

I think the JPA follow-up (#48766) can address this - ScimJPAPredicateProvider already implements the pattern via attrInfo.isCaseExact() + cb.lower(). The current in-memory evaluator doesn't have Attribute schema metadata based on which the decision could be made, therefore I opted for case-sensitive matching for all fields for the needs of MVP.

understood

michalvavrik · 2026-05-25T11:18:34Z

+
+    protected Stream<BaseClientRepresentation> applyProjection(Stream<BaseClientRepresentation> stream, ClientProjectionOptions projectionOptions) {
+        if (projectionOptions.getFields().isEmpty()) return stream;
+        return stream.peek(rep -> MAPPERS.applyProjection(rep, projectionOptions.getFields()));


10BC8
I thought peek should be used mostly for debugging. as you are mutating rep, I'd prefer map as I don't expect peek to have sideeffects on rep. Keep it if you like...

Tbh, peek is used here because applyProjection returns void, but as I am reading through JDK, I see what you're saying - docs say peek exists mainly for debugging and stream optimizations can skip it entirely. I used map instead, altough the cleanest fix would likely be changing applyProjection to return the rep (in both BaseClientModelMapper and ClientModelMappers) so we can use map idiomatically.

I don't have a preference, happy to do whatever others prefer here.

IMO map is fine, thanks

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

michalvavrik

LGTM

shawkins

LGTM, seems like we have a handle now on what to expect when we align with the scim JPA logic.

vmuzikar

@Pepo48 Sorry for the late review. Overall looks good.

vmuzikar · 2026-06-01T15:25:16Z

+    public static String unescapeJsonString(String s) {
+        return s.replace("\\\"", "\"")
+                .replace("\\\\", "\\")
+                .replace("\\/", "/")
+                .replace("\\b", "\b")
+                .replace("\\f", "\f")
+                .replace("\\n", "\n")
+                .replace("\\r", "\r")
+                .replace("\\t", "\t");
+    }


This is not correct because replace always runs on the full string. E.g. an input like:

\\n

would be decoded as a new line instead of literal \n string. That is because the first pass replaces \\ with \ with second pass replacing \n with a new line.

This should probably be handled in a separate issue given that it's existing scim logic.

From what I see the bug is valid, but as Steve said, the method was just extracted in order to make it reusable -I haven't changed the logic outside of the rest module. Actually, this made me realize that the same is used also in ScimFilterToJsonNodeConverter, so I replaced it even there.

But the rest is for the follow-up, I guess. I can file an issue.

Yes, please create a follow-up issue.

vmuzikar · 2026-06-01T15:30:13Z

+        assertTrue(matches("loginFlows eq \"STANDARD\"", client));
+        assertTrue(matches("loginFlows eq \"STANDARD\" and loginFlows eq \"DIRECT_GRANT\"", client));


This seems rather like "contains" syntax than "equals". Is there a way to define whole collection value, e.g. I want to find a client with only STANDARD flow.

I see what you mean, but that's how SCIM works per RFC 7644 3.4.2.2:

If the specified attribute in a filter expression is a multi-valued attribute, the filter matches if any of the values of the specified attribute match the specified criterion; e.g., if a User has multiple "emails" values, only one has to match for the entire User to match.

And no, there's no way to express exact-collection equality in SCIM filter syntax. Back when I was researching this I also came across an extension draft that was acknowledging this exact gap in SCIM, but based on their tracker, this was never implemented.

@Pepo48 Thanks. Let's discuss on the team meeting today. This is definitely a downside and potential candidate for diverging from strict SCIM spec for Admin v2 (we knew we might have some instances of use cases like that). But in any case, that's a follow-up.

@vmuzikar @Pepo48 Based upon the current scim implementation, a multi-valued field predicate "works" as expected by the scim spec iff it's formed by a join denormalization. So for example if you query role eq "name", then you see all clients that have role named name.

If you are dealing with an underlying field that contains a multi-valued contruct - the generated JPA query will fail as the types will be incompatible.

Or, as in this case, if the multi-valued field is created via projection logic - then that would fail as well (or could possibly be done in memory).

More than likely only the first case has come up with the current scim.

I agree that having a more semantically clear way of doing this would be good.

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

vmuzikar

@Pepo48 LGTM, nice work, thanks.

* Initial implementation of Querying for Client v2 Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com> * Initial implementation of Querying for Client v2 Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com> * Initial implementation of Querying for Client v2 Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com> * Initial implementation of Querying for Client v2 Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com> * Initial implementation of Querying for Client v2 Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com> --------- Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

shawkins reviewed Apr 21, 2026

View reviewed changes

Pepo48 force-pushed the issue-48294 branch 2 times, most recently from eca431b to fbedfce Compare May 11, 2026 18:19

Pepo48 force-pushed the issue-48294 branch 2 times, most recently from d24dd9d to ee97246 Compare May 15, 2026 18:36

Pepo48 marked this pull request as ready for review May 15, 2026 21:08

Pepo48 requested review from a team as code owners May 15, 2026 21:08

keycloak-github-bot Bot added team/ui team/cloud-native labels May 15, 2026

shawkins previously approved these changes May 16, 2026

View reviewed changes

Pepo48 dismissed shawkins’s stale review via 520b772 May 18, 2026 14:47

Pepo48 force-pushed the issue-48294 branch from 520b772 to acd1e0e Compare May 18, 2026 14:53

stianst removed the team/ui label May 21, 2026

Pepo48 force-pushed the issue-48294 branch from acd1e0e to 9d8dada Compare May 22, 2026 16:49

Pepo48 added 3 commits May 25, 2026 12:31

Initial implementation of Querying for Client v2

9d3dd15

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

Initial implementation of Querying for Client v2

46c98a5

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

Initial implementation of Querying for Client v2

237d1a3

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

Pepo48 force-pushed the issue-48294 branch from 9d8dada to 237d1a3 Compare May 25, 2026 11:03

Pepo48 requested review from edewit, michalvavrik and vmuzikar May 25, 2026 11:14

michalvavrik reviewed May 25, 2026

View reviewed changes

Initial implementation of Querying for Client v2

0525f79

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

Pepo48 force-pushed the issue-48294 branch from bb17fab to 0525f79 Compare May 25, 2026 13:18

michalvavrik approved these changes May 25, 2026

View reviewed changes

shawkins previously approved these changes May 27, 2026

View reviewed changes

vmuzikar requested changes Jun 1, 2026

View reviewed changes

Initial implementation of Querying for Client v2
10BC8

3c74953

Closes keycloak#48765 Signed-off-by: Peter Zaoral <pzaoral@redhat.com>

Pepo48 dismissed shawkins’s stale review via 3c74953 June 1, 2026 19:42

vmuzikar approved these changes Jun 2, 2026

View reviewed changes

vmuzikar enabled auto-merge (squash) June 2, 2026 07:19

vmuzikar merged commit 2844fb4 into keycloak:main Jun 2, 2026
148 of 214 checks passed

Pepo48 mentioned this pull request Jun 2, 2026

FilterUtils.unescapeJsonString() incorrectly decodes escaped backslash sequences #49571

Closed

1 of 2 tasks

		@@ -16,7 +16,15 @@
		public interface ClientService extends Service {

		class ClientSearchOptions {

		assertTrue(matches("loginFlows eq \"STANDARD\"", client));
		assertTrue(matches("loginFlows eq \"STANDARD\" and loginFlows eq \"DIRECT_GRANT\"", client));

Conversation

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!