From 831d2f4152a532c1f68bdcbc08352d86efbe8e40 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=98yvind=20Horneland?= <oyvind.horneland@acando.no>
Date: Mon, 14 Mar 2016 15:08:19 +0100
Subject: [PATCH] SessionRepositoryFilter caches null session lookup

If a session cannot be found by id, we will cache that result for any
subsequent calls to getSession(false) for the duration of this request.

Fixes gh-423
---
 .../web/http/SessionRepositoryFilter.java     | 13 +++++++-
 .../http/SessionRepositoryFilterTests.java    | 30 +++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/spring-session/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java b/spring-session/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java
index 2f0def1..10851ab 100644
--- a/spring-session/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java
+++ b/spring-session/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java
@@ -87,6 +87,11 @@ public class SessionRepositoryFilter<S extends ExpiringSession>
 	public static final String SESSION_REPOSITORY_ATTR = SessionRepository.class
 			.getName();
 
+	/**
+	 * Invalid session id (not backed by the session repository) request attribute name.
+	 */
+	public static final String INVALID_SESSION_ID_ATTR = SESSION_REPOSITORY_ATTR + ".invalidSessionId";
+
 	/**
 	 * The default filter order.
 	 */
@@ -330,7 +335,7 @@ public class SessionRepositoryFilter<S extends ExpiringSession>
 				return currentSession;
 			}
 			String requestedSessionId = getRequestedSessionId();
-			if (requestedSessionId != null) {
+			if (requestedSessionId != null && getAttribute(INVALID_SESSION_ID_ATTR) == null) {
 				S session = getSession(requestedSessionId);
 				if (session != null) {
 					this.requestedSessionIdValid = true;
@@ -338,6 +343,12 @@ public class SessionRepositoryFilter<S extends ExpiringSession>
 					currentSession.setNew(false);
 					setCurrentSession(currentSession);
 					return currentSession;
+				} else {
+					// This is an invalid session id. No need to ask again if request.getSession is invoked for the duration of this request
+					if (SESSION_LOGGER.isDebugEnabled()) {
+						SESSION_LOGGER.debug("No session found by id: Caching result for getSession(false) for this HttpServletRequest.");
+					}
+					setAttribute(INVALID_SESSION_ID_ATTR, "true");
 				}
 			}
 			if (!create) {
diff --git a/spring-session/src/test/java/org/springframework/session/web/http/SessionRepositoryFilterTests.java b/spring-session/src/test/java/org/springframework/session/web/http/SessionRepositoryFilterTests.java
index 7e677e0..179e571 100644
--- a/spring-session/src/test/java/org/springframework/session/web/http/SessionRepositoryFilterTests.java
+++ b/spring-session/src/test/java/org/springframework/session/web/http/SessionRepositoryFilterTests.java
@@ -64,6 +64,7 @@ import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.internal.verification.VerificationModeFactory.times;
 
 @RunWith(MockitoJUnitRunner.class)
 @SuppressWarnings("deprecation")
@@ -1341,6 +1342,35 @@ public class SessionRepositoryFilterTests {
 		this.filter.setHttpSessionStrategy((MultiHttpSessionStrategy) null);
 	}
 
+	@Test
+	public void getSessionFalseWithInvalidSessionIdShouldOnlyAskRepositoryOnce() throws ServletException, IOException {
+		this.sessionRepository = spy(this.sessionRepository);
+		this.filter = new SessionRepositoryFilter<ExpiringSession>(this.sessionRepository);
+
+		final String nonExistantSessionId = "nonExistantSessionId";
+		setSessionCookie(nonExistantSessionId);
+
+		doFilter(new DoInFilter() {
+			@Override
+			public void doFilter(HttpServletRequest wrappedRequest) {
+				// Before first invocation
+				assertThat(SessionRepositoryFilterTests.this.request.getAttribute(SessionRepositoryFilter.INVALID_SESSION_ID_ATTR)).isNull();
+
+				// First call should go all the way through to the sessioRepository (it will not find the session)
+				HttpSession session = wrappedRequest.getSession(false);
+				verify(sessionRepository, times(1)).getSession(nonExistantSessionId);
+				assertThat(session).isNull();
+				assertThat(SessionRepositoryFilterTests.this.request.getAttribute(SessionRepositoryFilter.INVALID_SESSION_ID_ATTR)).isNotNull();
+
+				// Second call should not reach the sessionRepository
+				session = wrappedRequest.getSession(false);
+				verify(sessionRepository, times(1)).getSession(nonExistantSessionId); // still only called once
+				assertThat(session).isNull();
+				assertThat(SessionRepositoryFilterTests.this.request.getAttribute(SessionRepositoryFilter.INVALID_SESSION_ID_ATTR)).isNotNull();
+			}
+		});
+	}
+
 	// --- helper methods
 
 	private void assertNewSession() {