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

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;
 	}
+
 }

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);

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);
 	}
+
 }

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);
 	}
+
 }

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);
 	}
+
 }

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<S extends Session> 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<S extends Session> 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<S extends Session> implements HttpSession {
 		public Enumeration<String> getIds() {
 			return EMPTY_ENUMERATION;
 		}
+
 	};
 
 	private static final Enumeration<String> EMPTY_ENUMERATION = new Enumeration<String>() {
+
 		@Override
 		public boolean hasMoreElements() {
 			return false;
@@ -183,5 +196,7 @@ class HttpSessionAdapter<S extends Session> implements HttpSession {
 		public String nextElement() {
 			throw new NoSuchElementException("a");
 		}
+
 	};
+
 }

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<HttpSessionEvent> sessionEvent;
 
-	SessionDestroyedEvent destroyed;
-	SessionCreatedEvent created;
-	SessionEventHttpSessionListenerAdapter listener;
+	@Mock
+	private HttpSessionListener listener1;
+
+	@Mock
+	private HttpSessionListener listener2;
+
+	@Mock
+	private ServletContext servletContext;
+
+	@Captor
+	private ArgumentCaptor<HttpSessionEvent> 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());
 	}
+
 }

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<byte[]> message;
+	private Message<byte[]> 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<String, Object> attributes;
+	private Map<String, Object> 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));
 	}
+
 }

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<Object, Object> loaded, String channel) {
+	private void handleCreated(Map<Object, Object> 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) {