Repository navigation
Querying MVP in Client v2 #48342
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
Querying MVP in Client v2 #48342
Changes from all commits
9d3dd15
46c98a5
237d1a3
0525f79
3c74953
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| package org.keycloak.services.client.query; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.Objects; | ||
|
|
||
| import org.keycloak.representations.admin.v2.BaseClientRepresentation; | ||
| import org.keycloak.scim.filter.FilterUtils; | ||
| import org.keycloak.scim.filter.ScimFilterParser; | ||
| import org.keycloak.scim.filter.ScimFilterParserBaseVisitor; | ||
|
|
||
| public class ClientQueryEvaluator extends ScimFilterParserBaseVisitor<Boolean> { | ||
|
|
||
| private final BaseClientRepresentation client; | ||
|
|
||
| private ClientQueryEvaluator(BaseClientRepresentation client) { | ||
| this.client = client; | ||
| } | ||
|
|
||
| public static boolean matches(ScimFilterParser.FilterContext filter, BaseClientRepresentation client) { | ||
| return new ClientQueryEvaluator(client).visit(filter); | ||
| } | ||
|
|
||
| @Override | ||
| public Boolean visitFilter(ScimFilterParser.FilterContext ctx) { | ||
| return visit(ctx.expression()); | ||
| } | ||
|
|
||
| @Override | ||
| public Boolean visitExpression(ScimFilterParser.ExpressionContext ctx) { | ||
| if (ctx.OR() != null) { | ||
| return visit(ctx.expression()) || visit(ctx.andExpression()); | ||
| } | ||
| return visit(ctx.andExpression()); | ||
| } | ||
|
|
||
| @Override | ||
| public Boolean visitAndExpression(ScimFilterParser.AndExpressionContext ctx) { | ||
| if (ctx.AND() != null) { | ||
| return visit(ctx.andExpression()) && visit(ctx.notExpression()); | ||
| } | ||
| return visit(ctx.notExpression()); | ||
| } | ||
|
|
||
| @Override | ||
| public Boolean visitNotExpression(ScimFilterParser.NotExpressionContext ctx) { | ||
| if (ctx.NOT() != null) { | ||
| return !visit(ctx.notExpression()); | ||
| } | ||
| return visit(ctx.atom()); | ||
| } | ||
|
|
||
| @Override | ||
| public Boolean visitAtom(ScimFilterParser.AtomContext ctx) { | ||
| if (ctx.attributeExpression() != null) { | ||
| return visit(ctx.attributeExpression()); | ||
| } | ||
| return visit(ctx.expression()); | ||
| } | ||
|
|
||
| @Override | ||
| public Boolean visitPresentExpression(ScimFilterParser.PresentExpressionContext ctx) { | ||
| String fieldPath = ctx.ATTRPATH().getText(); | ||
| return FieldResolver.resolve(fieldPath, client) != null; | ||
| } | ||
|
|
||
| @Override | ||
| public Boolean visitComparisonExpression(ScimFilterParser.ComparisonExpressionContext ctx) { | ||
| String fieldPath = ctx.ATTRPATH().getText(); | ||
| Object fieldValue = FieldResolver.resolve(fieldPath, client); | ||
| String operator = ctx.compareOp().getText().toLowerCase(); | ||
| String queryValue = FilterUtils.extractCompValue(ctx.compValue()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out to be deeper - the NPE exists in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you double check with the spec on whether it uses three valued boolean logic like SQL?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which I read as no "unknown" state compared to SQL, although the "For example" claim only covers
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, it's not very specific.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, just for the record, we have two distinct cases:
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. |
||
|
|
||
| return switch (operator) { | ||
| case "eq" -> fieldValue == null ? queryValue == null : matchScalar(fieldValue, queryValue, Objects::equals); | ||
| case "ne" -> fieldValue == null ? queryValue != null : !matchScalar(fieldValue, queryValue, Objects::equals); | ||
| case "co" -> fieldValue != null && matchScalar(fieldValue, queryValue, (fv, qv) -> fv != null && fv.contains(qv)); | ||
| case "sw" -> fieldValue != null && matchScalar(fieldValue, queryValue, (fv, qv) -> fv != null && fv.startsWith(qv)); | ||
| case "ew" -> fieldValue != null && matchScalar(fieldValue, queryValue, (fv, qv) -> fv != null && fv.endsWith(qv)); | ||
| default -> false; | ||
| }; | ||
| } | ||
|
|
||
| private boolean matchScalar(Object fieldValue, String queryValue, | ||
| java.util.function.BiPredicate<String, String> predicate) { | ||
| if (fieldValue instanceof Collection<?> collection) { | ||
| return collection.stream() | ||
| .anyMatch(item -> predicate.test(item.toString(), queryValue)); | ||
| } | ||
| return predicate.test(fieldValue.toString(), queryValue); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package org.keycloak.services.client.query; | ||
|
|
||
| public class ClientQueryException extends RuntimeException { | ||
|
|
||
| public ClientQueryException(String message) { | ||
| super(message); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package org.keycloak.services.client.query; | ||
|
|
||
| import org.keycloak.models.mapper.ClientModelMappers; | ||
| import org.keycloak.representations.admin.v2.BaseClientRepresentation; | ||
| import org.keycloak.representations.admin.v2.OIDCClientRepresentation; | ||
|
|
||
| public class FieldResolver { | ||
|
|
||
| private static final ClientModelMappers MAPPERS = new ClientModelMappers(); | ||
|
|
||
| public static boolean isKnownField(String fieldPath) { | ||
| if ("auth.method".equals(fieldPath)) { | ||
| return true; | ||
| } | ||
| return MAPPERS.isKnownField(fieldPath); | ||
| } | ||
|
|
||
| public static Object resolve(String fieldPath, BaseClientRepresentation client) { | ||
| if ("auth.method".equals(fieldPath)) { | ||
| if (client instanceof OIDCClientRepresentation oidc && oidc.getAuth() != null) { | ||
| return oidc.getAuth().getMethod(); | ||
| } | ||
| return null; | ||
| } | ||
| return MAPPERS.resolveFieldValue(fieldPath, client); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.