Repository navigation
Client streaming for admin api v2 - #47611
Conversation
e436488 to
52a43e0
Compare
|
Marked most of the copilot reviews as not needed - we only care about ensuring that the inputstream is closed. And while we don't and we don't expect an untyped Stream to be used in our api, there's no harm in accounting for it in the type handling, so that change was applied. |
5d4eb43 to
3dd8c4a
Compare
3dd8c4a to
7c87e7f
Compare
|
Should be ready for review. I didn't initially appriciate there is a difference registering something as an object vs a class, which means we need to keep the if / else when creating the client. It also means that any custom json handler must be an instance, not a class. |
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.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) |
7c87e7f to
af11244
Compare
3167a9e to
ddc0c5d
Compare
ebdac4b to
5acb37c
Compare
|
This should be ready. It will need some kind of follow-up for the admin client docs that indicates any Streams accessed should be read completely or closed. |
5acb37c to
9dad046
Compare
| if (token == JsonToken.END_ARRAY) { | ||
| return Stream.of(); | ||
| } | ||
| if (token != JsonToken.START_OBJECT) { |
There was a problem hiding this comment.
This won't work for anything else than streams of objects. Is that correct?
There was a problem hiding this comment.
Yes, that is the only payload expected with Stream<Object>
There was a problem hiding this comment.
Hmmm, I'm thinking if we need the support for streaming primitives as well, but probably not.
| throw new IOException("Expected Object"); | ||
| } | ||
|
|
||
| Class<?> elementType = Object.class; |
There was a problem hiding this comment.
We are loosing type safety for things IMO should be an illegal state ex (raw, standalone wildcard, type variable and streams of lists). If you want to keep it as is, it is fine with me, but my preference would be to support what we want to support like Stream<ClientRepresentation> and fire exception for other scenarios.
On the other hand, I expect everything to be tested, hence anytime someone would actually use it with our client when the element was Object I'd expect something like ClassCastException downstream, so we will know one way or another.
well, that was waste of words 😀
There was a problem hiding this comment.
LGTM. I tested it a bit and I have just one UX suggestion below. But it's not something that would block this PR from being merged.
Thanks Steve!
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: keycloak#47542 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
…nt/spi/StreamMessageBodyReader.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com>
56a636b to
5412c56
Compare
There was a problem hiding this comment.
@shawkins I am fine with changes in this PR as long as everything is backwards compatible and testsuite is passing OK. Which seems to be the case :-)
Also curious if something needs to be changed in this documentation https://www.keycloak.org/securing-apps/admin-client ? It seems to me that everything on that page would be still valid and people will not need any update in their code once they upgrade to the newer version of java admin-client with your changes incorporated. Can you please confirm?
That is correct, this pr isn't adding anything new to what a user needs to do for v1 - but it is clarifying some of the code to avoid any confusion. I also created #47582 to further refine how we're creating the client and update the docs as necessary. |
* demonstrates client side streaming Signed-off-by: Steve Hawkins <shawkins@redhat.com> * fix: adding client streaming support closes: keycloak#47542 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com> * using onClose instead of the proxy close Signed-off-by: Steve Hawkins <shawkins@redhat.com> * Update integration/admin-client/src/main/java/org/keycloak/admin/client/spi/StreamMessageBodyReader.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Peter Zaoral <pepo48@gmail.com>
closes: #47542
In the end the most straight-forward solution seems to be making the logic Resteasy classic specific. I don't see that other stacks implement a proxy method, so realistically the only other thing we will need to worry about is resteasy reactive. For that to work, we'll need an RxInvoker (but simplied to resteasy reactive specifically).
The extra changes for ClientBuilderWrapper and ResteasyClassicClientProvider could alternatively go under #47582 - but I wanted to show here at least that we could consolidate to a single method for creating a ClientBuilder that would be default add the Stream logic.
cc @robson90