From 36823042420ffec565c2b63a739294a055858421 Mon Sep 17 00:00:00 2001 From: John Blum Date: Mon, 17 Dec 2018 13:52:05 -0800 Subject: [PATCH] Fix bug in DeltaCapableGemFireSessionAttriutes.newSessionAttributes(lock:Object) not properly passing the lock to the new GemFireSessionAttributes instance. --- ...actGemFireOperationsSessionRepository.java | 19 +++-- ...mFireOperationsSessionRepositoryTests.java | 72 ++++++++++++++----- 2 files changed, 66 insertions(+), 25 deletions(-) diff --git a/spring-session-data-geode/src/main/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepository.java b/spring-session-data-geode/src/main/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepository.java index d0de1e1..319bc61 100644 --- a/spring-session-data-geode/src/main/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepository.java +++ b/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 implements ApplicationEventPublisherAware, FindByIndexNameSessionRepository, 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 {@link Class Sub-type} of {@link GemFireSessionAttributes}. * @return new {@link GemFireSession}. * @see #create(Duration) */ - public static GemFireSession create() { + public static GemFireSession 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 {@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 GemFireSession 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 {@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 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(); diff --git a/spring-session-data-geode/src/test/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepositoryTests.java b/spring-session-data-geode/src/test/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepositoryTests.java index 164f23e..1c832e1 100644 --- a/spring-session-data-geode/src/test/java/org/springframework/session/data/gemfire/AbstractGemFireOperationsSessionRepositoryTests.java +++ b/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());