From 6188fe68b7779bb5a141dd40d598161870fe6027 Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Mon, 27 Nov 2017 00:52:30 +0100 Subject: [PATCH] Improve session event handling This commit removes constructor that takes session id instead of session object for the entire `AbstractSessionEvent` hierarchy. The ability to create `AbstractSessionEvent` instances with no underlying session object leads to NPE when interacting with `HttpSession` obtained from `HttpSessionEvent`. See gh-499 Closes gh-939 --- .../session/events/AbstractSessionEvent.java | 10 ++--- .../session/events/SessionCreatedEvent.java | 11 ++--- .../session/events/SessionDeletedEvent.java | 13 +++--- .../session/events/SessionDestroyedEvent.java | 12 ++---- .../session/events/SessionExpiredEvent.java | 13 +++--- .../session/web/http/HttpSessionAdapter.java | 15 +++++++ ...nEventHttpSessionListenerAdapterTests.java | 33 ++++++++++----- .../WebSocketRegistryListenerTests.java | 40 +++++++++++-------- .../RedisOperationsSessionRepository.java | 33 ++++++--------- 9 files changed, 97 insertions(+), 83 deletions(-) diff --git a/spring-session-core/src/main/java/org/springframework/session/events/AbstractSessionEvent.java b/spring-session-core/src/main/java/org/springframework/session/events/AbstractSessionEvent.java index a48048af..6b99945d 100644 --- a/spring-session-core/src/main/java/org/springframework/session/events/AbstractSessionEvent.java +++ b/spring-session-core/src/main/java/org/springframework/session/events/AbstractSessionEvent.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-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. @@ -29,16 +29,11 @@ import org.springframework.session.SessionRepository; */ @SuppressWarnings("serial") public abstract class AbstractSessionEvent extends ApplicationEvent { + private final String sessionId; private final Session session; - protected AbstractSessionEvent(Object source, String sessionId) { - super(source); - this.sessionId = sessionId; - this.session = null; - } - AbstractSessionEvent(Object source, Session session) { super(source); this.session = session; @@ -62,4 +57,5 @@ public abstract class AbstractSessionEvent extends ApplicationEvent { public String getSessionId() { return this.sessionId; } + } diff --git a/spring-session-core/src/main/java/org/springframework/session/events/SessionCreatedEvent.java b/spring-session-core/src/main/java/org/springframework/session/events/SessionCreatedEvent.java index a5ba8597..77c46950 100644 --- a/spring-session-core/src/main/java/org/springframework/session/events/SessionCreatedEvent.java +++ b/spring-session-core/src/main/java/org/springframework/session/events/SessionCreatedEvent.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-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. @@ -25,19 +25,14 @@ import org.springframework.session.SessionRepository; * * @author Rob Winch * @since 1.0 - * */ @SuppressWarnings("serial") public class SessionCreatedEvent extends AbstractSessionEvent { - public SessionCreatedEvent(Object source, String sessionId) { - super(source, sessionId); - } - /** * Create a new {@link SessionCreatedEvent}. - * @param source The Source of the SessionCreatedEvent - * @param session the Session that was created + * @param source the source of the event + * @param session the session that was created */ public SessionCreatedEvent(Object source, Session session) { super(source, session); diff --git a/spring-session-core/src/main/java/org/springframework/session/events/SessionDeletedEvent.java b/spring-session-core/src/main/java/org/springframework/session/events/SessionDeletedEvent.java index 484a8bd6..bf662ae2 100644 --- a/spring-session-core/src/main/java/org/springframework/session/events/SessionDeletedEvent.java +++ b/spring-session-core/src/main/java/org/springframework/session/events/SessionDeletedEvent.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-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. @@ -26,16 +26,17 @@ import org.springframework.session.SessionRepository; * @author Mark Anderson * @author Rob Winch * @since 1.1 - * */ @SuppressWarnings("serial") public class SessionDeletedEvent extends SessionDestroyedEvent { - public SessionDeletedEvent(Object source, String sessionId) { - super(source, sessionId); - } - + /** + * Create a new {@link SessionDeletedEvent}. + * @param source the source of the event + * @param session the session that was created + */ public SessionDeletedEvent(Object source, Session session) { super(source, session); } + } diff --git a/spring-session-core/src/main/java/org/springframework/session/events/SessionDestroyedEvent.java b/spring-session-core/src/main/java/org/springframework/session/events/SessionDestroyedEvent.java index 9eacbe43..e6083802 100644 --- a/spring-session-core/src/main/java/org/springframework/session/events/SessionDestroyedEvent.java +++ b/spring-session-core/src/main/java/org/springframework/session/events/SessionDestroyedEvent.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-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. @@ -23,21 +23,17 @@ import org.springframework.session.Session; * * @author Rob Winch * @since 1.0 - * */ @SuppressWarnings("serial") public class SessionDestroyedEvent extends AbstractSessionEvent { - public SessionDestroyedEvent(Object source, String sessionId) { - super(source, sessionId); - } - /** * Create a new {@link SessionDestroyedEvent}. - * @param source The Source of the SessionDestoryedEvent - * @param session the Session that was created + * @param source the source of the event + * @param session the session that was created */ public SessionDestroyedEvent(Object source, Session session) { super(source, session); } + } diff --git a/spring-session-core/src/main/java/org/springframework/session/events/SessionExpiredEvent.java b/spring-session-core/src/main/java/org/springframework/session/events/SessionExpiredEvent.java index f2efeacf..06d4286b 100644 --- a/spring-session-core/src/main/java/org/springframework/session/events/SessionExpiredEvent.java +++ b/spring-session-core/src/main/java/org/springframework/session/events/SessionExpiredEvent.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-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. @@ -26,16 +26,17 @@ import org.springframework.session.SessionRepository; * @author Mark Anderson * @author Rob Winch * @since 1.1 - * */ @SuppressWarnings("serial") public class SessionExpiredEvent extends SessionDestroyedEvent { - public SessionExpiredEvent(Object source, String sessionId) { - super(source, sessionId); - } - + /** + * Create a new {@link SessionExpiredEvent}. + * @param source the source of the event + * @param session the session that was created + */ public SessionExpiredEvent(Object source, Session session) { super(source, session); } + } diff --git a/spring-session-core/src/main/java/org/springframework/session/web/http/HttpSessionAdapter.java b/spring-session-core/src/main/java/org/springframework/session/web/http/HttpSessionAdapter.java index 555fea0c..becdbda9 100644 --- a/spring-session-core/src/main/java/org/springframework/session/web/http/HttpSessionAdapter.java +++ b/spring-session-core/src/main/java/org/springframework/session/web/http/HttpSessionAdapter.java @@ -37,12 +37,22 @@ import org.springframework.session.Session; */ @SuppressWarnings("deprecation") class HttpSessionAdapter implements HttpSession { + private S session; + private final ServletContext servletContext; + private boolean invalidated; + private boolean old; HttpSessionAdapter(S session, ServletContext servletContext) { + if (session == null) { + throw new IllegalArgumentException("session cannot be null"); + } + if (servletContext == null) { + throw new IllegalArgumentException("servletContext cannot be null"); + } this.session = session; this.servletContext = servletContext; } @@ -162,6 +172,7 @@ class HttpSessionAdapter implements HttpSession { } private static final HttpSessionContext NOOP_SESSION_CONTEXT = new HttpSessionContext() { + @Override public HttpSession getSession(String sessionId) { return null; @@ -171,9 +182,11 @@ class HttpSessionAdapter implements HttpSession { public Enumeration getIds() { return EMPTY_ENUMERATION; } + }; private static final Enumeration EMPTY_ENUMERATION = new Enumeration() { + @Override public boolean hasMoreElements() { return false; @@ -183,5 +196,7 @@ class HttpSessionAdapter implements HttpSession { public String nextElement() { throw new NoSuchElementException("a"); } + }; + } diff --git a/spring-session-core/src/test/java/org/springframework/session/web/http/SessionEventHttpSessionListenerAdapterTests.java b/spring-session-core/src/test/java/org/springframework/session/web/http/SessionEventHttpSessionListenerAdapterTests.java index f8a08b31..a60e1ac0 100644 --- a/spring-session-core/src/test/java/org/springframework/session/web/http/SessionEventHttpSessionListenerAdapterTests.java +++ b/spring-session-core/src/test/java/org/springframework/session/web/http/SessionEventHttpSessionListenerAdapterTests.java @@ -31,6 +31,7 @@ import org.mockito.Captor; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.mock.web.MockServletContext; import org.springframework.session.MapSession; import org.springframework.session.Session; import org.springframework.session.events.SessionCreatedEvent; @@ -42,28 +43,37 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; /** + * Tests for {@link SessionEventHttpSessionListenerAdapter}. + * * @author Rob Winch * @since 1.1 */ @RunWith(MockitoJUnitRunner.class) public class SessionEventHttpSessionListenerAdapterTests { - @Mock - HttpSessionListener listener1; - @Mock - HttpSessionListener listener2; - @Mock - ServletContext servletContext; - @Captor - ArgumentCaptor sessionEvent; - SessionDestroyedEvent destroyed; - SessionCreatedEvent created; - SessionEventHttpSessionListenerAdapter listener; + @Mock + private HttpSessionListener listener1; + + @Mock + private HttpSessionListener listener2; + + @Mock + private ServletContext servletContext; + + @Captor + private ArgumentCaptor sessionEvent; + + private SessionDestroyedEvent destroyed; + + private SessionCreatedEvent created; + + private SessionEventHttpSessionListenerAdapter listener; @Before public void setup() { this.listener = new SessionEventHttpSessionListenerAdapter( Arrays.asList(this.listener1, this.listener2)); + this.listener.setServletContext(new MockServletContext()); Session session = new MapSession(); this.destroyed = new SessionDestroyedEvent(this, session); @@ -114,4 +124,5 @@ public class SessionEventHttpSessionListenerAdapterTests { assertThat(this.sessionEvent.getValue().getSession().getId()) .isEqualTo(this.created.getSessionId()); } + } diff --git a/spring-session-core/src/test/java/org/springframework/session/web/socket/handler/WebSocketRegistryListenerTests.java b/spring-session-core/src/test/java/org/springframework/session/web/socket/handler/WebSocketRegistryListenerTests.java index f1f37c06..08ae75f1 100644 --- a/spring-session-core/src/test/java/org/springframework/session/web/socket/handler/WebSocketRegistryListenerTests.java +++ b/spring-session-core/src/test/java/org/springframework/session/web/socket/handler/WebSocketRegistryListenerTests.java @@ -29,6 +29,7 @@ import org.mockito.junit.MockitoJUnitRunner; import org.springframework.messaging.Message; import org.springframework.messaging.MessageHeaders; import org.springframework.messaging.simp.SimpMessageHeaderAccessor; +import org.springframework.session.MapSession; import org.springframework.session.events.SessionDeletedEvent; import org.springframework.session.events.SessionExpiredEvent; import org.springframework.session.web.socket.events.SessionConnectEvent; @@ -46,36 +47,40 @@ import static org.mockito.Mockito.verify; @RunWith(MockitoJUnitRunner.class) public class WebSocketRegistryListenerTests { + @Mock - WebSocketSession wsSession; + private WebSocketSession wsSession; + @Mock - WebSocketSession wsSession2; + private WebSocketSession wsSession2; + @Mock - Message message; + private Message message; + @Mock - Principal principal; + private Principal principal; - SessionConnectEvent connect; + private SessionConnectEvent connect; - SessionConnectEvent connect2; + private SessionConnectEvent connect2; - SessionDisconnectEvent disconnect; + private SessionDisconnectEvent disconnect; - SessionDeletedEvent deleted; + private SessionDeletedEvent deleted; - SessionExpiredEvent expired; + private SessionExpiredEvent expired; - Map attributes; + private Map attributes; - String sessionId; - - WebSocketRegistryListener listener; + private WebSocketRegistryListener listener; @Before public void setup() { - this.sessionId = "session-id"; + String sessionId = "session-id"; + MapSession session = new MapSession(sessionId); + this.attributes = new HashMap<>(); - SessionRepositoryMessageInterceptor.setSessionId(this.attributes, this.sessionId); + SessionRepositoryMessageInterceptor.setSessionId(this.attributes, sessionId); given(this.wsSession.getAttributes()).willReturn(this.attributes); given(this.wsSession.getPrincipal()).willReturn(this.principal); @@ -94,8 +99,8 @@ public class WebSocketRegistryListenerTests { this.connect2 = new SessionConnectEvent(this.listener, this.wsSession2); this.disconnect = new SessionDisconnectEvent(this.listener, this.message, this.wsSession.getId(), CloseStatus.NORMAL); - this.deleted = new SessionDeletedEvent(this.listener, this.sessionId); - this.expired = new SessionExpiredEvent(this.listener, this.sessionId); + this.deleted = new SessionDeletedEvent(this.listener, session); + this.expired = new SessionExpiredEvent(this.listener, session); } @Test @@ -170,4 +175,5 @@ public class WebSocketRegistryListenerTests { verify(this.wsSession2).close(WebSocketRegistryListener.SESSION_EXPIRED_STATUS); verify(this.wsSession, times(0)).close(any(CloseStatus.class)); } + } diff --git a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java index 1ab35c84..30c45195 100644 --- a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java +++ b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java @@ -527,6 +527,12 @@ public class RedisOperationsSessionRepository implements RedisSession session = getSession(sessionId, true); + if (session == null) { + logger.warn("Unable to publish SessionDestroyedEvent for session " + + sessionId); + return; + } + if (logger.isDebugEnabled()) { logger.debug("Publishing SessionDestroyedEvent for session " + sessionId); } @@ -534,18 +540,15 @@ public class RedisOperationsSessionRepository implements cleanupPrincipalIndex(session); if (isDeleted) { - handleDeleted(sessionId, session); + handleDeleted(session); } else { - handleExpired(sessionId, session); + handleExpired(session); } } } private void cleanupPrincipalIndex(RedisSession session) { - if (session == null) { - return; - } String sessionId = session.getId(); String principal = PRINCIPAL_NAME_RESOLVER.resolvePrincipal(session); if (principal != null) { @@ -554,28 +557,18 @@ public class RedisOperationsSessionRepository implements } } - public void handleCreated(Map loaded, String channel) { + private void handleCreated(Map loaded, String channel) { String id = channel.substring(channel.lastIndexOf(":") + 1); Session session = loadSession(id, loaded); publishEvent(new SessionCreatedEvent(this, session)); } - private void handleDeleted(String sessionId, RedisSession session) { - if (session == null) { - publishEvent(new SessionDeletedEvent(this, sessionId)); - } - else { - publishEvent(new SessionDeletedEvent(this, session)); - } + private void handleDeleted(RedisSession session) { + publishEvent(new SessionDeletedEvent(this, session)); } - private void handleExpired(String sessionId, RedisSession session) { - if (session == null) { - publishEvent(new SessionExpiredEvent(this, sessionId)); - } - else { - publishEvent(new SessionExpiredEvent(this, session)); - } + private void handleExpired(RedisSession session) { + publishEvent(new SessionExpiredEvent(this, session)); } private void publishEvent(ApplicationEvent event) {