Add Jackson 3 support for Keycloak Admin Client#50848
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Jackson 3 support to the Keycloak Admin Client while retaining Jackson 2 compatibility.
Changes:
- Extracts Jackson-independent client code into
keycloak-admin-client-core. - Adds Jackson 3 REST, mapper, serializer, and service-provider implementations.
- Introduces JSON abstractions and Jackson 3 CI profiles for Admin API v1 and v2.
Reviewed changes
Copilot reviewed 64 out of 148 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/actions/conditional/action.yml |
Exposes the Jackson 3 CI condition. |
.github/actions/conditional/conditions |
Selects changes requiring Jackson 3 tests. |
.github/workflows/ci.yml |
Runs Jackson 3 admin-client tests. |
integration/pom.xml |
Registers the new client modules. |
integration/admin-client/pom.xml |
Makes Jackson 2 client depend on shared core. |
integration/admin-client-core/pom.xml |
Defines the Jackson-independent client module. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/Config.java |
Moves client configuration into shared core. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/CreatedResponseUtil.java |
Moves response utilities into shared core. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/Keycloak.java |
Moves main client implementation into shared core. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/KeycloakBuilder.java |
Moves the client builder into shared core. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/spi/ResteasyClientProvider.java |
Defines the REST client provider SPI. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/token/TokenManager.java |
Moves token management into shared core. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/token/TokenService.java |
Moves the token endpoint proxy into shared core. |
integration/admin-client-core/src/main/java/org/keycloak/admin/client/resource/* |
Moves Admin REST resource interfaces and filters into shared core. |
integration/admin-client-jackson3/pom.xml |
Defines the Jackson 3 client module and dependencies. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/JacksonProvider3.java |
Configures the Jackson 3 REST provider. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/Jackson3JsonMapper.java |
Implements the mapper SPI for Jackson 3. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/Jackson3RawJsonValueSupport.java |
Implements raw JSON operations for Jackson 3. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/KeycloakAnnotationIntrospector3.java |
Maps version-neutral annotations to Jackson 3 handlers. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/MultivaluedHashMapValueDeserializer3.java |
Adds Jackson 3 multivalued-map deserialization. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/MultivaluedHashMapValueSerializer3.java |
Adds Jackson 3 multivalued-map serialization. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/ResteasyClientClassicProvider3.java |
Supplies the Jackson 3 RESTEasy client. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/StreamDeserializer3.java |
Adds stream response deserialization. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/StringListMapDeserializer3.java |
Adds flexible string-list map deserialization. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/StringOrArrayDeserializer3.java |
Reads scalar-or-array string values. |
integration/admin-client-jackson3/src/main/java/org/keycloak/admin/client/jackson3/StringOrArraySerializer3.java |
Writes scalar-or-array string values. |
integration/admin-client-jackson3/src/main/resources/META-INF/services/org.keycloak.admin.client.spi.ResteasyClientProvider |
Registers the Jackson 3 REST provider. |
integration/admin-client-jackson3/src/main/resources/META-INF/services/org.keycloak.json.KeycloakJsonMapper |
Registers the Jackson 3 mapper. |
integration/admin-client-jackson3/src/main/resources/META-INF/services/org.keycloak.json.RawJsonValueSupport |
Registers Jackson 3 raw JSON support. |
core/src/main/java/org/keycloak/json/KeycloakJsonMapper.java |
Defines a version-neutral mapper SPI. |
core/src/main/java/org/keycloak/json/KeycloakJsonMapperFactory.java |
Loads the active mapper implementation. |
core/src/main/java/org/keycloak/json/RawJsonValue.java |
Introduces a version-neutral raw JSON wrapper. |
core/src/main/java/org/keycloak/json/RawJsonValueSupport.java |
Defines raw JSON navigation operations. |
core/src/main/java/org/keycloak/json/Jackson2JsonMapper.java |
Implements the mapper SPI for Jackson 2. |
core/src/main/java/org/keycloak/json/Jackson2RawJsonValueSupport.java |
Implements raw JSON operations for Jackson 2. |
core/src/main/java/org/keycloak/json/StringOrArray.java |
Introduces a scalar-or-array annotation. |
core/src/main/java/org/keycloak/json/StringListMap.java |
Introduces a flexible string-list map annotation. |
core/src/main/java/org/keycloak/json/MultivaluedHashMapValue.java |
Introduces a multivalued-map annotation. |
core/src/main/resources/META-INF/services/org.keycloak.json.KeycloakJsonMapper |
Registers the Jackson 2 mapper. |
core/src/main/resources/META-INF/services/org.keycloak.json.RawJsonValueSupport |
Registers Jackson 2 raw JSON support. |
core/src/main/java/org/keycloak/representations/** |
Replaces Jackson 2-specific representation types and annotations. |
core/src/main/java/org/keycloak/jose/jwe/JWE.java |
Uses the mapper SPI for JWE headers. |
core/src/main/java/org/keycloak/jose/jws/JWSBuilder.java |
Uses the mapper SPI for JWS serialization. |
core/src/main/java/org/keycloak/jose/jws/JWSHeader.java |
Uses the mapper SPI for header serialization. |
core/src/main/java/org/keycloak/jose/jws/JWSInput.java |
Uses the mapper SPI when reading headers. |
core/src/main/java/org/keycloak/util/JWKSUtils.java |
Uses the mapper SPI for thumbprints. |
core/src/main/java/org/keycloak/util/TokenUtil.java |
Uses the mapper SPI for token processing. |
core/src/test/java/org/keycloak/json/JacksonAgnosticRepresentationsTest.java |
Checks representations for Jackson 2-specific imports. |
services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java |
Unwraps version-neutral client-policy configuration. |
test-framework/builders/src/main/java/org/keycloak/testframework/realm/ClientPolicyBuilder.java |
Wraps condition configuration in RawJsonValue. |
test-framework/builders/src/main/java/org/keycloak/testframework/realm/ClientProfileBuilder.java |
Wraps executor configuration in RawJsonValue. |
tests/base/pom.xml |
Adds Jackson 3 Admin API v1 smoke tests. |
tests/base/src/test/java/org/keycloak/tests/oid4vc/OID4VCIssuerTestBase.java |
Adapts OID4VC client-policy configuration. |
rest/admin-v2/tests/pom.xml |
Adds the Jackson 3 Admin API v2 profile. |
rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientPoliciesV2Test.java |
Adapts v2 policy configuration to raw JSON. |
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/AbstractClientPoliciesTest.java |
Unwraps configuration for assertions. |
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java |
Wraps test policy configuration. |
| private static final Set<String> ALLOWLISTED_FILES = new HashSet<>(Arrays.asList( | ||
| "MultivaluedHashMapValueSerializer.java", | ||
| "MultivaluedHashMapValueDeserializer.java", | ||
| "AuthorizationSchema.java" |
There was a problem hiding this comment.
But this is Jackson 3 support for client, server specific things are not relevant. That would make PR much larger.
6f14125 to
0c0dfde
Compare
| } | ||
|
|
||
| public JsonNode getConfiguration() { | ||
| public RawJsonValue getConfiguration() { |
There was a problem hiding this comment.
I can't keep the same type if it is not present in Jackson 3. It is "breaking", but hardly an issue considering I provide same methods that are relevant. If you tightly couple representation with underlying libraries, this is what you get.
| } | ||
|
|
||
| public JsonNode getConfiguration() { | ||
| public RawJsonValue getConfiguration() { |
There was a problem hiding this comment.
Same comment, same answer, read here: #50848 (comment)
| <profile> | ||
| <id>jackson3</id> | ||
| <dependencies> |
There was a problem hiding this comment.
keycloak-admin-client-tests is NOT on the classpath, therefore ResteasyClientClassicProvider and JacksonProvider that live in that module cannot be loaded. keycloak-admin-v2-services needs Jackson 2 for server, but it is not used by the client as Jackson 3 ignores Jackson 2 stuff which is relevant in this context.
| <profile> | ||
| <id>jackson3</id> | ||
| <dependencies> |
0c0dfde to
34ac05a
Compare
|
I am actually not completely sure which labels should this PR have, so I left such one I am sure and someone else can add additional. |
34ac05a to
770d8f7
Compare
770d8f7 to
08e5a91
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.cluster.RealmInvalidationClusterTest#crudWithFailoverKeycloak CI - Store IT (mssql) |
|
We'll need to reviews from Core teams, especially @mposolda. |
|
I made a first pass through the changes and it seems like a good approach - great job @michalvavrik. I'll take a deeper look tomorrow. |
shawkins
left a comment
There was a problem hiding this comment.
Just one nit on convertValue, otherwise I think it looks good.
The comments about this being a breaking api change are of course correct. The only way around that would be:
- see if we can create / maintain a Jackson 2 to Jackson 3 shim - which seems pretty painful.
- have the admin client jar creation process also create a fork of the core that replaces usage of RawJsonValue with JsonNode. However this further complicates the admin client dependencies, and would require a good bit of discipline to ensure the references could be directly replaced (for example remove how the type is currently being used in hashcode and equals methods).
Seems like that would be a call for @mposolda to make on whether full compatibility is required.
08e5a91 to
b46fc8d
Compare
| core/src/main/java/org/keycloak/json/ ci-jackson3 | ||
| core/src/main/java/org/keycloak/representations/ ci-jackson3 | ||
| core/src/main/resources/META-INF/services/ ci-jackson3 |
There was a problem hiding this comment.
The Jackson 3 gate still omits changed core paths that execute in this client: DPoPAuthFilter calls DPoPGenerator, which uses the modified jose/jws/JWSBuilder, and this PR also routes TokenUtil/JWKSUtils through the new mapper. A later change to those paths can therefore bypass the only Jackson 3 job; add the JOSE path and the relevant utility files to this condition.
I think it would be misleading, because I don't see which client-side tests are verifying it. It is not straighforward, I could be missing something of course.
| <includes> | ||
| <include>**/AdminClientTest.java</include> | ||
| <include>**/RealmCreateTest.java</include> | ||
| <include>**/UserCreateTest.java</include> | ||
| <include>**/ClientTest.java</include> | ||
| <include>**/GroupTest.java</include> | ||
| </includes> |
There was a problem hiding this comment.
I didn't find a DPoP test in the new test framework which actually tests this DPoPAuthFilter. This is just about writeValueAsBytes which is trivial, I don't think there is an issue.
b46fc8d to
6c85c90
Compare
|
I think @mposolda is OOO, I'll ping him next week and ask if he can find a time for a review. |
|
@michalvavrik I looked at the It today because Maven consistently orders Would it be worth adding a priority mechanism to Edit: I noticed that Copilot already raised this above, but if I understand correctly, only the missing-dependency case has been covered, not the both-present case (which I think is a valid concern for the CI environment). Other than this, it looks good from my perspective. Thanks! |
|
@Pepo48 I thought about it yesterday and today again with stellar output: "Kinda". Let my clarify my starting points in case we do not share them:
If we can agree on that ^^, I will add priority to Jackson 3 Working on it now. |
Closes: keycloak#48216 Signed-off-by: Michal Vavřík <dev@michalvavrik.net>
6c85c90 to
0324a48
Compare
| try { | ||
| executor.setConfiguration(JsonSerialization.mapper.readValue(JsonSerialization.mapper.writeValueAsBytes(config), JsonNode.class)); | ||
| } catch(IOException e) { | ||
| executor.setConfiguration(RawJsonValue.of(JsonSerialization.mapper.readValue(JsonSerialization.mapper.writeValueAsBytes(config), JsonNode.class))); |
There was a problem hiding this comment.
If tests are passing, what is the point? Jackson 3 profile is a temporary smoke check.
There was a problem hiding this comment.
I didn't want to make such changes as it will smell to someone without context, but I'll address this probably, it sounds like it makes sense. Hope its OK.
There was a problem hiding this comment.
Actually, no, it is change for default tests as well and I don't want to affect them, I want to keep original test assertion. I'll keep it as is. I run all tests and they are passing locally.
| } | ||
| try { | ||
| condition.setConfiguration(JsonSerialization.mapper.readValue(JsonSerialization.mapper.writeValueAsBytes(config), JsonNode.class)); | ||
| condition.setConfiguration(RawJsonValue.of(JsonSerialization.mapper.readValue(JsonSerialization.mapper.writeValueAsBytes(config), JsonNode.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.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (chrome) |
Uh oh!
There was an error while loading. Please reload this page.