Repository navigation
Admin API v2 Projection - #48460
Admin API v2 Projection#48460
Conversation
|
To follow-up from today's discussion of possible directions from here:
The initial thinking is to do 1, which I will continue to refine this pr based upon. As for the greater implications of making this more generic, I'm not sure either is ideal. What we're doing here is more akin to kubernetes conversion webhooks, not crds. The more ideosyncratic our endpoints / options are and the storage layer is, the less this looks like a generic exercise. If / until we have an extremely regular paradigm for the storage layer logic, I wouldn't want to consider doing things like metadata driving via code generation the creation of the field mappings or the creation of an adapter classes. The former really starts to look like an extension to mapstruct.org which we tried to use initially. An after thought is that we could use sundrio to create simple impls for model interfaces in places where we need the "old" representation rather than creating temporary Clients: |
|
After trying a little bit more to see what 2 would look like, I made the next set of refinements to 1. The clients list endpoint now accepts a list of string fields and will only project what is in that list - with the caveat the protocol field which currently cannot be omitted. We will need to discuss / create a follow-up issue to determine how we should handle that and any other fixed fields we may introduce. I didn't worry about how the logic will apply to types beyond clients just yet as we'll address that later. ClientModelMappers exposes an isKnownField that can be used for validation with search and sort. We'll then likely want to expose the MappedField.getValue method from the specific BaseClientModelMapper to actually implement the comparisons. Polymorphism could end up causing some issues with field handling - a given field name could be different types in subclasses. |
0a05d8e to
678b397
Compare
Unreported flaky test detectedIf 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.RPInitiatedFrontChannelLogoutTest#testFrontChannelLogoutWithoutSessionRequiredKeycloak CI - Forms IT (firefox) |
Unreported flaky test detectedIf 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.LoginTest#loginDisabledUserKeycloak CI - Forms IT (chrome) |
There was a problem hiding this comment.
Looks fine to me. It is PR like this where I regret most that we don't have meta-programming model, e.g. we can't leverage Quarkus build steps, as this manual mapping of properties to their setters, getters and default value. It would be better to just have this declarative (like annotations). But I prefer doing it your way over the reflection. I trust we will never forget to add the attribute mapping because every attribute we add in the future will be tested.
| @@ -169,6 +169,17 @@ paths: | |||
| operationId: getClients | |||
| tags: | |||
| - Clients (v2) | |||
| parameters: | |||
| - description: "Set of fields to include in the response. Must be top-level\ | |||
There was a problem hiding this comment.
I think the description should explain how do I select nested object fields. What if I want the auth.method but not auth.secret?
There was a problem hiding this comment.
Top-level here should mean just direct children. It's not possible in this PR to specify field.nested.
There was a problem hiding this comment.
I missed the top-level. I think I concentrated on purpose of \. My bad.
| @@ -82,7 +75,7 @@ private void setModelFromFlows(Set<OIDCClientRepresentation.Flow> flows, ClientM | |||
|
|
|||
| private Set<String> getServiceAccountRoles(ClientModel client) { | |||
| if (client.isServiceAccountsEnabled()) { | |||
| var serviceAccount = session.users().getServiceAccount(client); | |||
| var serviceAccount = KeycloakSessionUtil.getKeycloakSession().users().getServiceAccount(client); | |||
There was a problem hiding this comment.
When reviewing this change, it really surprised me that Keycloak trusts ThreadLocal, rather than propagation session via storage based on what is available (Arc or some test storage based on thread local). But it appears that all endpoints are blocking and as long as Keycloak never does reactive / async code (I found the health checks, but they don't need session), there isn't any functional issue.
There was a problem hiding this comment.
We have filters that keep the request scoped session bean the same as what is in the threadlocal. Just like with our main services module we don't have a direct dependency on quarkus so that we can directly utilize Arc.
But it appears that all endpoints are blocking and as long as Keycloak never does reactive / async code (I found the health checks, but they don't need session), there isn't any functional issue.
Correct, the assumption here is that we're always dealing with a blocking call. See a recent issue with our assumptions about transactions / session handling that has come up with non-blocking work https://github.com/keycloak/keycloak/pull/48471/changes#diff-a7df5c95700a743072dd7321dd8a6df9e71cb775e2490afb0e52b04ee9c8f758R38
There was a problem hiding this comment.
This wasn't introduced by this PR, but since this PR is called "Projection" and you modified the ClientProjectionOptions -> this method never uses its parameter projectOptions. It is weird that we have it as part of the API, probably for the future. Maybe for now we should throw illegal param if it is not null or something.
There was a problem hiding this comment.
I wanted to add it into the list endpoint first #48292 (comment) then if that all looks good, add it to the single resource - or determine if that's even really needed.
|
|
||
| public class ClientModelMappers { | ||
|
|
||
| private Map<String, BaseClientModelMapper<?>> mappers = new HashMap<>(); |
There was a problem hiding this comment.
| private Map<String, BaseClientModelMapper<?>> mappers = new HashMap<>(); | |
| private final Map<String, BaseClientModelMapper<?>> mappers = new HashMap<>(); |
but quite honestly, I think it should be Map.of(...) to avoid extra allocation for long lived map. Feel free to not do it as it is just nitpick...
| @@ -35,7 +38,7 @@ public interface ClientsApi { | |||
| @APIResponses(value = { | |||
| @APIResponse(responseCode = "200", content = @Content(schema = @Schema(type = SchemaType.ARRAY, implementation = BaseClientRepresentation.class))) | |||
| }) | |||
| Stream<BaseClientRepresentation> getClients(); | |||
| Stream<BaseClientRepresentation> getClients(@Parameter(description = "Set of fields to include in the response. Must be top-level fields on one of the client types. If omitted or empty, all fields will be populated.") @QueryParam("fields") Set<String> fields); | |||
There was a problem hiding this comment.
IIUC protocol is always present in the response regardless of the fields. I think it should be documented so that noone can report bug. I know why it needs to be there (serialization on the client side) but users also use curl. They will see it.
|
I think all review comments have been address and I added another test, and another follow-up #48650 |
|
Updated the pr:
|
closes: keycloak#48734 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: keycloak#48734 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes #48734
@Pepo48 @vmuzikar I appologize for the rough state, but I wanted to get some feedback on direction.
The changes in BeanIntrospection give us the ability to reason over the Jackson fields, which we can use in several ways.
The changes in the ObjectMapperResolver show how we can do projection directly on the output document. Unlike my comment on the issue, it doesn't appear easy to handle anything other than the top-level fields, so I'm not sure that we currently need this - but it does give us full control over what fields are emitted even those with fixed fields, such as protocol.
The model mappers I would like demote from being services as they are going to undergo more refinement, and they will likely end up being an implemenation detail of the entire ClientService layer, rather than functionality that is directly usable.
With a change in the mapping logic to be based upon individual field mappings, we can easily omit fields from being read from the model (which as mentioned on the issue is typically good enough to omit join fetching).
The tie in to the BeanIntrospection here is that I've made the mappers become the place that understands the metamodel of our representations - we can reason over them if need. e.g. if there is a MappedProperty without a repSetter, then it's something we can't filter via the model mapper and instead need via jackson. I'm still capturing the getter / setter from the representations though as that gives us better type checking and the Jackson logic would use reflection instead.
These mappings are reusable by the top-level querying logic in the pr from @Pepo48. We may need to expand the mapping to capture whether, or what, value converstion is happening so that we can push predicates further down.