Fix delta handling in JdbcOperationsSessionRepository

See gh-1070

Author: candrews

spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java

@@ -733,13 +733,23 @@ public Set<String> getAttributeNames() {
 		@Override
 		public void setAttribute(String attributeName, Object attributeValue) {
 			if (attributeValue == null) {
-				this.delta.put(attributeName, DeltaValue.REMOVED);
+				if (this.delta.get(attributeName) == DeltaValue.ADDED) {
+					this.delta.remove(attributeName);
+				}
+				else {
+					this.delta.put(attributeName, DeltaValue.REMOVED);
+				}
 			}
-			else if (this.delegate.getAttribute(attributeName) != null) {
+			else if (this.delta.get(attributeName) != DeltaValue.ADDED && this.delegate.getAttribute(attributeName) != null) {
 				this.delta.put(attributeName, DeltaValue.UPDATED);
 			}
 			else {
-				this.delta.put(attributeName, DeltaValue.ADDED);
+				if (this.delta.get(attributeName) == DeltaValue.REMOVED) {
+					this.delta.put(attributeName, DeltaValue.UPDATED);
+				}
+				else {
+					this.delta.put(attributeName, DeltaValue.ADDED);
+				}
 			}
 			this.delegate.setAttribute(attributeName, attributeValue);
 			if (PRINCIPAL_NAME_INDEX_NAME.equals(attributeName) ||

spring-session-jdbc/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java

@@ -43,12 +43,14 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.isA;
 import static org.mockito.ArgumentMatchers.startsWith;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyZeroInteractions;
@@ -338,6 +340,23 @@ public void saveUpdatedAddSingleAttribute() {
 		verifyZeroInteractions(this.jdbcOperations);
 	}
 
+	@Test
+	public void saveUpdatedAddSingleAttributeSetTwice() {
+		JdbcOperationsSessionRepository.JdbcSession session = this.repository.new JdbcSession("primaryKey",
+				new MapSession());
+		session.setAttribute("testName", "testValue");
+		session.setAttribute("testName", "testValue");
+
+		this.repository.save(session);
+
+		assertThat(session.isNew()).isFalse();
+		assertPropagationRequiresNew();
+		verify(this.jdbcOperations, times(1)).update(
+				startsWith("INSERT INTO SPRING_SESSION_ATTRIBUTES("),
+				isA(PreparedStatementSetter.class));
+		verifyZeroInteractions(this.jdbcOperations);
+	}
+
 	@Test
 	public void saveUpdatedAddMultipleAttributes() {
 		JdbcOperationsSessionRepository.JdbcSession session = this.repository.new JdbcSession("primaryKey",
@@ -431,6 +450,61 @@ public void saveUpdatedRemoveMultipleAttributes() {
 		verifyZeroInteractions(this.jdbcOperations);
 	}
 
+	@Test
+	public void saveUpdatedAddThenRemoveSingleAttribute() {
+		JdbcOperationsSessionRepository.JdbcSession session = this.repository.new JdbcSession("primaryKey",
+				new MapSession());
+		session.setAttribute("testName", "testValue");
+		session.removeAttribute("testName");
+
+		this.repository.save(session);
+
+		assertThat(session.isNew()).isFalse();
+		assertPropagationRequiresNew();
+		verify(this.jdbcOperations, never()).update(
+				anyString(),
+				isA(PreparedStatementSetter.class));
+		verifyZeroInteractions(this.jdbcOperations);
+	}
+
+	@Test
+	public void saveUpdatedModifyThenRemoveSingleAttribute() {
+		JdbcOperationsSessionRepository.JdbcSession session = this.repository.new JdbcSession("primaryKey",
+				new MapSession());
+		session.setAttribute("testName", "testValue");
+		session.clearChangeFlags();
+		session.setAttribute("testName", "testValueModifed");
+		session.removeAttribute("testName");
+
+		this.repository.save(session);
+
+		assertThat(session.isNew()).isFalse();
+		assertPropagationRequiresNew();
+		verify(this.jdbcOperations, times(1)).update(
+				startsWith("DELETE FROM SPRING_SESSION_ATTRIBUTES WHERE"),
+				isA(PreparedStatementSetter.class));
+		verifyZeroInteractions(this.jdbcOperations);
+	}
+
+	@Test
+	public void saveUpdatedRemoveThenModifySingleAttribute() {
+		JdbcOperationsSessionRepository.JdbcSession session = this.repository.new JdbcSession("primaryKey",
+				new MapSession());
+		session.setAttribute("testName", "testValue");
+		session.clearChangeFlags();
+		session.removeAttribute("testName");
+		session.setAttribute("testName", "testValueModifed");
+
+		this.repository.save(session);
+
+		assertThat(session.isNew()).isFalse();
+		assertPropagationRequiresNew();
+		verify(this.jdbcOperations, times(1)).update(
+				startsWith("UPDATE SPRING_SESSION_ATTRIBUTES SET"),
+				isA(PreparedStatementSetter.class));
+		verifyZeroInteractions(this.jdbcOperations);
+	}
+
 	@Test
 	public void saveUpdatedLastAccessedTime() {
 		JdbcOperationsSessionRepository.JdbcSession session = this.repository.new JdbcSession("primaryKey",