pivotal-jbarrett commented on a change in pull request #7405:
URL: https://github.com/apache/geode/pull/7405#discussion_r816926722
##########
File path:
extensions/geode-modules-test/src/main/java/org/apache/geode/modules/session/catalina/AbstractDeltaSessionIntegrationTest.java
##########
@@ -107,4 +115,79 @@ public void
serializedAttributesNotLeakedWhenSessionInvalidated() throws IOExcep
verifyNoMoreInteractions(listener);
assertThat(event.getValue().getValue()).isEqualTo(value1);
}
+
+ @Test
+ public void setNewAttributeWithNullValueInvokesRemove() {
+ final HttpSessionAttributeListener listener =
mock(HttpSessionAttributeListener.class);
+ when(context.getApplicationEventListeners()).thenReturn(new Object[]
{listener});
+ when(manager.isBackingCacheAvailable()).thenReturn(true);
+
+ final DeltaSessionT session = spy(newSession(manager));
+ session.setId(KEY, false);
+ session.setValid(true);
+ session.setOwner(manager);
+
+ final String name = "attribute";
+ final Object nullValue = null;
+
+ session.setAttribute(name, nullValue);
+ assertThat(session.getAttributes().size()).isEqualTo(0);
+
+ // Mockito.inOrder
Review comment:
You left a note comment here.
##########
File path:
extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -266,7 +266,7 @@ public void setAttribute(String name, Object value, boolean
notify) {
super.setAttribute(name, serializedValue, true);
}
- if (serializedValue == null) {
+ if (value == null) {
Review comment:
I think a comment is necessary here. Without knowing the details of
`super.setAttribute` why we would return here is unclear.
```
// super.setAttribute above performed a removeAttribute for null value.
```
Or something like that.
##########
File path:
extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -266,7 +266,7 @@ public void setAttribute(String name, Object value, boolean
notify) {
super.setAttribute(name, serializedValue, true);
Review comment:
In the unlikely case that `preferDeserializedForm` is `false` then this
line won't end up invoking `removeAttribute` as expected by the caller sending
`null`. Quick fix is line 257:
```java
final byte[] serializedValue = value == null ? null : serialize(value);
```
##########
File path:
extensions/geode-modules-test/src/main/java/org/apache/geode/modules/session/catalina/AbstractDeltaSessionIntegrationTest.java
##########
@@ -107,4 +115,79 @@ public void
serializedAttributesNotLeakedWhenSessionInvalidated() throws IOExcep
verifyNoMoreInteractions(listener);
assertThat(event.getValue().getValue()).isEqualTo(value1);
}
+
+ @Test
+ public void setNewAttributeWithNullValueInvokesRemove() {
+ final HttpSessionAttributeListener listener =
mock(HttpSessionAttributeListener.class);
+ when(context.getApplicationEventListeners()).thenReturn(new Object[]
{listener});
+ when(manager.isBackingCacheAvailable()).thenReturn(true);
+
+ final DeltaSessionT session = spy(newSession(manager));
+ session.setId(KEY, false);
+ session.setValid(true);
+ session.setOwner(manager);
+
+ final String name = "attribute";
+ final Object nullValue = null;
+
+ session.setAttribute(name, nullValue);
+ assertThat(session.getAttributes().size()).isEqualTo(0);
+
+ // Mockito.inOrder
+
verify(session).queueAttributeEvent(any(DeltaSessionDestroyAttributeEvent.class),
anyBoolean());
+ verify(session,
times(0)).queueAttributeEvent(any(DeltaSessionUpdateAttributeEvent.class),
+ anyBoolean());
+ verify(session).removeAttribute(eq(name));
+ }
+
+ @Test
+ public void setExistingAttributeWithNullValueInvokesRemove() {
+ final HttpSessionAttributeListener listener =
mock(HttpSessionAttributeListener.class);
+ when(context.getApplicationEventListeners()).thenReturn(new Object[]
{listener});
+ when(manager.isBackingCacheAvailable()).thenReturn(true);
+
+ final DeltaSessionT session = spy(newSession(manager));
+ session.setId(KEY, false);
+ session.setValid(true);
+ session.setOwner(manager);
+
+ final String name = "attribute";
+ final Object value = "value";
+ final Object nullValue = null;
+
+ session.setAttribute(name, value);
+ assertThat(session.getAttributes().size()).isEqualTo(1);
+
+ session.setAttribute(name, nullValue);
+ assertThat(session.getAttributes().size()).isEqualTo(0);
+
+
+ InOrder inOrder = Mockito.inOrder(session);
+
inOrder.verify(session).queueAttributeEvent(any(DeltaSessionUpdateAttributeEvent.class),
+ anyBoolean());
+ inOrder.verify(session).removeAttribute(eq(name));
+
inOrder.verify(session).queueAttributeEvent(any(DeltaSessionDestroyAttributeEvent.class),
+ anyBoolean());
+ }
+
+ @Test
+ public void getAttributeWithNullValueReturnsNull() throws IOException,
ClassNotFoundException {
Review comment:
Similarly, I think we have a bug. The Javadoc for `getAttribute` does
not specify the behavior of `getAttribute(null)` but the implementation in
Tomcat returns `null`. We should make sure our implementation does not throw
any exceptions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
