From 12dd3edb1080bd53bc4861c3bb4c50f183c76aa3 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Tue, 6 Jun 2023 14:17:54 +0200 Subject: [PATCH] Fix pagination issue with H6 With Hibernate ORM 6, pagination started to be unreliable: When setting the max results only if the first row was 0 has randomly affected other threads where first row was greater than 0. The latter thread sometimes produced query which did *not* account for the offset (cf. threads `-t1` and `-t2` below, while `-t2` missed the `offset ? rows` part whic `-t3` has). This has been fixed by setting the first row offset unconditionally. Closes: #20202 Closes: #16570 ``` 2023-06-02 10:19:03.855000 TRACE [org.keycloak.models.sessions.infinispan.initializer.SessionInitializerWorker] (blocking-thread-node-2-p8-t1) Running computation for segment 0 with worker 0 2023-06-02 10:19:03.856000 TRACE [org.keycloak.models.sessions.infinispan.initializer.OfflinePersistentUserSessionLoader] (blocking-thread-node-2-p8-t1) Loading sessions for segment=0 lastSessionId=00000000-0000-0000-0000-000000000000 first=0 2023-06-02 10:19:03.856000 DEBUG [org.keycloak.models.jpa.PaginationUtils] (blocking-thread-node-2-p8-t1) Set max to 64 in org.hibernate.query.sqm.internal.QuerySqmImpl@2fb60f8b 2023-06-02 10:19:03.856000 DEBUG [org.keycloak.models.jpa.PaginationUtils] (blocking-thread-node-2-p8-t1) After pagination: 0, 64 2023-06-02 10:19:03.857000 TRACE [org.keycloak.models.sessions.infinispan.initializer.SessionInitializerWorker] (blocking-thread-node-2-p8-t2) Running computation for segment 1 with worker 1 2023-06-02 10:19:03.857000 TRACE [org.keycloak.models.sessions.infinispan.initializer.OfflinePersistentUserSessionLoader] (blocking-thread-node-2-p8-t2) Loading sessions for segment=1 lastSessionId=00000000-0000-0000-0000-000000000000 first=64 2023-06-02 10:19:03.857000 TRACE [org.keycloak.models.sessions.infinispan.initializer.SessionInitializerWorker] (blocking-thread-node-2-p8-t3) Running computation for segment 2 with worker 2 2023-06-02 10:19:03.857000 DEBUG [org.keycloak.models.jpa.PaginationUtils] (blocking-thread-node-2-p8-t2) Set first to 64 in org.hibernate.query.sqm.internal.QuerySqmImpl@71464e9f 2023-06-02 10:19:03.857000 DEBUG [org.keycloak.models.jpa.PaginationUtils] (blocking-thread-node-2-p8-t2) Set max to 64 in org.hibernate.query.sqm.internal.QuerySqmImpl@71464e9f 2023-06-02 10:19:03.857000 DEBUG [org.keycloak.models.jpa.PaginationUtils] (blocking-thread-node-2-p8-t2) After pagination: 64, 64 2023-06-02 10:19:03.857000 TRACE [org.keycloak.models.sessions.infinispan.initializer.OfflinePersistentUserSessionLoader] (blocking-thread-node-2-p8-t3) Loading sessions for segment=2 lastSessionId=00000000-0000-0000-0000-000000000000 first=128 10:19:03,859 DEBUG [org.hibernate.SQL] (blocking-thread-node-2-p8-t1) select p1_0.OFFLINE_FLAG, p1_0.USER_SESSION_ID, p1_0.CREATED_ON, p1_0.DATA, p1_0.LAST_SESSION_REFRESH, p1_0.REALM_ID, p1_0.USER_ID from OFFLINE_USER_SESSION p1_0, REALM r1_0 where r1_0.ID=p1_0.REALM_ID and p1_0.OFFLINE_FLAG=? and p1_0.USER_SESSION_ID>? order by p1_0.USER_SESSION_ID fetch first ? rows only 10:19:03,859 DEBUG [org.hibernate.SQL] (blocking-thread-node-2-p8-t2) select p1_0.OFFLINE_FLAG, p1_0.USER_SESSION_ID, p1_0.CREATED_ON, p1_0.DATA, p1_0.LAST_SESSION_REFRESH, p1_0.REALM_ID, p1_0.USER_ID from OFFLINE_USER_SESSION p1_0, REALM r1_0 where r1_0.ID=p1_0.REALM_ID and p1_0.OFFLINE_FLAG=? and p1_0.USER_SESSION_ID>? order by p1_0.USER_SESSION_ID fetch first ? rows only 2023-06-02 10:19:03.860000 TRACE [org.hibernate.orm.jdbc.bind] (blocking-thread-node-2-p8-t1) binding parameter [1] as [VARCHAR] - [1] 2023-06-02 10:19:03.860000 TRACE [org.hibernate.orm.jdbc.bind] (blocking-thread-node-2-p8-t1) binding parameter [2] as [VARCHAR] - [00000000-0000-0000-0000-000000000000] 2023-06-02 10:19:03.860000 TRACE [org.hibernate.orm.jdbc.bind] (blocking-thread-node-2-p8-t1) binding parameter [3] as [INTEGER] - [64] 10:19:03,860 DEBUG [org.hibernate.SQL] (blocking-thread-node-2-p8-t3) select p1_0.OFFLINE_FLAG, p1_0.USER_SESSION_ID, p1_0.CREATED_ON, p1_0.DATA, p1_0.LAST_SESSION_REFRESH, p1_0.REALM_ID, p1_0.USER_ID from OFFLINE_USER_SESSION p1_0, REALM r1_0 where r1_0.ID=p1_0.REALM_ID and p1_0.OFFLINE_FLAG=? and p1_0.USER_SESSION_ID>? order by p1_0.USER_SESSION_ID offset ? rows fetch first ? rows only 2023-06-02 10:19:03.861000 TRACE [org.hibernate.orm.jdbc.bind] (blocking-thread-node-2-p8-t3) binding parameter [3] as [INTEGER] - [128] 2023-06-02 10:19:03.861000 TRACE [org.hibernate.orm.jdbc.bind] (blocking-thread-node-2-p8-t3) binding parameter [4] as [INTEGER] - [64] ``` Co-authored-by: mkanis --- .../OfflinePersistentUserSessionLoader.java | 2 +- .../keycloak/models/jpa/PaginationUtils.java | 6 ++-- .../JpaUserSessionPersisterProvider.java | 6 +++- ...tentAuthenticatedClientSessionAdapter.java | 5 +++ .../session/PersistentUserSessionAdapter.java | 5 +++ .../OfflineSessionPersistenceTest.java | 31 ++++++++++++++++--- 6 files changed, 46 insertions(+), 9 deletions(-) diff --git a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/initializer/OfflinePersistentUserSessionLoader.java b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/initializer/OfflinePersistentUserSessionLoader.java index 9a4065c8f411..83d5f1209b50 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/initializer/OfflinePersistentUserSessionLoader.java +++ b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/initializer/OfflinePersistentUserSessionLoader.java @@ -94,7 +94,7 @@ public OfflinePersistentWorkerResult createFailedWorkerResult(OfflinePersistentL public OfflinePersistentWorkerResult loadSessions(KeycloakSession session, OfflinePersistentLoaderContext loaderContext, OfflinePersistentWorkerContext ctx) { int first = ctx.getWorkerId() * sessionsPerSegment; - log.tracef("Loading sessions for segment=%d lastSessionId=%s", ctx.getSegment(), ctx.getLastSessionId()); + log.tracef("Loading sessions for segment=%d lastSessionId=%s first=%d", ctx.getSegment(), ctx.getLastSessionId(), (Object) first); UserSessionPersisterProvider persister = session.getProvider(UserSessionPersisterProvider.class); List sessions = persister diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/PaginationUtils.java b/model/jpa/src/main/java/org/keycloak/models/jpa/PaginationUtils.java index f52014acf1fa..cdb64906b0d6 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/PaginationUtils.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/PaginationUtils.java @@ -22,11 +22,11 @@ public class PaginationUtils { public static final int DEFAULT_MAX_RESULTS = Integer.MAX_VALUE >> 1; - + public static TypedQuery paginateQuery(TypedQuery query, Integer first, Integer max) { - if (first != null && first > 0) { + if (first != null && first >= 0) { query = query.setFirstResult(first); - + // Workaround for https://hibernate.atlassian.net/browse/HHH-14295 if (max == null || max < 0) { max = DEFAULT_MAX_RESULTS; diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java index 2d5b3745af3e..935863f9719c 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java @@ -417,7 +417,6 @@ public AuthenticatedClientSessionModel loadClientSession(RealmModel realm, Clien * @return */ private Stream loadUserSessionsWithClientSessions(TypedQuery query, String offlineStr, boolean useExact) { - List userSessionAdapters = closing(query.getResultStream() .map(this::toAdapter) .filter(Objects::nonNull)) @@ -463,6 +462,8 @@ private Stream loadUserSessionsWithClientSessions(TypedQuery

