Fold DefaultWebSession within InMemoryWebSessionStore

InMemoryWebSessionStore is very closely associated to DefaultWebSession
passing it to it several fields and functions. Now that the store also
creates the session, it makes sense to bring the latter in as an inner,
nested class.

Issue: SPR-15875, 15876

spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSession.java

@@ -1,176 +0,0 @@
-/*
- * Copyright 2002-2017 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.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.springframework.web.server.session;
-
-import java.time.Clock;
-import java.time.Duration;
-import java.time.Instant;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicReference;
-import java.util.function.BiFunction;
-import java.util.function.Function;
-
-import reactor.core.publisher.Mono;
-
-import org.springframework.util.Assert;
-import org.springframework.util.IdGenerator;
-import org.springframework.web.server.WebSession;
-
-/**
- * Default implementation of {@link org.springframework.web.server.WebSession}.
- *
- * @author Rossen Stoyanchev
- * @since 5.0
- */
-class DefaultWebSession implements WebSession {
-
-	private final AtomicReference<String> id;
-
-	private final IdGenerator idGenerator;
-
-	private final Map<String, Object> attributes;
-
-	private final Clock clock;
-
-	private final BiFunction<String, WebSession, Mono<Void>> changeIdOperation;
-
-	private final Function<WebSession, Mono<Void>> saveOperation;
-
-	private final Instant creationTime;
-
-	private final Instant lastAccessTime;
-
-	private volatile Duration maxIdleTime;
-
-	private volatile State state;
-
-
-	/**
-	 * Constructor for creating a new session instance.
-	 * @param idGenerator the session id generator
-	 * @param clock for access to current time
-	 */
-	DefaultWebSession(IdGenerator idGenerator, Clock clock,
-			BiFunction<String, WebSession, Mono<Void>> changeIdOperation,
-			Function<WebSession, Mono<Void>> saveOperation) {
-
-		Assert.notNull(idGenerator, "'idGenerator' is required.");
-		Assert.notNull(clock, "'clock' is required.");
-		Assert.notNull(changeIdOperation, "'changeIdOperation' is required.");
-		Assert.notNull(saveOperation, "'saveOperation' is required.");
-
-		this.id = new AtomicReference<>(String.valueOf(idGenerator.generateId()));
-		this.idGenerator = idGenerator;
-		this.clock = clock;
-		this.changeIdOperation = changeIdOperation;
-		this.saveOperation = saveOperation;
-		this.attributes = new ConcurrentHashMap<>();
-		this.creationTime = Instant.now(clock);
-		this.lastAccessTime = this.creationTime;
-		this.maxIdleTime = Duration.ofMinutes(30);
-		this.state = State.NEW;
-	}
-
-	/**
-	 * Copy constructor to re-create a session at the start of a new request
-	 * refreshing the last access time of the session.
-	 * @param existingSession the existing session to copy
-	 * @param lastAccessTime the new last access time
-	 */
-	DefaultWebSession(DefaultWebSession existingSession, Instant lastAccessTime) {
-		this.id = existingSession.id;
-		this.idGenerator = existingSession.idGenerator;
-		this.attributes = existingSession.attributes;
-		this.clock = existingSession.clock;
-		this.changeIdOperation = existingSession.changeIdOperation;
-		this.saveOperation = existingSession.saveOperation;
-		this.creationTime = existingSession.creationTime;
-		this.lastAccessTime = lastAccessTime;
-		this.maxIdleTime = existingSession.maxIdleTime;
-		this.state = existingSession.isStarted() ? State.STARTED : State.NEW;
-	}
-
-
-	@Override
-	public String getId() {
-		return this.id.get();
-	}
-
-	@Override
-	public Map<String, Object> getAttributes() {
-		return this.attributes;
-	}
-
-	@Override
-	public Instant getCreationTime() {
-		return this.creationTime;
-	}
-
-	@Override
-	public Instant getLastAccessTime() {
-		return this.lastAccessTime;
-	}
-
-	/**
-	 * <p>By default this is set to 30 minutes.
-	 * @param maxIdleTime the max idle time
-	 */
-	@Override
-	public void setMaxIdleTime(Duration maxIdleTime) {
-		this.maxIdleTime = maxIdleTime;
-	}
-
-	@Override
-	public Duration getMaxIdleTime() {
-		return this.maxIdleTime;
-	}
-
-
-	@Override
-	public void start() {
-		this.state = State.STARTED;
-	}
-
-	@Override
-	public boolean isStarted() {
-		State value = this.state;
-		return (State.STARTED.equals(value) || (State.NEW.equals(value) && !getAttributes().isEmpty()));
-	}
-
-	@Override
-	public Mono<Void> changeSessionId() {
-		String oldId = this.id.get();
-		String newId = String.valueOf(this.idGenerator.generateId());
-		this.id.set(newId);
-		return this.changeIdOperation.apply(oldId, this).doOnError(ex -> this.id.set(oldId));
-	}
-
-	@Override
-	public Mono<Void> save() {
-		return this.saveOperation.apply(this);
-	}
-
-	@Override
-	public boolean isExpired() {
-		return (isStarted() && !this.maxIdleTime.isNegative() &&
-				Instant.now(this.clock).minus(this.maxIdleTime).isAfter(this.lastAccessTime));
-	}
-
-
-	private enum State { NEW, STARTED }
-
-}

spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java

@@ -16,16 +16,18 @@
 package org.springframework.web.server.session;
 
 import java.time.Clock;
+import java.time.Duration;
 import java.time.Instant;
 import java.time.ZoneId;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicReference;
+
+import reactor.core.publisher.Mono;
 
 import org.springframework.util.Assert;
 import org.springframework.util.IdGenerator;
 import org.springframework.util.JdkIdGenerator;
-import reactor.core.publisher.Mono;
-
 import org.springframework.web.server.WebSession;
 
 /**
@@ -67,6 +69,11 @@ public class InMemoryWebSessionStore implements WebSessionStore {
 	}
 
 
+	@Override
+	public Mono<WebSession> createWebSession() {
+		return Mono.fromSupplier(InMemoryWebSession::new);
+	}
+
 	@Override
 	public Mono<WebSession> retrieveSession(String id) {
 		return (this.sessions.containsKey(id) ? Mono.just(this.sessions.get(id)) : Mono.empty());
@@ -78,21 +85,16 @@ public class InMemoryWebSessionStore implements WebSessionStore {
 		return Mono.empty();
 	}
 
-	public Mono<WebSession> createWebSession() {
-		return Mono.fromSupplier(() ->
-				new DefaultWebSession(idGenerator, getClock(),
-						(oldId, session) -> this.changeSessionId(oldId, session),
-						this::storeSession));
-	}
-
 	public Mono<WebSession> updateLastAccessTime(WebSession webSession) {
 		return Mono.fromSupplier(() -> {
-			DefaultWebSession session = (DefaultWebSession) webSession;
+			InMemoryWebSession session = (InMemoryWebSession) webSession;
 			Instant lastAccessTime = Instant.now(getClock());
-			return new DefaultWebSession(session, lastAccessTime);
+			return new InMemoryWebSession(session, lastAccessTime);
 		});
 	}
 
+	/* Private methods for InMemoryWebSession */
+
 	private Mono<Void> changeSessionId(String oldId, WebSession session) {
 		this.sessions.remove(oldId);
 		this.sessions.put(session.getId(), session);
@@ -103,4 +105,100 @@ public class InMemoryWebSessionStore implements WebSessionStore {
 		this.sessions.put(session.getId(), session);
 		return Mono.empty();
 	}
+
+
+	private class InMemoryWebSession implements WebSession {
+
+		private final AtomicReference<String> id;
+
+		private final Map<String, Object> attributes;
+
+		private final Instant creationTime;
+
+		private final Instant lastAccessTime;
+
+		private volatile Duration maxIdleTime;
+
+		private volatile boolean started;
+
+
+		InMemoryWebSession() {
+			this.id = new AtomicReference<>(String.valueOf(idGenerator.generateId()));
+			this.attributes = new ConcurrentHashMap<>();
+			this.creationTime = Instant.now(getClock());
+			this.lastAccessTime = this.creationTime;
+			this.maxIdleTime = Duration.ofMinutes(30);
+		}
+
+		InMemoryWebSession(InMemoryWebSession existingSession, Instant lastAccessTime) {
+			this.id = existingSession.id;
+			this.attributes = existingSession.attributes;
+			this.creationTime = existingSession.creationTime;
+			this.lastAccessTime = lastAccessTime;
+			this.maxIdleTime = existingSession.maxIdleTime;
+			this.started = existingSession.isStarted(); // Use method (explicit or implicit start)
+		}
+
+
+		@Override
+		public String getId() {
+			return this.id.get();
+		}
+
+		@Override
+		public Map<String, Object> getAttributes() {
+			return this.attributes;
+		}
+
+		@Override
+		public Instant getCreationTime() {
+			return this.creationTime;
+		}
+
+		@Override
+		public Instant getLastAccessTime() {
+			return this.lastAccessTime;
+		}
+
+		@Override
+		public void setMaxIdleTime(Duration maxIdleTime) {
+			this.maxIdleTime = maxIdleTime;
+		}
+
+		@Override
+		public Duration getMaxIdleTime() {
+			return this.maxIdleTime;
+		}
+
+
+		@Override
+		public void start() {
+			this.started = true;
+		}
+
+		@Override
+		public boolean isStarted() {
+			return this.started || !getAttributes().isEmpty();
+		}
+
+		@Override
+		public Mono<Void> changeSessionId() {
+			String oldId = this.id.get();
+			String newId = String.valueOf(idGenerator.generateId());
+			this.id.set(newId);
+			return InMemoryWebSessionStore.this.changeSessionId(oldId, this).doOnError(ex -> this.id.set(oldId));
+		}
+
+		@Override
+		public Mono<Void> save() {
+			return InMemoryWebSessionStore.this.storeSession(this);
+		}
+
+		@Override
+		public boolean isExpired() {
+			return (isStarted() && !this.maxIdleTime.isNegative() &&
+					Instant.now(getClock()).minus(this.maxIdleTime).isAfter(this.lastAccessTime));
+		}
+	}
+
 }

spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java

@@ -16,59 +16,58 @@
 package org.springframework.web.server.session;
 
 import org.junit.Test;
-import org.springframework.util.IdGenerator;
-import org.springframework.util.JdkIdGenerator;
-import reactor.core.publisher.Mono;
 
-import java.time.Clock;
-import java.time.ZoneId;
+import org.springframework.web.server.WebSession;
 
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 /**
+ * Unit tests.
  * @author Rob Winch
- * @since 5.0
  */
-public class DefaultWebSessionTests {
-	private static final Clock CLOCK = Clock.system(ZoneId.of("GMT"));
+public class InMemoryWebSessionStoreTests {
+
+	private InMemoryWebSessionStore sessionStore = new InMemoryWebSessionStore();
 
-	private static final IdGenerator idGenerator = new JdkIdGenerator();
 
 	@Test
 	public void constructorWhenImplicitStartCopiedThenCopyIsStarted() {
-		DefaultWebSession original = createDefaultWebSession();
+		WebSession original = this.sessionStore.createWebSession().block();
+		assertNotNull(original);
 		original.getAttributes().put("foo", "bar");
 
-		DefaultWebSession copy = new DefaultWebSession(original, CLOCK.instant());
-
+		WebSession copy = this.sessionStore.updateLastAccessTime(original).block();
+		assertNotNull(copy);
 		assertTrue(copy.isStarted());
 	}
 
 	@Test
 	public void constructorWhenExplicitStartCopiedThenCopyIsStarted() {
-		DefaultWebSession original = createDefaultWebSession();
+		WebSession original = this.sessionStore.createWebSession().block();
+		assertNotNull(original);
 		original.start();
 
-		DefaultWebSession copy = new DefaultWebSession(original, CLOCK.instant());
-
+		WebSession copy = this.sessionStore.updateLastAccessTime(original).block();
+		assertNotNull(copy);
 		assertTrue(copy.isStarted());
 	}
 
 	@Test
 	public void startsSessionExplicitly() {
-		DefaultWebSession session = createDefaultWebSession();
+		WebSession session = this.sessionStore.createWebSession().block();
+		assertNotNull(session);
 		session.start();
 		assertTrue(session.isStarted());
 	}
 
 	@Test
 	public void startsSessionImplicitly() {
-		DefaultWebSession session = createDefaultWebSession();
+		WebSession session = this.sessionStore.createWebSession().block();
+		assertNotNull(session);
+		session.start();
 		session.getAttributes().put("foo", "bar");
 		assertTrue(session.isStarted());
 	}
 
-	private DefaultWebSession createDefaultWebSession() {
-		return new DefaultWebSession(idGenerator, CLOCK, (s, session) -> Mono.empty(), s -> Mono.empty());
-	}
 }

spring-web/src/test/java/org/springframework/web/server/session/WebSessionIntegrationTests.java

@@ -111,11 +111,9 @@ public class WebSessionIntegrationTests extends AbstractHttpHandlerIntegrationTe
 
 		// Now set the clock of the session back by 31 minutes
 		InMemoryWebSessionStore store = (InMemoryWebSessionStore) this.sessionManager.getSessionStore();
-		DefaultWebSession session = (DefaultWebSession) store.retrieveSession(id).block();
+		WebSession session = store.retrieveSession(id).block();
 		assertNotNull(session);
-		Instant lastAccessTime = Clock.offset(store.getClock(), Duration.ofMinutes(-31)).instant();
-		session = new DefaultWebSession(session, lastAccessTime);
-		session.save().block();
+		store.setClock(Clock.offset(store.getClock(), Duration.ofMinutes(31)));
 
 		// Third request: expired session, new session created
 		request = RequestEntity.get(createUri()).header("Cookie", "SESSION=" + id).build();