Fix bug in DeltaCapableGemFireSessionAttriutes.newSessionAttributes(lock:Object) not properly passing the lock to the new GemFireSessionAttributes instance.

spring-session-data-geode/src/main/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepository.java

@@ -105,6 +105,7 @@ import org.apache.commons.logging.LogFactory;
 public abstract class AbstractGemFireOperationsSessionRepository extends CacheListenerAdapter<Object, Session>
 		implements ApplicationEventPublisherAware, FindByIndexNameSessionRepository<Session>, InitializingBean {
 
+	// TODO - refactor and use non-static variable
 	private static final AtomicBoolean usingDataSerialization = new AtomicBoolean(false);
 
 	private ApplicationEventPublisher applicationEventPublisher = event -> {};
@@ -180,6 +181,7 @@ public abstract class AbstractGemFireOperationsSessionRepository extends CacheLi
 	 * @return a {@link String} containing the fully qualified name of the cache {@link Region}
 	 * used to store and manage {@link Session} data.
 	 */
+	// TODO - refactor and rename to SessionRegionName
 	protected String getFullyQualifiedRegionName() {
 		return this.fullyQualifiedRegionName;
 	}
@@ -601,7 +603,7 @@ public abstract class AbstractGemFireOperationsSessionRepository extends CacheLi
 
 		@Override
 		protected DeltaCapableGemFireSessionAttributes newSessionAttributes(Object lock) {
-			return new DeltaCapableGemFireSessionAttributes();
+			return new DeltaCapableGemFireSessionAttributes(lock);
 		}
 
 		public synchronized void toDelta(DataOutput out) throws IOException {
@@ -642,10 +644,11 @@ public abstract class AbstractGemFireOperationsSessionRepository extends CacheLi
 		 * Factory method used to create a new instance of {@link GemFireSession} initialized with
 		 * the {@link #DEFAULT_MAX_INACTIVE_INTERVAL}.
 		 *
+		 * @param <T> {@link Class Sub-type} of {@link GemFireSessionAttributes}.
 		 * @return new {@link GemFireSession}.
 		 * @see #create(Duration)
 		 */
-		public static GemFireSession create() {
+		public static <T extends GemFireSessionAttributes> GemFireSession<T> create() {
 			return create(DEFAULT_MAX_INACTIVE_INTERVAL);
 		}
 
@@ -653,6 +656,7 @@ public abstract class AbstractGemFireOperationsSessionRepository extends CacheLi
 		 * Factory method used to create a new instance of {@link GemFireSession} initialized with
 		 * the given {@link Duration max inactive interval}.
 		 *
+		 * @param <T> {@link Class Sub-type} of {@link GemFireSessionAttributes}.
 		 * @param maxInactiveInterval {@link Duration} specifying the max inactive interval before
 		 * this {@link Session} will expire.
 		 * @return a new instance of {@link GemFireSession} initialized with
@@ -660,7 +664,9 @@ public abstract class AbstractGemFireOperationsSessionRepository extends CacheLi
 		 * @see #isUsingDataSerialization()
 		 * @see java.time.Duration
 		 */
-		public static GemFireSession create(Duration maxInactiveInterval) {
+		@SuppressWarnings("unchecked")
+		// TODO - remove
+		public static <T extends GemFireSessionAttributes> GemFireSession<T> create(Duration maxInactiveInterval) {
 
 			GemFireSession session = isUsingDataSerialization()
 				? new DeltaCapableGemFireSession()
@@ -690,15 +696,14 @@ public abstract class AbstractGemFireOperationsSessionRepository extends CacheLi
 		 * Returns the given {@link Session} if the {@link Session} is a {@link GemFireSession} or return a copy
 		 * of the given {@link Session} as a {@link GemFireSession}.
 		 *
-		 * @param <T> {@link Class sub-type} of {@link GemFireSession}.
 		 * @param session {@link Session} to evaluate and possibly copy.
 		 * @return the given {@link Session} if the {@link Session} is a {@link GemFireSession} or return a copy
 		 * of the given {@link Session} as a {@link GemFireSession}
 		 * @see #copy(Session)
 		 */
 		@SuppressWarnings("unchecked")
-		public static <T extends GemFireSession> T from(@NonNull Session session) {
-			return (T) (session instanceof GemFireSession ? session : copy(session));
+		public static GemFireSession from(@NonNull Session session) {
+			return session instanceof GemFireSession ? (GemFireSession) session : copy(session);
 		}
 
 		private transient boolean delta = true;
@@ -752,7 +757,7 @@ public abstract class AbstractGemFireOperationsSessionRepository extends CacheLi
 		 */
 		protected GemFireSession(Session session) {
 
-			Assert.notNull(session, "The Session to copy must not be null");
+			Assert.notNull(session, "Session is required");
 
 			this.id = session.getId();
 			this.creationTime = session.getCreationTime();

spring-session-data-geode/src/test/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepositoryTests.java

@@ -1212,11 +1212,10 @@ public class AbstractGemFireOperationsSessionRepositoryTests {
 		assertThat(session.getAttributes()).isEmpty();
 	}
 
-	@Test(expected = IllegalArgumentException.class)
-	public void constructGemFireSessionWithNullId() {
+	private void testConstructGemFireSessionWithInvalidId(String id) {
 
 		try {
-			new GemFireSession((String) null);
+			new GemFireSession(id);
 		}
 		catch (IllegalArgumentException expected) {
 
@@ -1227,19 +1226,19 @@ public class AbstractGemFireOperationsSessionRepositoryTests {
 		}
 	}
 
+	@Test(expected = IllegalArgumentException.class)
+	public void constructGemFireSessionWithEmptyId() {
+		testConstructGemFireSessionWithInvalidId("");
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void constructGemFireSessionWithNullId() {
+		testConstructGemFireSessionWithInvalidId(null);
+	}
+
 	@Test(expected = IllegalArgumentException.class)
 	public void constructGemFireSessionWithUnspecifiedId() {
-
-		try {
-			new GemFireSession(" ");
-		}
-		catch (IllegalArgumentException expected) {
-
-			assertThat(expected).hasMessage("ID is required");
-			assertThat(expected).hasNoCause();
-
-			throw expected;
-		}
+		testConstructGemFireSessionWithInvalidId("  ");
 	}
 
 	@Test
@@ -1287,7 +1286,7 @@ public class AbstractGemFireOperationsSessionRepositoryTests {
 		}
 		catch (IllegalArgumentException expected) {
 
-			assertThat(expected).hasMessage("The Session to copy must not be null");
+			assertThat(expected).hasMessage("Session is required");
 			assertThat(expected).hasNoCause();
 
 			throw expected;
@@ -1312,7 +1311,7 @@ public class AbstractGemFireOperationsSessionRepositoryTests {
 	}
 
 	@Test
-	public void createNewGemFireSessionWithProvidedMaxInactiveInterval() {
+	public void createNewGemFireSessionWithSpecifiedMaxInactiveInterval() {
 
 		Instant testCreationTime = Instant.now();
 
@@ -1331,14 +1330,14 @@ public class AbstractGemFireOperationsSessionRepositoryTests {
 	}
 
 	@Test(expected = IllegalArgumentException.class)
-	public void copyNullThrowsException() {
+	public void copyNullThrowsIllegalArgumentException() {
 
 		try {
 			GemFireSession.copy(null);
 		}
 		catch (IllegalArgumentException expected) {
 
-			assertThat(expected).hasMessage("The Session to copy must not be null");
+			assertThat(expected).hasMessage("Session is required");
 			assertThat(expected).hasNoCause();
 
 			throw expected;
@@ -1448,6 +1447,21 @@ public class AbstractGemFireOperationsSessionRepositoryTests {
 		assertThat(fromGemFireSession).isSameAs(gemfireSession);
 	}
 
+	@Test(expected = IllegalArgumentException.class)
+	public void fromNullSessionThrowsIllegalArgumentException() {
+
+		try {
+			GemFireSession.from(null);
+		}
+		catch (IllegalArgumentException expected) {
+
+			assertThat(expected).hasMessage("Session is required");
+			assertThat(expected).hasNoCause();
+
+			throw expected;
+		}
+	}
+
 	@Test
 	public void setGetAndRemoveAttribute() {
 
@@ -2448,6 +2462,28 @@ public class AbstractGemFireOperationsSessionRepositoryTests {
 		assertThat(entry.getValue()).isEqualTo("valueThree");
 	}
 
+	@Test
+	public void gemfireSessionIsLockForGemFireSessionAttributes() {
+
+		GemFireSession session = new GemFireSession();
+
+		GemFireSessionAttributes sessionAttributes = session.newSessionAttributes(session);
+
+		assertThat(sessionAttributes).isNotNull();
+		assertThat(sessionAttributes.getLock()).isSameAs(session);
+	}
+
+	@Test
+	public void deltaCapableGemFireSessionIsLockForDeltaCapableGemFirSessionAttributes() {
+
+		DeltaCapableGemFireSession session = new DeltaCapableGemFireSession();
+
+		DeltaCapableGemFireSessionAttributes sessionAttributes = session.newSessionAttributes(session);
+
+		assertThat(sessionAttributes).isNotNull();
+		assertThat(sessionAttributes.getLock()).isSameAs(session);
+	}
+
 	@Test
 	public void sessionWithAttributesAreThreadSafe() throws Throwable {
 		TestFramework.runOnce(new ThreadSafeSessionTestCase());