Fix session event handling in HazelcastSessionRepository

Previously, invoking HttpServletRequest#changeSessionId on session backed by HazelcastSessionRepository generated generated invalid session destroyed and session created events. This was due to use of IMap#remove and IMap#set when handling the change session id.

This commit improves change session id handling to prevent publishing invalid events by using IMap#delete instead of IMap#remove and keeping track of originally assigned session id.

Closes gh-1077

Author: vpavic

spring-session-core/src/main/java/org/springframework/session/MapSession.java

@@ -122,7 +122,12 @@ public String getId() {
 		return this.id;
 	}
 
-	String getOriginalId() {
+	/**
+	 * Get the original session id.
+	 * @return the original session id
+	 * @see #changeSessionId()
+	 */
+	public String getOriginalId() {
 		return this.originalId;
 	}
 

spring-session-hazelcast/src/integration-test/java/org/springframework/session/hazelcast/config/annotation/web/http/EnableHazelcastHttpSessionEventsTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2017 the original author or authors.
+ * Copyright 2014-2018 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.
@@ -52,6 +52,7 @@
  * expected after each SessionEvent.
  *
  * @author Tommy Ludwig
+ * @author Vedran Pavic
  */
 @RunWith(SpringRunner.class)
 @ContextConfiguration
@@ -168,6 +169,24 @@ public void saveUpdatesTimeToLiveTest() throws InterruptedException {
 		assertThat(this.repository.findById(sessionToUpdate.getId())).isNotNull();
 	}
 
+	@Test // gh-1077
+	public void changeSessionIdNoEventTest() throws InterruptedException {
+		S sessionToSave = this.repository.createSession();
+		sessionToSave.setMaxInactiveInterval(Duration.ofMinutes(30));
+
+		this.repository.save(sessionToSave);
+
+		assertThat(this.registry.receivedEvent(sessionToSave.getId())).isTrue();
+		assertThat(this.registry.<SessionCreatedEvent>getEvent(sessionToSave.getId()))
+				.isInstanceOf(SessionCreatedEvent.class);
+		this.registry.clear();
+
+		sessionToSave.changeSessionId();
+		this.repository.save(sessionToSave);
+
+		assertThat(this.registry.receivedEvent(sessionToSave.getId())).isFalse();
+	}
+
 	@Configuration
 	@EnableHazelcastHttpSession(maxInactiveIntervalInSeconds = MAX_INACTIVE_INTERVAL_IN_SECONDS)
 	static class HazelcastSessionConfig {

spring-session-hazelcast/src/main/java/org/springframework/session/hazelcast/HazelcastSessionRepository.java

@@ -224,20 +224,30 @@ public HazelcastSession createSession() {
 
 	@Override
 	public void save(HazelcastSession session) {
-		if (!session.getId().equals(session.originalId)) {
-			this.sessions.remove(session.originalId);
-			session.originalId = session.getId();
-		}
 		if (session.isNew) {
 			this.sessions.set(session.getId(), session.getDelegate(),
 					session.getMaxInactiveInterval().getSeconds(), TimeUnit.SECONDS);
 		}
-		else if (session.changed) {
-			this.sessions.executeOnKey(session.getId(),
-					new SessionUpdateEntryProcessor(session.getLastAccessedTime(),
-							session.getMaxInactiveInterval(), session.delta));
+		else if (session.sessionIdChanged) {
+			this.sessions.delete(session.originalId);
+			session.originalId = session.getId();
+			this.sessions.set(session.getId(), session.getDelegate(),
+					session.getMaxInactiveInterval().getSeconds(), TimeUnit.SECONDS);
+		}
+		else if (session.hasChanges()) {
+			SessionUpdateEntryProcessor entryProcessor = new SessionUpdateEntryProcessor();
+			if (session.lastAccessedTimeChanged) {
+				entryProcessor.setLastAccessedTime(session.getLastAccessedTime());
+			}
+			if (session.maxInactiveIntervalChanged) {
+				entryProcessor.setMaxInactiveInterval(session.getMaxInactiveInterval());
+			}
+			if (!session.delta.isEmpty()) {
+				entryProcessor.setDelta(session.delta);
+			}
+			this.sessions.executeOnKey(session.getId(), entryProcessor);
 		}
-		session.clearFlags();
+		session.clearChangeFlags();
 	}
 
 	@Override
@@ -275,10 +285,13 @@ public Map<String, HazelcastSession> findByIndexNameAndIndexValue(String indexNa
 
 	@Override
 	public void entryAdded(EntryEvent<String, MapSession> event) {
-		if (logger.isDebugEnabled()) {
-			logger.debug("Session created with id: " + event.getValue().getId());
+		MapSession session = event.getValue();
+		if (session.getId().equals(session.getOriginalId())) {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Session created with id: " + session.getId());
+			}
+			this.eventPublisher.publishEvent(new SessionCreatedEvent(this, session));
 		}
-		this.eventPublisher.publishEvent(new SessionCreatedEvent(this, event.getValue()));
 	}
 
 	@Override
@@ -292,11 +305,13 @@ public void entryEvicted(EntryEvent<String, MapSession> event) {
 
 	@Override
 	public void entryRemoved(EntryEvent<String, MapSession> event) {
-		if (logger.isDebugEnabled()) {
-			logger.debug("Session deleted with id: " + event.getOldValue().getId());
+		MapSession session = event.getOldValue();
+		if (session != null) {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Session deleted with id: " + session.getId());
+			}
+			this.eventPublisher.publishEvent(new SessionDeletedEvent(this, session));
 		}
-		this.eventPublisher
-				.publishEvent(new SessionDeletedEvent(this, event.getOldValue()));
 	}
 
 	/**
@@ -311,7 +326,11 @@ final class HazelcastSession implements Session {
 
 		private boolean isNew;
 
-		private boolean changed;
+		private boolean sessionIdChanged;
+
+		private boolean lastAccessedTimeChanged;
+
+		private boolean maxInactiveIntervalChanged;
 
 		private String originalId;
 
@@ -341,7 +360,7 @@ final class HazelcastSession implements Session {
 		@Override
 		public void setLastAccessedTime(Instant lastAccessedTime) {
 			this.delegate.setLastAccessedTime(lastAccessedTime);
-			this.changed = true;
+			this.lastAccessedTimeChanged = true;
 			flushImmediateIfNecessary();
 		}
 
@@ -362,8 +381,9 @@ public String getId() {
 
 		@Override
 		public String changeSessionId() {
-			this.isNew = true;
-			return this.delegate.changeSessionId();
+			String newSessionId = this.delegate.changeSessionId();
+			this.sessionIdChanged = true;
+			return newSessionId;
 		}
 
 		@Override
@@ -374,7 +394,7 @@ public Instant getLastAccessedTime() {
 		@Override
 		public void setMaxInactiveInterval(Duration interval) {
 			this.delegate.setMaxInactiveInterval(interval);
-			this.changed = true;
+			this.maxInactiveIntervalChanged = true;
 			flushImmediateIfNecessary();
 		}
 
@@ -397,25 +417,30 @@ public Set<String> getAttributeNames() {
 		public void setAttribute(String attributeName, Object attributeValue) {
 			this.delegate.setAttribute(attributeName, attributeValue);
 			this.delta.put(attributeName, attributeValue);
-			this.changed = true;
 			flushImmediateIfNecessary();
 		}
 
 		@Override
 		public void removeAttribute(String attributeName) {
 			this.delegate.removeAttribute(attributeName);
 			this.delta.put(attributeName, null);
-			this.changed = true;
 			flushImmediateIfNecessary();
 		}
 
 		MapSession getDelegate() {
 			return this.delegate;
 		}
 
-		void clearFlags() {
+		boolean hasChanges() {
+			return (this.lastAccessedTimeChanged || this.maxInactiveIntervalChanged
+					|| !this.delta.isEmpty());
+		}
+
+		void clearChangeFlags() {
 			this.isNew = false;
-			this.changed = false;
+			this.lastAccessedTimeChanged = false;
+			this.sessionIdChanged = false;
+			this.maxInactiveIntervalChanged = false;
 			this.delta.clear();
 		}
 
@@ -436,39 +461,50 @@ private void flushImmediateIfNecessary() {
 	private static final class SessionUpdateEntryProcessor
 			extends AbstractEntryProcessor<String, MapSession> {
 
-		private final Instant lastAccessedTime;
+		private Instant lastAccessedTime;
 
-		private final Duration maxInactiveInterval;
+		private Duration maxInactiveInterval;
 
-		private final Map<String, Object> delta;
-
-		SessionUpdateEntryProcessor(Instant lastAccessedTime,
-				Duration maxInactiveInterval, Map<String, Object> delta) {
-			this.lastAccessedTime = lastAccessedTime;
-			this.maxInactiveInterval = maxInactiveInterval;
-			this.delta = delta;
-		}
+		private Map<String, Object> delta;
 
 		@Override
 		public Object process(Map.Entry<String, MapSession> entry) {
 			MapSession value = entry.getValue();
 			if (value == null) {
 				return Boolean.FALSE;
 			}
-			value.setLastAccessedTime(this.lastAccessedTime);
-			value.setMaxInactiveInterval(this.maxInactiveInterval);
-			for (final Map.Entry<String, Object> attribute : this.delta.entrySet()) {
-				if (attribute.getValue() != null) {
-					value.setAttribute(attribute.getKey(), attribute.getValue());
-				}
-				else {
-					value.removeAttribute(attribute.getKey());
+			if (this.lastAccessedTime != null) {
+				value.setLastAccessedTime(this.lastAccessedTime);
+			}
+			if (this.maxInactiveInterval != null) {
+				value.setMaxInactiveInterval(this.maxInactiveInterval);
+			}
+			if (this.delta != null) {
+				for (final Map.Entry<String, Object> attribute : this.delta.entrySet()) {
+					if (attribute.getValue() != null) {
+						value.setAttribute(attribute.getKey(), attribute.getValue());
+					}
+					else {
+						value.removeAttribute(attribute.getKey());
+					}
 				}
 			}
 			entry.setValue(value);
 			return Boolean.TRUE;
 		}
 
+		public void setLastAccessedTime(Instant lastAccessedTime) {
+			this.lastAccessedTime = lastAccessedTime;
+		}
+
+		public void setMaxInactiveInterval(Duration maxInactiveInterval) {
+			this.maxInactiveInterval = maxInactiveInterval;
+		}
+
+		public void setDelta(Map<String, Object> delta) {
+			this.delta = delta;
+		}
+
 	}
 
 }