diff --git a/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSession.java b/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSession.java index 152c602c0a..74c4925cd5 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSession.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSession.java @@ -86,28 +86,9 @@ class DefaultWebSession implements WebSession { } /** - * Constructor to refresh an existing session for a new request. - * @param existingSession the session to recreate - * @param lastAccessTime the last access time - * @param saveOperation save operation for the current request - */ - DefaultWebSession(DefaultWebSession existingSession, Instant lastAccessTime, - Function> saveOperation) { - - this.id = existingSession.id; - this.idGenerator = existingSession.idGenerator; - this.attributes = existingSession.attributes; - this.clock = existingSession.clock; - this.changeIdOperation = existingSession.changeIdOperation; - this.saveOperation = saveOperation; - this.creationTime = existingSession.creationTime; - this.lastAccessTime = lastAccessTime; - this.maxIdleTime = existingSession.maxIdleTime; - this.state = existingSession.state; - } - - /** - * For testing purposes. + * Constructor for creating a new session with an updated last access time. + * @param existingSession the existing session to copy + * @param lastAccessTime the new last access time */ DefaultWebSession(DefaultWebSession existingSession, Instant lastAccessTime) { this.id = existingSession.id; @@ -119,7 +100,7 @@ class DefaultWebSession implements WebSession { this.creationTime = existingSession.creationTime; this.lastAccessTime = lastAccessTime; this.maxIdleTime = existingSession.maxIdleTime; - this.state = existingSession.state; + this.state = existingSession.isStarted() ? State.STARTED : State.NEW; } diff --git a/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java b/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java index 1d1ce7f58a..4a1cc17b58 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java @@ -15,9 +15,6 @@ */ package org.springframework.web.server.session; -import java.time.Clock; -import java.time.Instant; -import java.time.ZoneId; import java.util.List; import reactor.core.publisher.Flux; @@ -43,9 +40,6 @@ public class DefaultWebSessionManager implements WebSessionManager { private WebSessionStore sessionStore = new InMemoryWebSessionStore(); - private Clock clock = Clock.system(ZoneId.of("GMT")); - - /** * Configure the id resolution strategy. *

By default an instance of {@link CookieWebSessionIdResolver}. @@ -80,38 +74,14 @@ public class DefaultWebSessionManager implements WebSessionManager { return this.sessionStore; } - /** - * Configure the {@link Clock} to use to set lastAccessTime on every created - * session and to calculate if it is expired. - *

This may be useful to align to different timezone or to set the clock - * back in a test, e.g. {@code Clock.offset(clock, Duration.ofMinutes(-31))} - * in order to simulate session expiration. - *

By default this is {@code Clock.system(ZoneId.of("GMT"))}. - * @param clock the clock to use - */ - public void setClock(Clock clock) { - Assert.notNull(clock, "'clock' is required."); - this.clock = clock; - } - - /** - * Return the configured clock for session lastAccessTime calculations. - */ - public Clock getClock() { - return this.clock; - } - - @Override public Mono getSession(ServerWebExchange exchange) { return Mono.defer(() -> retrieveSession(exchange) .flatMap(session -> removeSessionIfExpired(exchange, session)) .flatMap(this.getSessionStore()::updateLastAccessTime) - .switchIfEmpty(createSession(exchange)) - .cast(DefaultWebSession.class) - .map(session -> new DefaultWebSession(session, session.getLastAccessTime(), s -> saveSession(exchange, s))) - .doOnNext(session -> exchange.getResponse().beforeCommit(session::save))); + .switchIfEmpty(this.sessionStore.createWebSession()) + .doOnNext(session -> exchange.getResponse().beforeCommit(() -> save(exchange, session)))); } private Mono retrieveSession(ServerWebExchange exchange) { @@ -128,35 +98,28 @@ public class DefaultWebSessionManager implements WebSessionManager { return Mono.just(session); } - private Mono saveSession(ServerWebExchange exchange, WebSession session) { + private Mono save(ServerWebExchange exchange, WebSession session) { if (session.isExpired()) { return Mono.error(new IllegalStateException( "Sessions are checked for expiration and have their " + - "lastAccessTime updated when first accessed during request processing. " + - "However this session is expired meaning that maxIdleTime elapsed " + - "before the call to session.save().")); + "lastAccessTime updated when first accessed during request processing. " + + "However this session is expired meaning that maxIdleTime elapsed " + + "before the call to session.save().")); } if (!session.isStarted()) { return Mono.empty(); } - // Force explicit start - session.start(); - if (hasNewSessionId(exchange, session)) { - this.sessionIdResolver.setSessionId(exchange, session.getId()); + DefaultWebSessionManager.this.sessionIdResolver.setSessionId(exchange, session.getId()); } - return this.sessionStore.storeSession(session); + return session.save(); } private boolean hasNewSessionId(ServerWebExchange exchange, WebSession session) { List ids = getSessionIdResolver().resolveSessionIds(exchange); return ids.isEmpty() || !session.getId().equals(ids.get(0)); } - - private Mono createSession(ServerWebExchange exchange) { - return this.sessionStore.createWebSession(); - } } diff --git a/spring-web/src/test/java/org/springframework/web/server/session/DefaultWebSessionManagerTests.java b/spring-web/src/test/java/org/springframework/web/server/session/DefaultWebSessionManagerTests.java index 290c8dac53..3bfe7ccf5a 100644 --- a/spring-web/src/test/java/org/springframework/web/server/session/DefaultWebSessionManagerTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/session/DefaultWebSessionManagerTests.java @@ -15,10 +15,6 @@ */ 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.Arrays; import java.util.Collections; @@ -32,8 +28,6 @@ import reactor.core.publisher.Mono; import org.springframework.http.codec.ServerCodecConfigurer; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse; -import org.springframework.util.IdGenerator; -import org.springframework.util.JdkIdGenerator; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebSession; import org.springframework.web.server.adapter.DefaultServerWebExchange; @@ -42,11 +36,11 @@ import org.springframework.web.server.i18n.AcceptHeaderLocaleContextResolver; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNotSame; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; /** @@ -57,11 +51,6 @@ import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) public class DefaultWebSessionManagerTests { - private static final Clock CLOCK = Clock.system(ZoneId.of("GMT")); - - private static final IdGenerator idGenerator = new JdkIdGenerator(); - - private DefaultWebSessionManager manager; private ServerWebExchange exchange; @@ -72,10 +61,23 @@ public class DefaultWebSessionManagerTests { @Mock private WebSessionStore store; + @Mock + private WebSession createSession; + + @Mock + private WebSession retrieveSession; + + @Mock + private WebSession updateSession; + @Before public void setUp() throws Exception { - when(this.store.createWebSession()).thenReturn(Mono.just(createDefaultWebSession())); - when(this.store.updateLastAccessTime(any())).thenAnswer( invocation -> Mono.just(invocation.getArgument(0))); + when(this.store.createWebSession()).thenReturn(Mono.just(this.createSession)); + when(this.store.updateLastAccessTime(any())).thenReturn(Mono.just(this.updateSession)); + when(this.store.retrieveSession(any())).thenReturn(Mono.just(this.retrieveSession)); + when(this.createSession.save()).thenReturn(Mono.empty()); + when(this.updateSession.getId()).thenReturn("update-session-id"); + when(this.retrieveSession.getId()).thenReturn("retrieve-session-id"); this.manager = new DefaultWebSessionManager(); this.manager.setSessionIdResolver(this.idResolver); @@ -87,90 +89,71 @@ public class DefaultWebSessionManagerTests { ServerCodecConfigurer.create(), new AcceptHeaderLocaleContextResolver()); } - @Test - public void getSessionWithoutStarting() throws Exception { + public void getSessionSaveWhenCreatedAndNotStartedThenNotSaved() throws Exception { when(this.idResolver.resolveSessionIds(this.exchange)).thenReturn(Collections.emptyList()); WebSession session = this.manager.getSession(this.exchange).block(); - session.save().block(); + this.exchange.getResponse().setComplete().block(); assertFalse(session.isStarted()); assertFalse(session.isExpired()); - verify(this.store, never()).storeSession(any()); + verifyZeroInteractions(this.retrieveSession, this.updateSession); + verify(this.createSession, never()).save(); verify(this.idResolver, never()).setSessionId(any(), any()); } @Test - public void startSessionExplicitly() throws Exception { + public void getSessionSaveWhenCreatedAndStartedThenSavesAndSetsId() throws Exception { when(this.idResolver.resolveSessionIds(this.exchange)).thenReturn(Collections.emptyList()); - when(this.store.storeSession(any())).thenReturn(Mono.empty()); WebSession session = this.manager.getSession(this.exchange).block(); - session.start(); - session.save().block(); + when(this.createSession.isStarted()).thenReturn(true); + this.exchange.getResponse().setComplete().block(); String id = session.getId(); verify(this.store).createWebSession(); - verify(this.store).storeSession(any()); + verify(this.createSession).save(); verify(this.idResolver).setSessionId(any(), eq(id)); } - @Test - public void startSessionImplicitly() throws Exception { - when(this.idResolver.resolveSessionIds(this.exchange)).thenReturn(Collections.emptyList()); - when(this.store.storeSession(any())).thenReturn(Mono.empty()); - WebSession session = this.manager.getSession(this.exchange).block(); - session.getAttributes().put("foo", "bar"); - session.save().block(); - - verify(this.store).createWebSession(); - verify(this.idResolver).setSessionId(any(), any()); - verify(this.store).storeSession(any()); - } - @Test public void exchangeWhenResponseSetCompleteThenSavesAndSetsId() throws Exception { when(this.idResolver.resolveSessionIds(this.exchange)).thenReturn(Collections.emptyList()); - when(this.store.storeSession(any())).thenReturn(Mono.empty()); + String id = this.createSession.getId(); WebSession session = this.manager.getSession(this.exchange).block(); - String id = session.getId(); - session.getAttributes().put("foo", "bar"); + when(this.createSession.isStarted()).thenReturn(true); this.exchange.getResponse().setComplete().block(); verify(this.idResolver).setSessionId(any(), eq(id)); - verify(this.store).storeSession(any()); + verify(this.createSession).save(); } @Test public void existingSession() throws Exception { - DefaultWebSession existing = createDefaultWebSession(); - String id = existing.getId(); - when(this.store.retrieveSession(id)).thenReturn(Mono.just(existing)); + String id = this.updateSession.getId(); + when(this.store.retrieveSession(id)).thenReturn(Mono.just(this.updateSession)); when(this.idResolver.resolveSessionIds(this.exchange)).thenReturn(Collections.singletonList(id)); WebSession actual = this.manager.getSession(this.exchange).block(); assertNotNull(actual); - assertEquals(existing.getId(), actual.getId()); + assertEquals(id, actual.getId()); } @Test public void existingSessionIsExpired() throws Exception { - DefaultWebSession existing = createDefaultWebSession(); - existing.start(); - Instant lastAccessTime = Instant.now(CLOCK).minus(Duration.ofMinutes(31)); - existing = new DefaultWebSession(existing, lastAccessTime, s -> Mono.empty()); - when(this.store.retrieveSession(existing.getId())).thenReturn(Mono.just(existing)); - when(this.store.removeSession(existing.getId())).thenReturn(Mono.empty()); - when(this.idResolver.resolveSessionIds(this.exchange)).thenReturn(Collections.singletonList(existing.getId())); + String id = this.retrieveSession.getId(); + when(this.retrieveSession.isExpired()).thenReturn(true); + when(this.idResolver.resolveSessionIds(this.exchange)).thenReturn(Collections.singletonList(id)); + when(this.store.removeSession(any())).thenReturn(Mono.empty()); WebSession actual = this.manager.getSession(this.exchange).block(); - assertNotSame(existing, actual); - verify(this.store).removeSession(existing.getId()); + assertEquals(this.createSession.getId(), actual.getId()); + verify(this.store).removeSession(id); verify(this.idResolver).expireSession(any()); } @Test public void multipleSessionIds() throws Exception { - DefaultWebSession existing = createDefaultWebSession(); + WebSession existing = this.updateSession; String id = existing.getId(); when(this.store.retrieveSession(any())).thenReturn(Mono.empty()); when(this.store.retrieveSession(id)).thenReturn(Mono.just(existing)); @@ -180,8 +163,4 @@ public class DefaultWebSessionManagerTests { assertNotNull(actual); assertEquals(existing.getId(), actual.getId()); } - - private DefaultWebSession createDefaultWebSession() { - return new DefaultWebSession(idGenerator, CLOCK, (s, session) -> Mono.empty(), s -> Mono.empty()); - } } diff --git a/spring-web/src/test/java/org/springframework/web/server/session/DefaultWebSessionTests.java b/spring-web/src/test/java/org/springframework/web/server/session/DefaultWebSessionTests.java new file mode 100644 index 0000000000..a3aa6635ac --- /dev/null +++ b/spring-web/src/test/java/org/springframework/web/server/session/DefaultWebSessionTests.java @@ -0,0 +1,74 @@ +/* + * 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 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 static org.junit.Assert.assertTrue; + +/** + * @author Rob Winch + * @since 5.0 + */ +public class DefaultWebSessionTests { + private static final Clock CLOCK = Clock.system(ZoneId.of("GMT")); + + private static final IdGenerator idGenerator = new JdkIdGenerator(); + + @Test + public void constructorWhenImplicitStartCopiedThenCopyIsStarted() { + DefaultWebSession original = createDefaultWebSession(); + original.getAttributes().put("foo", "bar"); + + DefaultWebSession copy = new DefaultWebSession(original, CLOCK.instant()); + + assertTrue(copy.isStarted()); + } + + @Test + public void constructorWhenExplicitStartCopiedThenCopyIsStarted() { + DefaultWebSession original = createDefaultWebSession(); + original.start(); + + DefaultWebSession copy = new DefaultWebSession(original, CLOCK.instant()); + + assertTrue(copy.isStarted()); + } + + @Test + public void startsSessionExplicitly() { + DefaultWebSession session = createDefaultWebSession(); + session.start(); + assertTrue(session.isStarted()); + } + + @Test + public void startsSessionImplicitly() { + DefaultWebSession session = createDefaultWebSession(); + session.getAttributes().put("foo", "bar"); + assertTrue(session.isStarted()); + } + + private DefaultWebSession createDefaultWebSession() { + return new DefaultWebSession(idGenerator, CLOCK, (s, session) -> Mono.empty(), s -> Mono.empty()); + } +} \ No newline at end of file diff --git a/spring-web/src/test/java/org/springframework/web/server/session/WebSessionIntegrationTests.java b/spring-web/src/test/java/org/springframework/web/server/session/WebSessionIntegrationTests.java index be20377025..5c06d7b665 100644 --- a/spring-web/src/test/java/org/springframework/web/server/session/WebSessionIntegrationTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/session/WebSessionIntegrationTests.java @@ -110,10 +110,10 @@ public class WebSessionIntegrationTests extends AbstractHttpHandlerIntegrationTe assertEquals(2, this.handler.getSessionRequestCount()); // Now set the clock of the session back by 31 minutes - WebSessionStore store = this.sessionManager.getSessionStore(); + InMemoryWebSessionStore store = (InMemoryWebSessionStore) this.sessionManager.getSessionStore(); DefaultWebSession session = (DefaultWebSession) store.retrieveSession(id).block(); assertNotNull(session); - Instant lastAccessTime = Clock.offset(this.sessionManager.getClock(), Duration.ofMinutes(-31)).instant(); + Instant lastAccessTime = Clock.offset(store.getClock(), Duration.ofMinutes(-31)).instant(); session = new DefaultWebSession(session, lastAccessTime); session.save().block();