From c433b01ee56568b5f8cccd2c72902894d6d02740 Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Mon, 16 Apr 2018 09:40:13 +0200 Subject: [PATCH] Optimize session retrieval in JdbcOperationsSessionRepository Previously, SessionResultSetExtractor used JdbcSession.setAttribute which had a side effect of freshly loaded session potentially having a non-empty delta and/or changed flag set. This commit optimizes session retrieval to invoke setAttribute directly on the delegate, therefore preventing unnecessary modifications of delta and change flags. Closes gh-1053 --- ...JdbcOperationsSessionRepositoryITests.java | 61 +++++++++++++++++-- .../jdbc/JdbcOperationsSessionRepository.java | 43 +++++++------ .../JdbcOperationsSessionRepositoryTests.java | 14 +++-- 3 files changed, 84 insertions(+), 34 deletions(-) diff --git a/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java b/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java index 851e3297..d2023784 100644 --- a/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java +++ b/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -34,10 +34,8 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; -import org.springframework.session.ExpiringSession; import org.springframework.session.FindByIndexNameSessionRepository; import org.springframework.session.MapSession; -import org.springframework.session.Session; import org.springframework.session.jdbc.config.annotation.web.http.EnableJdbcHttpSession; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @@ -82,6 +80,19 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { AuthorityUtils.createAuthorityList("ROLE_USER"))); } + @Test + public void saveWhenNoAttributesThenCanBeFound() { + JdbcOperationsSessionRepository.JdbcSession toSave = this.repository + .createSession(); + + this.repository.save(toSave); + JdbcOperationsSessionRepository.JdbcSession session = this.repository.getSession(toSave.getId()); + + assertThat(session).isNotNull(); + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } + @Test public void saves() throws InterruptedException { String username = "saves-" + System.currentTimeMillis(); @@ -100,9 +111,11 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { this.repository.save(toSave); - Session session = this.repository.getSession(toSave.getId()); + JdbcOperationsSessionRepository.JdbcSession session = this.repository.getSession(toSave.getId()); assertThat(session.getId()).isEqualTo(toSave.getId()); + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); assertThat(session.getAttributeNames()).isEqualTo(toSave.getAttributeNames()); assertThat(session.getAttribute(expectedAttributeName)) .isEqualTo(toSave.getAttribute(expectedAttributeName)); @@ -135,7 +148,9 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { this.repository.save(toSave); toSave = this.repository.getSession(toSave.getId()); - Session session = this.repository.getSession(toSave.getId()); + JdbcOperationsSessionRepository.JdbcSession session = this.repository.getSession(toSave.getId()); + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); assertThat(session.getAttributeNames().size()).isEqualTo(2); assertThat(session.getAttribute("a")).isEqualTo("b"); assertThat(session.getAttribute("1")).isEqualTo("2"); @@ -156,9 +171,11 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { toSave.setLastAccessedTime(lastAccessedTime); this.repository.save(toSave); - ExpiringSession session = this.repository.getSession(toSave.getId()); + JdbcOperationsSessionRepository.JdbcSession session = this.repository.getSession(toSave.getId()); assertThat(session).isNotNull(); + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); assertThat(session.isExpired()).isFalse(); assertThat(session.getLastAccessedTime()).isEqualTo(lastAccessedTime); } @@ -225,6 +242,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test @@ -247,6 +268,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test @@ -289,6 +314,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test @@ -336,6 +365,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test @@ -395,6 +428,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test @@ -416,6 +453,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test @@ -455,6 +496,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test @@ -499,6 +544,10 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { assertThat(findByPrincipalName).hasSize(1); assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId()); + for (JdbcOperationsSessionRepository.JdbcSession session : findByPrincipalName.values()) { + assertThat(session.isChanged()).isFalse(); + assertThat(session.getDelta()).isEmpty(); + } } @Test diff --git a/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java b/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java index 2bf90cfa..4ef1f6c1 100644 --- a/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java +++ b/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java @@ -188,8 +188,7 @@ public class JdbcOperationsSessionRepository implements private final TransactionOperations transactionOperations; - private final ResultSetExtractor> extractor = - new ExpiringSessionResultSetExtractor(); + private final ResultSetExtractor> extractor = new SessionResultSetExtractor(); /** * The name of database table used by Spring Session to store sessions. @@ -484,10 +483,10 @@ public class JdbcOperationsSessionRepository implements } public JdbcSession getSession(final String id) { - final ExpiringSession session = this.transactionOperations.execute(new TransactionCallback() { + final JdbcSession session = this.transactionOperations.execute(new TransactionCallback() { - public ExpiringSession doInTransaction(TransactionStatus status) { - List sessions = JdbcOperationsSessionRepository.this.jdbcOperations.query( + public JdbcSession doInTransaction(TransactionStatus status) { + List sessions = JdbcOperationsSessionRepository.this.jdbcOperations.query( JdbcOperationsSessionRepository.this.getSessionQuery, new PreparedStatementSetter() { @@ -511,7 +510,7 @@ public class JdbcOperationsSessionRepository implements delete(id); } else { - return new JdbcSession(session); + return session; } } return null; @@ -534,9 +533,9 @@ public class JdbcOperationsSessionRepository implements return Collections.emptyMap(); } - List sessions = this.transactionOperations.execute(new TransactionCallback>() { + List sessions = this.transactionOperations.execute(new TransactionCallback>() { - public List doInTransaction(TransactionStatus status) { + public List doInTransaction(TransactionStatus status) { return JdbcOperationsSessionRepository.this.jdbcOperations.query( JdbcOperationsSessionRepository.this.listSessionsByPrincipalNameQuery, new PreparedStatementSetter() { @@ -555,8 +554,8 @@ public class JdbcOperationsSessionRepository implements Map sessionMap = new HashMap( sessions.size()); - for (ExpiringSession session : sessions) { - sessionMap.put(session.getId(), new JdbcSession(session)); + for (JdbcSession session : sessions) { + sessionMap.put(session.getId(), session); } return sessionMap; @@ -764,33 +763,33 @@ public class JdbcOperationsSessionRepository implements } - private class ExpiringSessionResultSetExtractor - implements ResultSetExtractor> { + private class SessionResultSetExtractor implements ResultSetExtractor> { - public List extractData(ResultSet rs) throws SQLException, DataAccessException { - List sessions = new ArrayList(); + public List extractData(ResultSet rs) throws SQLException, DataAccessException { + List sessions = new ArrayList(); while (rs.next()) { String id = rs.getString("SESSION_ID"); - MapSession session; + JdbcSession session; if (sessions.size() > 0 && getLast(sessions).getId().equals(id)) { - session = (MapSession) getLast(sessions); + session = getLast(sessions); } else { - session = new MapSession(id); - session.setCreationTime(rs.getLong("CREATION_TIME")); - session.setLastAccessedTime(rs.getLong("LAST_ACCESS_TIME")); - session.setMaxInactiveIntervalInSeconds(rs.getInt("MAX_INACTIVE_INTERVAL")); + MapSession delegate = new MapSession(id); + delegate.setCreationTime(rs.getLong("CREATION_TIME")); + delegate.setLastAccessedTime(rs.getLong("LAST_ACCESS_TIME")); + delegate.setMaxInactiveIntervalInSeconds(rs.getInt("MAX_INACTIVE_INTERVAL")); + session = new JdbcSession(delegate); } String attributeName = rs.getString("ATTRIBUTE_NAME"); if (attributeName != null) { - session.setAttribute(attributeName, deserialize(rs, "ATTRIBUTE_BYTES")); + session.delegate.setAttribute(attributeName, deserialize(rs, "ATTRIBUTE_BYTES")); } sessions.add(session); } return sessions; } - private ExpiringSession getLast(List sessions) { + private JdbcSession getLast(List sessions) { return sessions.get(sessions.size() - 1); } diff --git a/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java b/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java index bfc8ff1d..6a85627d 100644 --- a/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java +++ b/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -412,7 +412,7 @@ public class JdbcOperationsSessionRepositoryTests { @Test public void getSessionExpired() { - MapSession expired = new MapSession(); + JdbcOperationsSessionRepository.JdbcSession expired = this.repository.new JdbcSession(); expired.setLastAccessedTime(System.currentTimeMillis() - (MapSession.DEFAULT_MAX_INACTIVE_INTERVAL_SECONDS * 1000 + 1000)); given(this.jdbcOperations.query(isA(String.class), @@ -432,7 +432,8 @@ public class JdbcOperationsSessionRepositoryTests { @Test public void getSessionFound() { - MapSession saved = new MapSession(); + JdbcOperationsSessionRepository.JdbcSession saved = this.repository.new JdbcSession( + new MapSession()); saved.setAttribute("savedName", "savedValue"); given(this.jdbcOperations.query(isA(String.class), isA(PreparedStatementSetter.class), isA(ResultSetExtractor.class))) @@ -493,11 +494,12 @@ public class JdbcOperationsSessionRepositoryTests { String principal = "username"; Authentication authentication = new UsernamePasswordAuthenticationToken(principal, "notused", AuthorityUtils.createAuthorityList("ROLE_USER")); - List saved = new ArrayList(2); - MapSession saved1 = new MapSession(); + List saved = + new ArrayList(2); + JdbcOperationsSessionRepository.JdbcSession saved1 = this.repository.new JdbcSession(); saved1.setAttribute(SPRING_SECURITY_CONTEXT, authentication); saved.add(saved1); - MapSession saved2 = new MapSession(); + JdbcOperationsSessionRepository.JdbcSession saved2 = this.repository.new JdbcSession(); saved2.setAttribute(SPRING_SECURITY_CONTEXT, authentication); saved.add(saved2); given(this.jdbcOperations.query(isA(String.class),