offlineSessionIds = createOfflineSessions(realmId, userIds); + Map> offlineSessionIdsDetailed = createOfflineSessionsDetailed(realmId, userIds); + Collection offlineSessionIds = offlineSessionIdsDetailed.values().stream().flatMap(Set::stream).collect(Collectors.toCollection(TreeSet::new)); assertOfflineSessionsExist(realmId, offlineSessionIds); // Simulate server restart reinitializeKeycloakSessionFactory(); - List actualOfflineSessionIds = withRealm(realmId, (session, realm) -> session.users().getUsersStream(realm).flatMap(user -> - session.sessions().getOfflineUserSessionsStream(realm, user)).map(UserSessionModel::getId).collect(Collectors.toList())); + Map> actualOfflineSessionIds = withRealm(realmId, (session, realm) -> session.users() + .getUsersStream(realm) + .collect(Collectors.toMap( + UserModel::getId, + user -> session.sessions().getOfflineUserSessionsStream(realm, user).map(UserSessionModel::getId).collect(Collectors.toCollection(TreeSet::new)) + )) + ); - assertThat(actualOfflineSessionIds, containsInAnyOrder(offlineSessionIds.toArray())); + assertThat("User IDs", actualOfflineSessionIds.keySet(), equalTo(offlineSessionIdsDetailed.keySet())); + for (Entry> me : offlineSessionIdsDetailed.entrySet()) { + assertThat("Session IDs", actualOfflineSessionIds.get(me.getKey()), equalTo(me.getValue())); + } } private String createOfflineClientSession(String offlineUserSessionId, String clientId) { @@ -408,6 +421,16 @@ private List createOfflineSessions(String realmId, List userIds) ); } + private Map> createOfflineSessionsDetailed(String realmId, List userIds) { + return withRealm(realmId, (session, realm) -> + userIds.stream() + .collect(Collectors.toMap( + Function.identity(), + userId -> createOfflineSessions(session, realm, userId, us -> {}).map(UserSessionModel::getId).collect(Collectors.toCollection(TreeSet::new)) + )) + ); + } + /** * Creates {@link #OFFLINE_SESSION_COUNT_PER_USER} offline sessions for {@code userId} user. */