fix: creating a cleaner module for use by java clients#47874
fix: creating a cleaner module for use by java clients#47874vmuzikar merged 3 commits intokeycloak:mainfrom
Conversation
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.model.session.AuthenticationSessionTest#testConcurrentAuthenticationSessionsCreationKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.AuthenticationSessionTest#testConcurrentAuthenticationSessionsCreationKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests |
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.model.session.UserSessionProviderOfflineModelTest#testOfflineSessionLazyLoadingKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests |
c331a54 to
c7e4a10
Compare
michalvavrik
left a comment
There was a problem hiding this comment.
+1 for extracting API. It feels like I shouldn't be the making this call as I am not yet familiar with all customs here, so I'll skip this one.
537803d to
be4f71c
Compare
With two modules the ValidationContext is used to isolate the "rest" module from keycloak-server-spi. Alternatives:
|
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.model.session.UserSessionProviderModelTest#testCreateUserSessionsParallelKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.AuthenticationSessionTest#testConcurrentAuthenticationSessionsCreationKeycloak CI - Store Model Tests |
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.model.session.UserSessionInitializerTest#testUserSessionPropagationBetweenSitesKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testLazyOfflineUserSessionFetchingKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionProviderOfflineModelTest#testOfflineSessionLazyLoadingKeycloak CI - Store Model Tests |
As discussed offline, let's try the 3 module structure: |
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.model.session.UserSessionProviderModelTest#testCreateUserSessionsParallelKeycloak CI - Store Model Tests |
closes: keycloak#48114 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
225746f to
29df2ac
Compare
also remove jsonnode logic from the oas filter and the databind dependency Signed-off-by: Steve Hawkins <shawkins@redhat.com>
| if (type == BaseClientRepresentation.class) { | ||
| uuidProvider = new ClientUuidProvider(); | ||
| } else { | ||
| throw new AssertionError("Not Uuid Provider defined for " + type); |
There was a problem hiding this comment.
| throw new AssertionError("Not Uuid Provider defined for " + type); | |
| throw new AssertionError("No Uuid Provider defined for " + type); |
| </exclusions> | ||
| </dependency> | ||
|
|
||
There was a problem hiding this comment.
let's drop the trailing white space (in case it was intentional, I'd like to learn about it so that I understand in the future as well)
There was a problem hiding this comment.
It's not intentional, just my ide seems to not be set to remove trailing from all file types.
| </dependency> | ||
| </dependencies> | ||
|
|
||
There was a problem hiding this comment.
(trailing white space, the new line is fine)
| uuidProvider = constraintAnnotation.uuidProvider().getDeclaredConstructor().newInstance(); | ||
| } catch (ReflectiveOperationException e) { | ||
| throw new RuntimeException("Failed to instantiate UUID provider: " + constraintAnnotation.uuidProvider().getName(), e); | ||
| Class<?> type = constraintAnnotation.type(); |
There was a problem hiding this comment.
This requires no changes if you want to make it explicit. I just meant to note you could avoid type (drop the annotation attribute) as you already know the type in isValid anyway, so you could initialize uuidProvider there lazily into volatile prop. It is just matter of preference and yours is fine with me.
There was a problem hiding this comment.
Thank you for this simplification. Updated to remove the type attribute and addressed the other review comments.
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
| if (BaseClientRepresentation.class.isAssignableFrom(type)) { | ||
| uuidProvider = new ClientUuidProvider(); | ||
| } else { | ||
| throw new AssertionError("No UuidProvider defined for " + type); | ||
| } |
There was a problem hiding this comment.
|
Build seems to be failing, though. |
Should just be some npm flakiness. |
* fix: minimizing the dependencies for the rest module closes: keycloak#48114 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * renaming the modules also remove jsonnode logic from the oas filter and the databind dependency Signed-off-by: Steve Hawkins <shawkins@redhat.com> * addressing review comments Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
close: #48114
Rather than relying on dependency exclusion, we can refactor to have a cleaner layer for clients to use. Basically the representations, validation annotations, and rest interfaces are now in the "api" module, with everything else in "services".