From 49f24e493d56b0cee64254eb7cdad3bcc8f913c9 Mon Sep 17 00:00:00 2001
From: Erin Drummond <erin.drummond@tsbbank.co.nz>
Date: Fri, 11 Mar 2016 11:21:49 +1300
Subject: [PATCH] Fix JdbcOperationsSessionRepository session cleanup

Make JDBC session cleanup logic actually cleanup expired sessions
as opposed to all sessions

Fixes gh-421
---
 ...JdbcOperationsSessionRepositoryITests.java | 42 +++++++++++++++++++
 .../jdbc/JdbcOperationsSessionRepository.java | 23 +++++-----
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/spring-session/src/integration-test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryITests.java b/spring-session/src/integration-test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryITests.java
index 38f1837..0c0b722 100644
--- a/spring-session/src/integration-test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryITests.java
+++ b/spring-session/src/integration-test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryITests.java
@@ -473,6 +473,48 @@ public class JdbcOperationsSessionRepositoryITests {
 		assertThat(findByPrincipalName.keySet()).containsOnly(toSave.getId());
 	}
 
+	@Test
+	public void cleanupCleansUpInactiveSessions() {
+		this.repository.setDefaultMaxInactiveInterval(1800); // 30 minutes
+
+		JdbcOperationsSessionRepository.JdbcSession session = this.repository
+				.createSession();
+
+		this.repository.save(session);
+
+		assertThat(this.repository.getSession(session.getId())).isNotNull();
+
+		this.repository.cleanUpExpiredSessions();
+
+		// verify its not cleaned up immediately after creation
+		assertThat(this.repository.getSession(session.getId())).isNotNull();
+
+		long now = System.currentTimeMillis();
+		long tenMinutesInMilliseconds = 1000 * 60 * 10;
+		long thirtyMinutesInMilliseconds = 1000 * 60 * 30;
+
+		// set the last accessed time to 10 minutes in the past, session should not get
+		// cleaned up because it has not been inactive for 30 minutes yet
+		long tenMinutesInThePast = now - tenMinutesInMilliseconds;
+		session.setLastAccessedTime(tenMinutesInThePast);
+		this.repository.save(session);
+
+		this.repository.cleanUpExpiredSessions();
+
+		// session should still exist
+		assertThat(this.repository.getSession(session.getId())).isNotNull();
+
+		// set the last accessed time to 30 minutes in the past, session should get
+		// cleaned up as it has been inactive for 30 minutes
+		long thirtyMinutesInThePast = now - thirtyMinutesInMilliseconds;
+		session.setLastAccessedTime(thirtyMinutesInThePast);
+		this.repository.save(session);
+
+		this.repository.cleanUpExpiredSessions();
+		// session should have been cleaned up
+		assertThat(this.repository.getSession(session.getId())).isNull();
+	}
+
 	private String getSecurityName() {
 		return this.context.getAuthentication().getName();
 	}
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 829bd79..cd7b386 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
@@ -19,7 +19,6 @@ package org.springframework.session.jdbc;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
-import java.util.Calendar;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
@@ -302,14 +301,20 @@ public class JdbcOperationsSessionRepository implements
 	@Scheduled(cron = "0 * * * * *")
 	public void cleanUpExpiredSessions() {
 		long now = System.currentTimeMillis();
-		long roundedNow = roundDownMinute(now);
+		int maxInactiveIntervalSeconds = (this.defaultMaxInactiveInterval != null)
+				? this.defaultMaxInactiveInterval
+				: MapSession.DEFAULT_MAX_INACTIVE_INTERVAL_SECONDS;
+
+		long sessionsValidFromTime = now - (maxInactiveIntervalSeconds * 1000);
 
 		if (logger.isDebugEnabled()) {
-			logger.debug("Cleaning up sessions expiring at " + new Date(roundedNow));
+			logger.debug(
+					"Cleaning up sessions older than " + new Date(sessionsValidFromTime));
 		}
 
-		int deletedCount = this.jdbcOperations
-				.update(getQuery(DELETE_SESSIONS_BY_LAST_ACCESS_TIME_QUERY), roundedNow);
+		int deletedCount = this.jdbcOperations.update(
+				getQuery(DELETE_SESSIONS_BY_LAST_ACCESS_TIME_QUERY),
+				sessionsValidFromTime);
 
 		if (logger.isDebugEnabled()) {
 			logger.debug("Cleaned up " + deletedCount + " expired sessions");
@@ -332,14 +337,6 @@ public class JdbcOperationsSessionRepository implements
 				TypeDescriptor.valueOf(byte[].class));
 	}
 
-	private static long roundDownMinute(long timeInMs) {
-		Calendar date = Calendar.getInstance();
-		date.setTimeInMillis(timeInMs);
-		date.clear(Calendar.SECOND);
-		date.clear(Calendar.MILLISECOND);
-		return date.getTimeInMillis();
-	}
-
 	private static GenericConversionService createDefaultConversionService() {
 		GenericConversionService converter = new GenericConversionService();
 		converter.addConverter(ExpiringSession.class, byte[].class,