From a1ed020ba507f7b07431292b80d41eafd47af6c3 Mon Sep 17 00:00:00 2001 From: Ryan Rupp <3022260+ryanrupp@users.noreply.github.com> Date: Tue, 21 Jan 2025 18:19:12 -0600 Subject: [PATCH 1/3] Only start a new transaction if there's a delta for session updates with JDBC based repository Avoids eagerly acquiring a connection or any other eager initialization that may happen alongside a transaction starting if there's no actual update to be made. --- .../jdbc/JdbcIndexedSessionRepository.java | 62 +++++++++++-------- .../JdbcIndexedSessionRepositoryTests.java | 22 ++++++- 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java index 612f2527e..bcc196b34 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java @@ -907,45 +907,57 @@ private void save() { }); } else { - JdbcIndexedSessionRepository.this.transactionOperations.executeWithoutResult((status) -> { - if (JdbcSession.this.changed) { + List deltaActions = new ArrayList<>(); + if (JdbcSession.this.changed) { + deltaActions.add(() -> { Map indexes = JdbcIndexedSessionRepository.this.indexResolver - .resolveIndexesFor(JdbcSession.this); + .resolveIndexesFor(JdbcSession.this); JdbcIndexedSessionRepository.this.jdbcOperations - .update(JdbcIndexedSessionRepository.this.updateSessionQuery, (ps) -> { - ps.setString(1, getId()); - ps.setLong(2, getLastAccessedTime().toEpochMilli()); - ps.setInt(3, (int) getMaxInactiveInterval().getSeconds()); - ps.setLong(4, getExpiryTime().toEpochMilli()); - ps.setString(5, indexes.get(PRINCIPAL_NAME_INDEX_NAME)); - ps.setString(6, JdbcSession.this.primaryKey); - }); - } - List addedAttributeNames = JdbcSession.this.delta.entrySet() + .update(JdbcIndexedSessionRepository.this.updateSessionQuery, (ps) -> { + ps.setString(1, getId()); + ps.setLong(2, getLastAccessedTime().toEpochMilli()); + ps.setInt(3, (int) getMaxInactiveInterval().getSeconds()); + ps.setLong(4, getExpiryTime().toEpochMilli()); + ps.setString(5, indexes.get(PRINCIPAL_NAME_INDEX_NAME)); + ps.setString(6, JdbcSession.this.primaryKey); + }); + }); + } + + List addedAttributeNames = JdbcSession.this.delta.entrySet() .stream() .filter((entry) -> entry.getValue() == DeltaValue.ADDED) .map(Map.Entry::getKey) .collect(Collectors.toList()); - if (!addedAttributeNames.isEmpty()) { - insertSessionAttributes(JdbcSession.this, addedAttributeNames); - } - List updatedAttributeNames = JdbcSession.this.delta.entrySet() + if (!addedAttributeNames.isEmpty()) { + deltaActions.add(() -> insertSessionAttributes(JdbcSession.this, addedAttributeNames)); + } + + List updatedAttributeNames = JdbcSession.this.delta.entrySet() .stream() .filter((entry) -> entry.getValue() == DeltaValue.UPDATED) .map(Map.Entry::getKey) .collect(Collectors.toList()); - if (!updatedAttributeNames.isEmpty()) { - updateSessionAttributes(JdbcSession.this, updatedAttributeNames); - } - List removedAttributeNames = JdbcSession.this.delta.entrySet() + if (!updatedAttributeNames.isEmpty()) { + deltaActions.add(() -> updateSessionAttributes(JdbcSession.this, updatedAttributeNames)); + } + + List removedAttributeNames = JdbcSession.this.delta.entrySet() .stream() .filter((entry) -> entry.getValue() == DeltaValue.REMOVED) .map(Map.Entry::getKey) .collect(Collectors.toList()); - if (!removedAttributeNames.isEmpty()) { - deleteSessionAttributes(JdbcSession.this, removedAttributeNames); - } - }); + if (!removedAttributeNames.isEmpty()) { + deltaActions.add(() -> deleteSessionAttributes(JdbcSession.this, removedAttributeNames)); + } + + if (!deltaActions.isEmpty()) { + JdbcIndexedSessionRepository.this.transactionOperations.executeWithoutResult((status) -> { + for (Runnable action : deltaActions) { + action.run(); + } + }); + } } clearChangeFlags(); } diff --git a/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java b/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java index 7b60c5a4d..3d6379518 100644 --- a/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java +++ b/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.function.Consumer; import java.util.function.Supplier; import org.junit.jupiter.api.BeforeEach; @@ -48,10 +49,13 @@ import org.springframework.session.SaveMode; import org.springframework.session.Session; import org.springframework.session.jdbc.JdbcIndexedSessionRepository.JdbcSession; +import org.springframework.transaction.TransactionStatus; +import org.springframework.transaction.support.TransactionCallback; import org.springframework.transaction.support.TransactionOperations; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isA; @@ -59,9 +63,11 @@ import static org.mockito.ArgumentMatchers.startsWith; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; /** @@ -78,12 +84,25 @@ class JdbcIndexedSessionRepositoryTests { @Mock private JdbcOperations jdbcOperations; + @Mock + private TransactionOperations transactionOperations; + private JdbcIndexedSessionRepository repository; @BeforeEach void setUp() { + // Mock transaction callbacks to the real consumer + lenient().doAnswer(answer -> { + answer.getArgument(0, Consumer.class).accept(mock(TransactionStatus.class)); + return null; + }).when(transactionOperations).executeWithoutResult(any()); + + lenient().doAnswer(answer -> + answer.getArgument(0, TransactionCallback.class).doInTransaction(mock(TransactionStatus.class)) + ).when(transactionOperations).execute(any()); + this.repository = new JdbcIndexedSessionRepository(this.jdbcOperations, - TransactionOperations.withoutTransaction()); + transactionOperations); } @Test @@ -467,6 +486,7 @@ void saveUpdatedAddAndRemoveAttribute() { assertThat(session.isNew()).isFalse(); verifyNoMoreInteractions(this.jdbcOperations); + verifyNoMoreInteractions(this.transactionOperations); } @Test // gh-1070 From 0e64e824b43e3549a613b757072e50ef76af4b5f Mon Sep 17 00:00:00 2001 From: Ryan Rupp <3022260+ryanrupp@users.noreply.github.com> Date: Tue, 21 Jan 2025 18:28:21 -0600 Subject: [PATCH 2/3] Micro-optimization to try to right size the array --- .../session/jdbc/JdbcIndexedSessionRepository.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java index bcc196b34..826e23a1d 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java @@ -907,7 +907,7 @@ private void save() { }); } else { - List deltaActions = new ArrayList<>(); + List deltaActions = JdbcSession.this.changed ? new ArrayList<>(4) : new ArrayList<>(); if (JdbcSession.this.changed) { deltaActions.add(() -> { Map indexes = JdbcIndexedSessionRepository.this.indexResolver From b516013defbc6b06c039a88be970ab14198db797 Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Tue, 22 Apr 2025 10:20:43 -0500 Subject: [PATCH 3/3] Fix Formatting & Checkstyle Signed-off-by: Rob Winch <362503+rwinch@users.noreply.github.com> --- .../jdbc/JdbcIndexedSessionRepository.java | 42 +++++++++---------- .../JdbcIndexedSessionRepositoryTests.java | 16 +++---- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java index 826e23a1d..b801672e5 100644 --- a/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java +++ b/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java @@ -911,42 +911,42 @@ private void save() { if (JdbcSession.this.changed) { deltaActions.add(() -> { Map indexes = JdbcIndexedSessionRepository.this.indexResolver - .resolveIndexesFor(JdbcSession.this); + .resolveIndexesFor(JdbcSession.this); JdbcIndexedSessionRepository.this.jdbcOperations - .update(JdbcIndexedSessionRepository.this.updateSessionQuery, (ps) -> { - ps.setString(1, getId()); - ps.setLong(2, getLastAccessedTime().toEpochMilli()); - ps.setInt(3, (int) getMaxInactiveInterval().getSeconds()); - ps.setLong(4, getExpiryTime().toEpochMilli()); - ps.setString(5, indexes.get(PRINCIPAL_NAME_INDEX_NAME)); - ps.setString(6, JdbcSession.this.primaryKey); - }); + .update(JdbcIndexedSessionRepository.this.updateSessionQuery, (ps) -> { + ps.setString(1, getId()); + ps.setLong(2, getLastAccessedTime().toEpochMilli()); + ps.setInt(3, (int) getMaxInactiveInterval().getSeconds()); + ps.setLong(4, getExpiryTime().toEpochMilli()); + ps.setString(5, indexes.get(PRINCIPAL_NAME_INDEX_NAME)); + ps.setString(6, JdbcSession.this.primaryKey); + }); }); } List addedAttributeNames = JdbcSession.this.delta.entrySet() - .stream() - .filter((entry) -> entry.getValue() == DeltaValue.ADDED) - .map(Map.Entry::getKey) - .collect(Collectors.toList()); + .stream() + .filter((entry) -> entry.getValue() == DeltaValue.ADDED) + .map(Map.Entry::getKey) + .collect(Collectors.toList()); if (!addedAttributeNames.isEmpty()) { deltaActions.add(() -> insertSessionAttributes(JdbcSession.this, addedAttributeNames)); } List updatedAttributeNames = JdbcSession.this.delta.entrySet() - .stream() - .filter((entry) -> entry.getValue() == DeltaValue.UPDATED) - .map(Map.Entry::getKey) - .collect(Collectors.toList()); + .stream() + .filter((entry) -> entry.getValue() == DeltaValue.UPDATED) + .map(Map.Entry::getKey) + .collect(Collectors.toList()); if (!updatedAttributeNames.isEmpty()) { deltaActions.add(() -> updateSessionAttributes(JdbcSession.this, updatedAttributeNames)); } List removedAttributeNames = JdbcSession.this.delta.entrySet() - .stream() - .filter((entry) -> entry.getValue() == DeltaValue.REMOVED) - .map(Map.Entry::getKey) - .collect(Collectors.toList()); + .stream() + .filter((entry) -> entry.getValue() == DeltaValue.REMOVED) + .map(Map.Entry::getKey) + .collect(Collectors.toList()); if (!removedAttributeNames.isEmpty()) { deltaActions.add(() -> deleteSessionAttributes(JdbcSession.this, removedAttributeNames)); } diff --git a/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java b/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java index 3d6379518..9bc4dc93c 100644 --- a/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java +++ b/spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcIndexedSessionRepositoryTests.java @@ -67,7 +67,6 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; /** @@ -92,17 +91,18 @@ class JdbcIndexedSessionRepositoryTests { @BeforeEach void setUp() { // Mock transaction callbacks to the real consumer - lenient().doAnswer(answer -> { + lenient().doAnswer((answer) -> { answer.getArgument(0, Consumer.class).accept(mock(TransactionStatus.class)); return null; - }).when(transactionOperations).executeWithoutResult(any()); + }).when(this.transactionOperations).executeWithoutResult(any()); - lenient().doAnswer(answer -> - answer.getArgument(0, TransactionCallback.class).doInTransaction(mock(TransactionStatus.class)) - ).when(transactionOperations).execute(any()); + lenient() + .doAnswer((answer) -> answer.getArgument(0, TransactionCallback.class) + .doInTransaction(mock(TransactionStatus.class))) + .when(this.transactionOperations) + .execute(any()); - this.repository = new JdbcIndexedSessionRepository(this.jdbcOperations, - transactionOperations); + this.repository = new JdbcIndexedSessionRepository(this.jdbcOperations, this.transactionOperations); } @Test