You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/03/01 00:21:59 UTC
[GitHub] [geode] BenjaminPerryRoss opened a new pull request #7405: GEODE-10093 - Fixed attr issue in Delta Session
BenjaminPerryRoss opened a new pull request #7405:
URL: https://github.com/apache/geode/pull/7405
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #7405: GEODE-10093 - Fixed attr issue in Delta Session
Posted by GitBox <gi...@apache.org>.
BenjaminPerryRoss commented on a change in pull request #7405:
URL: https://github.com/apache/geode/pull/7405#discussion_r817032941
##########
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:
Oops, removed that.
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #7405: GEODE-10093 - Fixed attr issue in Delta Session
Posted by GitBox <gi...@apache.org>.
BenjaminPerryRoss commented on a change in pull request #7405:
URL: https://github.com/apache/geode/pull/7405#discussion_r817033858
##########
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:
I wrote up a test case for null name and it still returned null in all the cases I tried. Still just in case there are situations where it might not (not sure if maybe specific states of the underlying region could cause this to have different behavior or not) I went ahead and added an if(name == null) check at the beginning of the getAttribute method to ensure that this case will always return null.
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #7405: GEODE-10093 - Fixed attr issue in Delta Session
Posted by GitBox <gi...@apache.org>.
BenjaminPerryRoss commented on a change in pull request #7405:
URL: https://github.com/apache/geode/pull/7405#discussion_r817032703
##########
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:
Good call, I added this logic to that line.
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] BenjaminPerryRoss merged pull request #7405: GEODE-10093 - Fixed attr issue in Delta Session
Posted by GitBox <gi...@apache.org>.
BenjaminPerryRoss merged pull request #7405:
URL: https://github.com/apache/geode/pull/7405
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #7405: GEODE-10093 - Fixed attr issue in Delta Session
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #7405: GEODE-10093 - Fixed attr issue in Delta Session
Posted by GitBox <gi...@apache.org>.
BenjaminPerryRoss commented on a change in pull request #7405:
URL: https://github.com/apache/geode/pull/7405#discussion_r817034073
##########
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:
That's a good point - added it.
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org