Repository navigation
45364 Migrate to adminClient for Java #46973
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
Changes from all commits
4bce678
32b226b
0b28cff
5273289
5305559
af0e1a6
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,55 @@ | ||||||
| package org.keycloak.admin.client.deserializer; | ||||||
|
|
||||||
| import java.io.IOException; | ||||||
| import java.util.List; | ||||||
| import java.util.stream.Stream; | ||||||
|
|
||||||
| import com.fasterxml.jackson.core.JsonParser; | ||||||
| import com.fasterxml.jackson.databind.BeanProperty; | ||||||
| import com.fasterxml.jackson.databind.DeserializationContext; | ||||||
| import com.fasterxml.jackson.databind.JavaType; | ||||||
| import com.fasterxml.jackson.databind.JsonDeserializer; | ||||||
| import com.fasterxml.jackson.databind.deser.ContextualDeserializer; | ||||||
|
|
||||||
| public class StreamDeserializer extends JsonDeserializer<Stream<?>> implements ContextualDeserializer { | ||||||
|
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. @shawkins Do you want to review too, especially the deserializer?
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. I'm trying to understand this better. On the server side we are registering a StreamSerializer ( keycloak/services/src/main/java/org/keycloak/services/util/ObjectMapperResolver.java Line 52 in fa3e964
From the discussion in #46973 (comment) it would be good to understand exactly what quarkus is doing with the Stream (is it creating a full response buffer for error handling purposes?). I know in some places we have resorted to using Response, and in particular the response stream directly for performance reasons keycloak/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java Line 1395 in aba0b71
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. I assume Quarkus is handling as the payload is looking like a list. I think there are two different things that are important:
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.
Yes, can confirm the StreamSerializer just writes an array. From a server side perspective I confirmed that is happening in a streaming manner as the serialization is directly into the output stream.
When we are returning a "list" the interface will be one of List (array), Stream, or Multi. Unless I'm missing something, the response body would be identical no matter the type. The client proxy logic only knows about List by default, but we don't want to use that when the server logic is actually streaming. Based upon what you have seen here, the use of Stream or Multi will require a custom deserializer. In more exotic scenarios we'll return a Response. I haven't checked how a RestEasy proxy handles a raw Response, but that would at least require additional client side logic to consume. Switching to an InputStream would not good either as that negates the server side simplicity of directly returning the objects.
We'd need async parsing support as well. The only simple solutions here seem to be:
Another thought is that successful response codes are basically not available via the proxy (or can they be obtained somehow?). Our PUT can return differentiated codes (created vs updated), so you won't know which occurred.
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.
That's something we did with v1. If possible, I'd like to avoid doing that again (especially if not generated but maintained manually). I'm quite surprised resteasy doesn't support a basic stream deserializer OOTB. So we'll need to add custom one but it's not ideal of course.
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. Ah, so that the concern about alternative success codes as well - any time we have to return one we'll be using a response. So then we'll just need some helper logic for consuming Responses. Something we need to do in a follow-up is add APIResponses to everything that is returning a Response: The client side handling of a raw Response is still not ideal. There is a resteasy reactive RestResponse construct that is typed - are we allowed to use that or do we need to stick to plain jax-rs? I also haven't check yet how that is handled on the client side. Alternatively if we can't use that, we could consider introducing a TypedResponse class that extends Response (which may not be allowable) and delegates to an actual response, then wrap the target proxy with another proxy that detects when a method is called that returns a TypedResponse and does the conversion there such that getEntity returns the expected type.
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.
Captured as #47365.
+1
AFAIK we need to be as platform/lib agnostic as possible in our endpoints.
That would sound good. But we could do that as a follow-up.
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.
Captured as #47367. |
||||||
|
|
||||||
| private final JavaType contentType; | ||||||
|
|
||||||
| public StreamDeserializer() { | ||||||
| this(null); | ||||||
| } | ||||||
|
|
||||||
| public StreamDeserializer(JavaType contentType) { | ||||||
| this.contentType = contentType; | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public Stream<?> deserialize(JsonParser p, DeserializationContext ctxt) throws IOException { | ||||||
| JavaType streamType = ctxt.getContextualType(); | ||||||
| JavaType actualContentType = contentType; | ||||||
|
|
||||||
| if (actualContentType == null && streamType != null && streamType.containedTypeCount() == 1) { | ||||||
| actualContentType = streamType.containedType(0); | ||||||
| } | ||||||
|
|
||||||
| if (actualContentType == null) { | ||||||
| actualContentType = ctxt.getTypeFactory().constructType(Object.class); | ||||||
| } | ||||||
|
|
||||||
| JavaType listType = ctxt.getTypeFactory().constructCollectionType(List.class, actualContentType); | ||||||
| List<?> list = ctxt.readValue(p, listType); | ||||||
| return list.stream(); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public JsonDeserializer<?> createContextual(DeserializationContext ctxt, BeanProperty property) { | ||||||
| JavaType contextualType = ctxt.getContextualType(); | ||||||
| JavaType actualContentType = null; | ||||||
|
|
||||||
| if (contextualType != null && contextualType.containedTypeCount() == 1) { | ||||||
| actualContentType = contextualType.containedType(0); | ||||||
| } | ||||||
|
|
||||||
| return new StreamDeserializer(actualContentType); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package org.keycloak.admin.client.wrapper; | ||
|
|
||
| import jakarta.ws.rs.client.WebTarget; | ||
|
|
||
| import org.keycloak.admin.api.AdminRootV2; | ||
| import org.keycloak.admin.api.client.ClientsApi; | ||
| import org.keycloak.admin.client.spi.ResteasyClientProvider; | ||
|
|
||
| public class Clients { | ||
|
|
||
| private final ResteasyClientProvider clientProvider; | ||
| private final WebTarget target; | ||
| private final String realmName; | ||
|
|
||
| public Clients(ResteasyClientProvider clientProvider, WebTarget target, String realmName) { | ||
| this.clientProvider = clientProvider; | ||
| this.target = target; | ||
| this.realmName = realmName; | ||
| } | ||
|
|
||
| public ClientsApi v2() { | ||
| return clientProvider.targetProxy(target, AdminRootV2.class).adminApi(realmName).clients("v2"); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,48 +1,21 @@ | ||
| package org.keycloak.admin.api; | ||
|
|
||
| import jakarta.ws.rs.NotFoundException; | ||
| import jakarta.ws.rs.OPTIONS; | ||
| import jakarta.ws.rs.Path; | ||
| import jakarta.ws.rs.PathParam; | ||
| import jakarta.ws.rs.core.Context; | ||
| import jakarta.ws.rs.core.Response; | ||
| import jakarta.ws.rs.ext.Provider; | ||
|
|
||
| import org.keycloak.common.Profile; | ||
| import org.keycloak.models.KeycloakSession; | ||
| import org.keycloak.services.resources.admin.AdminCorsPreflightService; | ||
|
|
||
| import org.eclipse.microprofile.openapi.annotations.Operation; | ||
|
|
||
| @Provider | ||
| @Path("admin/api") | ||
| public class AdminRootV2 { | ||
|
|
||
| @Context | ||
| protected KeycloakSession session; | ||
| public interface AdminRootV2 { | ||
|
|
||
| @Path("{realmName}") | ||
| public AdminApi adminApi(@PathParam("realmName") String realmName) { | ||
| checkApiEnabled(); | ||
| return new DefaultAdminApi(session, realmName); | ||
| } | ||
| AdminApi adminApi(@PathParam("realmName") String realmName); | ||
|
|
||
| // TODO Fix preflights | ||
| @Path("{realmName}/{any:.*}") | ||
| @OPTIONS | ||
| @Operation(hidden = true) | ||
| public Response preFlight() { | ||
| checkApiEnabled(); | ||
| return new AdminCorsPreflightService().preflight(); | ||
| } | ||
|
|
||
| private void checkApiEnabled() { | ||
| if (!isAdminApiV2Enabled()) { | ||
| throw new NotFoundException(); | ||
| } | ||
| } | ||
|
|
||
| public static boolean isAdminApiV2Enabled() { | ||
| return Profile.isFeatureEnabled(Profile.Feature.CLIENT_ADMIN_API_V2); // There's currently only Client API for the new Admin API v2 | ||
| } | ||
| Response preFlight(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package org.keycloak.admin.api; | ||
|
|
||
| import jakarta.ws.rs.NotFoundException; | ||
| import jakarta.ws.rs.core.Context; | ||
| import jakarta.ws.rs.core.Response; | ||
| import jakarta.ws.rs.ext.Provider; | ||
|
|
||
| import org.keycloak.common.Profile; | ||
| import org.keycloak.models.KeycloakSession; | ||
| import org.keycloak.services.resources.admin.AdminCorsPreflightService; | ||
|
|
||
| @Provider | ||
| public class DefaultAdminRootV2 implements AdminRootV2 { | ||
|
|
||
| @Context | ||
| protected KeycloakSession session; | ||
|
|
||
| @Override | ||
| public AdminApi adminApi(String realmName) { | ||
| checkApiEnabled(); | ||
| return new DefaultAdminApi(session, realmName); | ||
| } | ||
|
|
||
| @Override | ||
| public Response preFlight() { | ||
| checkApiEnabled(); | ||
| return new AdminCorsPreflightService().preflight(); | ||
| } | ||
|
|
||
| private void checkApiEnabled() { | ||
| if (!isAdminApiV2Enabled()) { | ||
| throw new NotFoundException(); | ||
| } | ||
| } | ||
|
|
||
| public static boolean isAdminApiV2Enabled() { | ||
| return Profile.isFeatureEnabled(Profile.Feature.CLIENT_ADMIN_API_V2); // There's currently only Client API for the new Admin API v2 | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,6 @@ public String getRealmName() { | |
| public String getClientsApiUrl() { | ||
| return "http://localhost:8080/admin/api/%s/clients/v2".formatted(getRealmName()); | ||
| } | ||
|
|
||
|
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. Just a nitpick. Not necessary change. |
||
| public String getClientApiUrl(String clientId) { | ||
| return "http://localhost:8080/admin/api/%s/clients/v2/%s".formatted(getRealmName(), clientId); | ||
| } | ||
|
|
||
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.
As discussed on the meeting, the concerns about including it into the admin client - we could conditionally register it if the Client V2 feature is enabled. It'll help to decrease the probability that something is broken for the admin client core itself.
cc: @robson90 @keycloak/cloud-native WDYT?
Uh oh!
There was an error while loading. Please reload this page.
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.
IMHO the safest path here would be to make it return
List<>for now.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.
Based upon what the module is doing, it is safe to leave this as is.
Swiching to List is not desirable as that can have a heavier memory footprint server side.
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.
It's an admin client, it doesn't know what features are enabled. We could, hoverer, hide it behind some system property toggle?
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.
If @shawkins has reviewed the logic and has the context, I'm ok to leave it as is.
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.
I understand that, but you are moving the memory footprint from the Server to the Client, as the Client is creating a list from it and then streams it.
Somewhere above are the Debug logs from the Payload, that Keycloak is returning. For me that looks like a normal
List<>as response. Im unsure, if we are getting any real benefit here.Im fine with
Stream<>for now.