JPA map storage: Groups no-downtime store#9997
Conversation
fea5ee4 to
a0fd2a5
Compare
a0fd2a5 to
5ac96fd
Compare
ahus1
left a comment
There was a problem hiding this comment.
I reviewed the code and left some comments.
When testing this locally with an empty database, I found that I was unable to login.
The following SQL failed - apparently at the empty "in" select here jpagroupen0_.id in ()
Here's the full SQL: select jpagroupen0_.id as col_0_0_, jpagroupen0_.version as col_1_0_, jpagroupen0_.entityVersion as col_2_0_, jpagroupen0_.realmId as col_3_0_, jpagroupen0_.name as col_4_0_, jpagroupen0_.parentId as col_5_0_ from "group" jpagroupen0_ where (jpagroupen0_.id in ()) and jpagroupen0_.realmId=? order by jpagroupen0_.name asc
Here's the stacktrace:
13:52:31,007 ERROR XNIO-1 task-2 [org.keycloak.services.error.KeycloakErrorHandler] Uncaught server error
org.hibernate.exception.SQLGrammarException: could not extract ResultSet
at org.hibernate.exception.internal.SQLStateConversionDelegate.convert(SQLStateConversionDelegate.java:106)
at org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:42)
at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:113)
at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:99)
at org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.extract(ResultSetReturnImpl.java:69)
at org.hibernate.loader.Loader.getResultSet(Loader.java:2265)
at org.hibernate.loader.Loader.executeQueryStatement(Loader.java:2028)
at org.hibernate.loader.Loader.executeQueryStatement(Loader.java:1990)
at org.hibernate.loader.Loader.scroll(Loader.java:2863)
at org.hibernate.loader.hql.QueryLoader.scroll(QueryLoader.java:574)
at org.hibernate.hql.internal.ast.QueryTranslatorImpl.scroll(QueryTranslatorImpl.java:447)
at org.hibernate.engine.query.spi.HQLQueryPlan.performScroll(HQLQueryPlan.java:354)
at org.hibernate.internal.SessionImpl.scroll(SessionImpl.java:1658)
at org.hibernate.query.internal.AbstractProducedQuery.doScroll(AbstractProducedQuery.java:1537)
at org.hibernate.query.internal.AbstractProducedQuery.scroll(AbstractProducedQuery.java:1523)
at org.hibernate.query.internal.AbstractProducedQuery.stream(AbstractProducedQuery.java:1547)
at org.hibernate.query.criteria.internal.compile.CriteriaQueryTypeQueryAdapter.stream(CriteriaQueryTypeQueryAdapter.java:89)
at org.hibernate.query.Query.getResultStream(Query.java:1107)
at org.keycloak.models.map.storage.jpa.JpaMapKeycloakTransaction.read(JpaMapKeycloakTransaction.java:111)
at org.keycloak.models.map.group.MapGroupProvider.getGroupsStream(MapGroupProvider.java:113)
at org.keycloak.storage.GroupStorageManager.getGroupsStream(GroupStorageManager.java:79)
at org.keycloak.models.cache.infinispan.RealmCacheSession.getGroupsStream(RealmCacheSession.java:919)
at org.keycloak.models.jpa.UserAdapter.getGroupsStream(UserAdapter.java:377)
at org.keycloak.models.jpa.UserAdapter.getGroupsStream(UserAdapter.java:372)
at org.keycloak.models.cache.infinispan.entities.CachedUser.lambda$new$3(CachedUser.java:68)
at org.keycloak.models.cache.infinispan.DefaultLazyLoader.get(DefaultLazyLoader.java:43)
at org.keycloak.models.cache.infinispan.entities.CachedUser.getGroups(CachedUser.java:116)
at org.keycloak.models.cache.infinispan.UserAdapter.getGroupsStream(UserAdapter.java:340)
at org.keycloak.models.utils.KeycloakModelUtils.resolveAttribute(KeycloakModelUtils.java:514)
at org.keycloak.protocol.oidc.mappers.UserAttributeMapper.setClaim(UserAttributeMapper.java:101)
at org.keycloak.protocol.oidc.mappers.AbstractOIDCProtocolMapper.setClaim(AbstractOIDCProtocolMapper.java:132)
at org.keycloak.protocol.oidc.mappers.AbstractOIDCProtocolMapper.transformAccessToken(AbstractOIDCProtocolMapper.java:82)
at org.keycloak.protocol.oidc.TokenManager.lambda$transformAccessToken$4(TokenManager.java:735)
at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:258)
at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:258)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:485)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
at org.keycloak.protocol.oidc.TokenManager.transformAccessToken(TokenManager.java:734)
at org.keycloak.protocol.oidc.TokenManager.createClientAccessToken(TokenManager.java:531)
at org.keycloak.protocol.oidc.endpoints.TokenEndpoint.createTokenResponse(TokenEndpoint.java:437)
at org.keycloak.protocol.oidc.endpoints.TokenEndpoint.codeToToken(TokenEndpoint.java:432)
at org.keycloak.protocol.oidc.endpoints.TokenEndpoint.processGrantRequest(TokenEndpoint.java:196)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.jboss.resteasy.core.MethodInjectorImpl.invoke(MethodInjectorImpl.java:138)
at org.jboss.resteasy.core.ResourceMethodInvoker.internalInvokeOnTarget(ResourceMethodInvoker.java:546)
at org.jboss.resteasy.core.ResourceMethodInvoker.invokeOnTargetAfterFilter(ResourceMethodInvoker.java:435)
at org.jboss.resteasy.core.ResourceMethodInvoker.lambda$invokeOnTarget$0(ResourceMethodInvoker.java:396)
at org.jboss.resteasy.core.interception.PreMatchContainerRequestContext.filter(PreMatchContainerRequestContext.java:358)
at org.jboss.resteasy.core.ResourceMethodInvoker.invokeOnTarget(ResourceMethodInvoker.java:398)
at org.jboss.resteasy.core.ResourceMethodInvoker.invoke(ResourceMethodInvoker.java:365)
at org.jboss.resteasy.core.ResourceLocatorInvoker.invokeOnTargetObject(ResourceLocatorInvoker.java:150)
at org.jboss.resteasy.core.ResourceLocatorInvoker.invoke(ResourceLocatorInvoker.java:110)
at org.jboss.resteasy.core.ResourceLocatorInvoker.invokeOnTargetObject(ResourceLocatorInvoker.java:141)
at org.jboss.resteasy.core.ResourceLocatorInvoker.invoke(ResourceLocatorInvoker.java:104)
at org.jboss.resteasy.core.SynchronousDispatcher.invoke(SynchronousDispatcher.java:440)
at org.jboss.resteasy.core.SynchronousDispatcher.lambda$invoke$4(SynchronousDispatcher.java:229)
at org.jboss.resteasy.core.SynchronousDispatcher.lambda$preprocess$0(SynchronousDispatcher.java:135)
at org.jboss.resteasy.core.interception.PreMatchContainerRequestContext.filter(PreMatchContainerRequestContext.java:358)
at org.jboss.resteasy.core.SynchronousDispatcher.preprocess(SynchronousDispatcher.java:138)
at org.jboss.resteasy.core.SynchronousDispatcher.invoke(SynchronousDispatcher.java:215)
at org.jboss.resteasy.plugins.server.servlet.ServletContainerDispatcher.service(ServletContainerDispatcher.java:245)
at org.jboss.resteasy.plugins.server.servlet.HttpServletDispatcher.service(HttpServletDispatcher.java:61)
at org.jboss.resteasy.plugins.server.servlet.HttpServletDispatcher.service(HttpServletDispatcher.java:56)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:847)
at io.undertow.servlet.handlers.ServletHandler.handleRequest(ServletHandler.java:74)
at io.undertow.servlet.handlers.FilterHandler$FilterChainImpl.doFilter(FilterHandler.java:129)
at org.keycloak.testsuite.UndertowRequestFilter.lambda$doFilter$0(UndertowRequestFilter.java:49)
at org.keycloak.services.filters.AbstractRequestFilter.filter(AbstractRequestFilter.java:43)
at org.keycloak.testsuite.UndertowRequestFilter.doFilter(UndertowRequestFilter.java:47)
at io.undertow.servlet.core.ManagedFilter.doFilter(ManagedFilter.java:61)
at io.undertow.servlet.handlers.FilterHandler$FilterChainImpl.doFilter(FilterHandler.java:131)
at io.undertow.servlet.handlers.FilterHandler.handleRequest(FilterHandler.java:84)
at io.undertow.servlet.handlers.security.ServletSecurityRoleHandler.handleRequest(ServletSecurityRoleHandler.java:62)
at io.undertow.servlet.handlers.ServletChain$1.handleRequest(ServletChain.java:68)
at io.undertow.servlet.handlers.ServletDispatchingHandler.handleRequest(ServletDispatchingHandler.java:36)
at io.undertow.servlet.handlers.RedirectDirHandler.handleRequest(RedirectDirHandler.java:68)
at io.undertow.servlet.handlers.security.SSLInformationAssociationHandler.handleRequest(SSLInformationAssociationHandler.java:117)
at io.undertow.servlet.handlers.security.ServletAuthenticationCallHandler.handleRequest(ServletAuthenticationCallHandler.java:57)
at io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
at io.undertow.security.handlers.AbstractConfidentialityHandler.handleRequest(AbstractConfidentialityHandler.java:46)
at io.undertow.servlet.handlers.security.ServletConfidentialityConstraintHandler.handleRequest(ServletConfidentialityConstraintHandler.java:64)
at io.undertow.security.handlers.AuthenticationMechanismsHandler.handleRequest(AuthenticationMechanismsHandler.java:60)
at io.undertow.servlet.handlers.security.CachedAuthenticatedSessionHandler.handleRequest(CachedAuthenticatedSessionHandler.java:77)
at io.undertow.security.handlers.AbstractSecurityContextAssociationHandler.handleRequest(AbstractSecurityContextAssociationHandler.java:43)
at io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
at io.undertow.servlet.handlers.SendErrorPageHandler.handleRequest(SendErrorPageHandler.java:52)
at io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
at io.undertow.servlet.handlers.ServletInitialHandler.handleFirstRequest(ServletInitialHandler.java:269)
at io.undertow.servlet.handlers.ServletInitialHandler.access$100(ServletInitialHandler.java:78)
at io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:133)
at io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:130)
at io.undertow.servlet.core.ServletRequestContextThreadSetupAction$1.call(ServletRequestContextThreadSetupAction.java:48)
at io.undertow.servlet.core.ContextClassLoaderSetupAction$1.call(ContextClassLoaderSetupAction.java:43)
at io.undertow.servlet.handlers.ServletInitialHandler.dispatchRequest(ServletInitialHandler.java:249)
at io.undertow.servlet.handlers.ServletInitialHandler.access$000(ServletInitialHandler.java:78)
at io.undertow.servlet.handlers.ServletInitialHandler$1.handleRequest(ServletInitialHandler.java:99)
at io.undertow.server.Connectors.executeRootHandler(Connectors.java:387)
at io.undertow.server.HttpServerExchange$1.run(HttpServerExchange.java:841)
at org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
at org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1982)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377)
at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: org.postgresql.util.PSQLException: ERROR: syntax error at or near ")"
Position: 263
at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2553)
at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2285)
at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:323)
at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:473)
at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:393)
at org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:164)
at org.postgresql.jdbc.PgPreparedStatement.executeQuery(PgPreparedStatement.java:114)
at org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.extract(ResultSetReturnImpl.java:60)
... 109 more
This is how I tested
-Dkeycloak.profile.feature.map_storage=enabled
-Dkeycloak.profile.feature.token_exchange=enabled
-Dkeycloak.profile.feature.map_storage=enabled
-Dkeycloak.client.provider=map
-Dkeycloak.clientScope.provider=map
-Dkeycloak.role.provider=map
-Dkeycloak.group.provider=map
-Dkeycloak.client.map.storage.provider=jpa-map-storage
-Dkeycloak.clientScope.map.storage.provider=jpa-map-storage
-Dkeycloak.role.map.storage.provider=jpa-map-storage
-Dkeycloak.group.map.storage.provider=jpa-map-storage
5ac96fd to
97221d6
Compare
|
Does merging #10129 affect the Liquibase scripts provided here? |
97221d6 to
d31a463
Compare
|
@ahus1 thank you. The following SQL failed - apparently at the empty "in" select here I've used It results in a condition |
ahus1
left a comment
There was a problem hiding this comment.
I did a review and the previous code review remarks were resolved. I found a new issue with one of the indexes and the indention.
I tried to test the situation with the empty "IN" selector, but I didn't succeed yet: The fix for synchronizing the Liquibase updates is not part of this PR 442d9ba - I still get the errors with "cannot end scope". Could you please rebase it, and re-request the PR review? Thanks!
There was a problem hiding this comment.
Nitpick: indentation needs fixing, "for" would be indented one level less.
There was a problem hiding this comment.
I'm sorry that I missed that earlier: This index should be IMHO split:
-
one unique index to ensure that realmid + name of the group are unique
-
another non-unique index for the parentId so that the foreign key is indexed and simple to look up when traversing the tree upwards. The index for parentId doesn't need to contain the group name and also doesn't need to contain the realmId as parentId is unique across realms ... at least technically, I don't know what query the criteria API constructs here.
There was a problem hiding this comment.
- one unique index to ensure that realmid + name of the group are unique
This would not be sufficient: The name needs to be unique within parent. For example, it must be possible to create a group "students" inside a group "2021" as well as the group "students" inside "2020".
- another non-unique index for the parentId so that the foreign key is indexed and simple to look up when traversing the tree upwards. The index for parentId doesn't need to contain the group name and also doesn't need to contain the realmId as parentId is unique across realms ... at least technically, I don't know what query the criteria API constructs here.
Is there any supporting use case for this index?
There was a problem hiding this comment.
Thanks for noting why the index needs to have three columns, I stand corrected. I didn't know the same name could appear in separate branches of the group tree.
To be able to query by parentId, I see the following use cases:
-
to validate the foreign key (group is referencing itself) - looking at the columns again, I don't see that foreign key in the schema, there is only a foreign key in the attributes. A foreign key constraint could ensure that the parent exists, and could also delete cascade children.
-
to search the leafs of a groups by parent ID (the getSubGroupsStream method as in classic JPA) ... now looking more closely in MapGroupAdapter, this is could be quite inefficient as it streams all groups from the database and then filters :-( Still, one could leave that for a future optimization and create a criteria query in MapGroupAdapter.
There was a problem hiding this comment.
Thank you for the details. I can now see the potential for the (realmId, parentId) index. Could you please create an issue to be addressed re getSubGroupsStream inefficiency?
For the sake of comleteness - the validation argument would only hold if the groups were only stored in JPA and only within a single JPA instance - then the index would be required. This assumption does not hold for the tree storage where the parent groups might be stored elsewhere, e.g. in LDAP.
There was a problem hiding this comment.
I'll add the issue later today. Reading that the parent can be stored somewhere else, there shouldn't be a foreign key constrained here. Reading this, my review comment is now resolved. I'll update this comment to reference the new issue later.
UPDATE: issue added #10193
d31a463 to
d068bff
Compare
ahus1
left a comment
There was a problem hiding this comment.
I reviewed the code and also did a local test: starting out with an empty database I was able to create and delete roles. Thank you for the PR!
Closes #9660