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 2020/06/16 17:19:45 UTC
[GitHub] [geode] pivotal-jbarrett opened a new pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
pivotal-jbarrett opened a new pull request #5256:
URL: https://github.com/apache/geode/pull/5256
When preferDeserializedForm is true we deserialize the previous attributes before update or remove.
Deprecates preferDeserializedForm since when false it's unclear when you will get serialized or unserialized forms of attributes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5256:
URL: https://github.com/apache/geode/pull/5256#issuecomment-644922137
This pull request **fixes 1 alert** when merging 5e2b66b338ac7013658ee6fc64728f940808cae3 into 9e52198049557c402ba79c5e3f36379321c769c2 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-f89aa6ec47ce445a33f0808a2b993bb376f3a6a5)
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett merged pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett merged pull request #5256:
URL: https://github.com/apache/geode/pull/5256
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5256:
URL: https://github.com/apache/geode/pull/5256#discussion_r444434215
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManagerConfiguration.java
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.modules.session.catalina;
+
+/**
+ * Method used by Catalina XML configuration.
+ */
+@SuppressWarnings("unused")
+interface DeltaSessionManagerConfiguration {
+
+ void setRegionName(String regionName);
+
+ String getRegionName();
+
+ void setEnableLocalCache(boolean enableLocalCache);
+
+ boolean getEnableLocalCache();
+
+ void setMaxActiveSessions(int maxActiveSessions);
+
+ int getMaxActiveSessions();
+
+ void setRegionAttributesId(String regionType);
+
+ String getRegionAttributesId();
+
+ void setEnableGatewayDeltaReplication(boolean enableGatewayDeltaReplication);
+
+ boolean getEnableGatewayDeltaReplication();
+
+ void setEnableGatewayReplication(boolean enableGatewayReplication);
+
+ boolean getEnableGatewayReplication();
+
+ void setEnableDebugListener(boolean enableDebugListener);
+
+ boolean getEnableDebugListener();
+
+ boolean isCommitValveEnabled();
+
+ void setEnableCommitValve(boolean enable);
+
+ boolean isCommitValveFailfastEnabled();
+
+ void setEnableCommitValveFailfast(boolean enable);
+
+ boolean isBackingCacheAvailable();
+
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
+ */
+ @Deprecated
+ void setPreferDeserializedForm(boolean enable);
+
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
Review comment:
Couple of typos here. Should be "Always prefer deserialized form."
##########
File path: extensions/geode-modules-tomcat7/src/test/java/org/apache/geode/modules/session/catalina/DeltaSession7Test.java
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.modules.session.catalina;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+
+import javax.servlet.http.HttpSessionAttributeListener;
+import javax.servlet.http.HttpSessionBindingEvent;
+
+import org.apache.catalina.Context;
+import org.apache.juli.logging.Log;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.internal.util.BlobHelper;
+
+public class DeltaSession7Test extends AbstractDeltaSessionTest {
+ final HttpSessionAttributeListener listener = mock(HttpSessionAttributeListener.class);
+
+ @Before
+ @Override
+ public void setup() {
+ super.setup();
+
+ final Context context = mock(Context.class);
+ when(manager.getContainer()).thenReturn(context);
+ when(context.getApplicationEventListeners()).thenReturn(new Object[] {listener});
+ when(context.getLogger()).thenReturn(mock(Log.class));
+ }
+
+ @Test
+ public void serializedAttributesNotLeakedInAttributeReplaceEvent() throws IOException {
+ final DeltaSession7 session = spy(new DeltaSession7(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ final Object value2 = "value2";
+ session.setAttribute(name, value2);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeReplaced(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isEqualTo(value1);
+ }
+
+ @Test
+ public void serializedAttributesNotLeakedInAttributeRemovedEvent() throws IOException {
+ final DeltaSession7 session = spy(new DeltaSession7(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ session.removeAttribute(name);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeRemoved(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isEqualTo(value1);
+ }
+
+ @Test
+ public void serializedAttributesLeakedInAttributeReplaceEventWhenPreferSerializedFormFalse()
+ throws IOException {
+ setPreferSeserializedForm();
+
+ final DeltaSession7 session = spy(new DeltaSession7(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ final Object value2 = "value2";
+ session.setAttribute(name, value2);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeReplaced(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isInstanceOf(byte[].class);
+ }
+
+ @Test
+ public void serializedAttributesLeakedInAttributeRemovedEventWhenPreferSerializedFormFalse()
+ throws IOException {
+ setPreferSeserializedForm();
+
+ final DeltaSession7 session = spy(new DeltaSession7(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ session.removeAttribute(name);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeRemoved(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isInstanceOf(byte[].class);
+ }
+
+ @SuppressWarnings("deprecation")
+ protected void setPreferSeserializedForm() {
Review comment:
Typo here, should be "setPreferDeserializedForm". Also, tests using this method should probably be renamed, since they state the condition "WhenPreferSerializedFormFalse" rather than "WhenPreferDeserializedFormFalse". It's also a little misleading as a method name, since `setPreferDeserializedForm()` with no argument would tend to imply setting the value to `true` rather than `false`.
##########
File path: extensions/geode-modules-tomcat8/src/test/java/org/apache/geode/modules/session/catalina/DeltaSession8Test.java
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.modules.session.catalina;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+
+import javax.servlet.http.HttpSessionAttributeListener;
+import javax.servlet.http.HttpSessionBindingEvent;
+
+import org.apache.catalina.Context;
+import org.apache.juli.logging.Log;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.internal.util.BlobHelper;
+
+public class DeltaSession8Test extends AbstractDeltaSessionTest {
+ final HttpSessionAttributeListener listener = mock(HttpSessionAttributeListener.class);
+
+ @Before
+ @Override
+ public void setup() {
+ super.setup();
+
+ final Context context = mock(Context.class);
+ when(manager.getContext()).thenReturn(context);
+ when(context.getApplicationEventListeners()).thenReturn(new Object[] {listener});
+ when(context.getLogger()).thenReturn(mock(Log.class));
+ }
+
+ @Test
+ public void serializedAttributesNotLeakedInAttributeReplaceEvent() throws IOException {
+ final DeltaSession8 session = spy(new DeltaSession8(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ final Object value2 = "value2";
+ session.setAttribute(name, value2);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeReplaced(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isEqualTo(value1);
+ }
+
+ @Test
+ public void serializedAttributesNotLeakedInAttributeRemovedEvent() throws IOException {
+ final DeltaSession8 session = spy(new DeltaSession8(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ session.removeAttribute(name);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeRemoved(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isEqualTo(value1);
+ }
+
+ @Test
+ public void serializedAttributesLeakedInAttributeReplaceEventWhenPreferSerializedFormFalse()
+ throws IOException {
+ setPreferSeserializedForm();
+
+ final DeltaSession8 session = spy(new DeltaSession8(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ final Object value2 = "value2";
+ session.setAttribute(name, value2);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeReplaced(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isInstanceOf(byte[].class);
+ }
+
+ @Test
+ public void serializedAttributesLeakedInAttributeRemovedEventWhenPreferSerializedFormFalse()
+ throws IOException {
+ setPreferSeserializedForm();
+
+ final DeltaSession8 session = spy(new DeltaSession8(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ session.removeAttribute(name);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeRemoved(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isInstanceOf(byte[].class);
+ }
+
+ @SuppressWarnings("deprecation")
+ protected void setPreferSeserializedForm() {
Review comment:
See comment on DeltaSession7Test.java regarding this typo and method names.
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -328,14 +353,14 @@ Object getAttributeWithoutDeserialize(String name) {
public void invalidate() {
super.invalidate();
// getOperatingRegion().destroy(this.id, true); // already done in super (remove)
Review comment:
Can this commented code be removed?
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -69,17 +71,21 @@
private final transient Object changeLock = new Object();
- private final List<DeltaSessionAttributeEvent> eventQueue = new ArrayList<>();
+ private final ArrayList<DeltaSessionAttributeEvent> eventQueue = new ArrayList<>();
private transient GatewayDeltaEvent currentGatewayDeltaEvent;
private transient boolean expired = false;
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
Review comment:
Typo here, should be "Always prefer deserialized form."
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java
##########
@@ -107,6 +110,10 @@
private static final boolean DEFAULT_ENABLE_COMMIT_VALVE_FAILFAST = false;
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
Review comment:
Typo here, should be "Always prefer deserialized form."
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java
##########
@@ -130,6 +137,10 @@
private boolean enableDebugListener = DEFAULT_ENABLE_DEBUG_LISTENER;
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
Review comment:
Typo here, should be "Always prefer deserialized form."
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java
##########
@@ -213,64 +225,78 @@ public boolean getEnableGatewayDeltaReplication() {
return false; // disabled
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableGatewayDeltaReplication(boolean enableGatewayDeltaReplication) {
// this.enableGatewayDeltaReplication = enableGatewayDeltaReplication;
// Disabled. Keeping the method for backward compatibility.
}
@Override
public boolean getEnableGatewayReplication() {
- return this.enableGatewayReplication;
+ return enableGatewayReplication;
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableGatewayReplication(boolean enableGatewayReplication) {
this.enableGatewayReplication = enableGatewayReplication;
}
@Override
public boolean getEnableDebugListener() {
- return this.enableDebugListener;
+ return enableDebugListener;
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableDebugListener(boolean enableDebugListener) {
this.enableDebugListener = enableDebugListener;
}
@Override
public boolean isCommitValveEnabled() {
- return this.enableCommitValve;
+ return enableCommitValve;
}
+ @Override
public void setEnableCommitValve(boolean enable) {
- this.enableCommitValve = enable;
+ enableCommitValve = enable;
}
@Override
public boolean isCommitValveFailfastEnabled() {
- return this.enableCommitValveFailfast;
+ return enableCommitValveFailfast;
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableCommitValveFailfast(boolean enable) {
- this.enableCommitValveFailfast = enable;
+ enableCommitValveFailfast = enable;
}
@Override
public boolean isBackingCacheAvailable() {
return sessionCache.isBackingCacheAvailable();
}
- @SuppressWarnings("unused")
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
Review comment:
Typo here, should be "Always prefer deserialized form."
##########
File path: extensions/geode-modules-tomcat9/src/test/java/org/apache/geode/modules/session/catalina/DeltaSession9Test.java
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.modules.session.catalina;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+
+import javax.servlet.http.HttpSessionAttributeListener;
+import javax.servlet.http.HttpSessionBindingEvent;
+
+import org.apache.catalina.Context;
+import org.apache.juli.logging.Log;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.internal.util.BlobHelper;
+
+public class DeltaSession9Test extends AbstractDeltaSessionTest {
+ final HttpSessionAttributeListener listener = mock(HttpSessionAttributeListener.class);
+
+ @Before
+ @Override
+ public void setup() {
+ super.setup();
+
+ final Context context = mock(Context.class);
+ when(manager.getContext()).thenReturn(context);
+ when(context.getApplicationEventListeners()).thenReturn(new Object[] {listener});
+ when(context.getLogger()).thenReturn(mock(Log.class));
+ }
+
+ @Test
+ public void serializedAttributesNotLeakedInAttributeReplaceEvent() throws IOException {
+ final DeltaSession9 session = spy(new DeltaSession9(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ final Object value2 = "value2";
+ session.setAttribute(name, value2);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeReplaced(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isEqualTo(value1);
+ }
+
+ @Test
+ public void serializedAttributesNotLeakedInAttributeRemovedEvent() throws IOException {
+ final DeltaSession9 session = spy(new DeltaSession9(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ session.removeAttribute(name);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeRemoved(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isEqualTo(value1);
+ }
+
+ @Test
+ public void serializedAttributesLeakedInAttributeReplaceEventWhenPreferSerializedFormFalse()
+ throws IOException {
+ setPreferSeserializedForm();
+
+ final DeltaSession9 session = spy(new DeltaSession9(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ final Object value2 = "value2";
+ session.setAttribute(name, value2);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeReplaced(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isInstanceOf(byte[].class);
+ }
+
+ @Test
+ public void serializedAttributesLeakedInAttributeRemovedEventWhenPreferSerializedFormFalse()
+ throws IOException {
+ setPreferSeserializedForm();
+
+ final DeltaSession9 session = spy(new DeltaSession9(manager));
+ session.setValid(true);
+ final String name = "attribute";
+ final Object value1 = "value1";
+ final byte[] serializedValue1 = BlobHelper.serializeToBlob(value1);
+ // simulates initial deserialized state with serialized attribute values.
+ session.getAttributes().put(name, serializedValue1);
+
+ session.removeAttribute(name);
+
+ final ArgumentCaptor<HttpSessionBindingEvent> event =
+ ArgumentCaptor.forClass(HttpSessionBindingEvent.class);
+ verify(listener).attributeRemoved(event.capture());
+ verifyNoMoreInteractions(listener);
+ assertThat(event.getValue().getValue()).isInstanceOf(byte[].class);
+ }
+
+ @SuppressWarnings("deprecation")
+ protected void setPreferSeserializedForm() {
Review comment:
See comment on DeltaSession7Test.java regarding this typo and method names.
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java
##########
@@ -213,64 +225,78 @@ public boolean getEnableGatewayDeltaReplication() {
return false; // disabled
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableGatewayDeltaReplication(boolean enableGatewayDeltaReplication) {
// this.enableGatewayDeltaReplication = enableGatewayDeltaReplication;
// Disabled. Keeping the method for backward compatibility.
}
@Override
public boolean getEnableGatewayReplication() {
- return this.enableGatewayReplication;
+ return enableGatewayReplication;
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableGatewayReplication(boolean enableGatewayReplication) {
this.enableGatewayReplication = enableGatewayReplication;
}
@Override
public boolean getEnableDebugListener() {
- return this.enableDebugListener;
+ return enableDebugListener;
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableDebugListener(boolean enableDebugListener) {
this.enableDebugListener = enableDebugListener;
}
@Override
public boolean isCommitValveEnabled() {
- return this.enableCommitValve;
+ return enableCommitValve;
}
+ @Override
public void setEnableCommitValve(boolean enable) {
- this.enableCommitValve = enable;
+ enableCommitValve = enable;
}
@Override
public boolean isCommitValveFailfastEnabled() {
- return this.enableCommitValveFailfast;
+ return enableCommitValveFailfast;
}
- @SuppressWarnings("unused")
+ @Override
public void setEnableCommitValveFailfast(boolean enable) {
- this.enableCommitValveFailfast = enable;
+ enableCommitValveFailfast = enable;
}
@Override
public boolean isBackingCacheAvailable() {
return sessionCache.isBackingCacheAvailable();
}
- @SuppressWarnings("unused")
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
+ */
+ @Deprecated
+ @Override
public void setPreferDeserializedForm(boolean enable) {
- this.preferDeserializedForm = enable;
+ log.warn("Use of deprecated preferDeserializedForm property to be removed in future release.");
+ if (!enable) {
+ log.warn(
+ "Use of HttpSessionAttributeListener may result in serialized form in HttpSessionBindingEvent.");
+ }
+ preferDeserializedForm = enable;
}
+ /**
+ * @deprecated No replacement. Always refer deserialized form.
Review comment:
Typo here, should be "Always prefer deserialized form."
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -616,7 +642,7 @@ public int getSizeInBytes() {
return size;
}
- @SuppressWarnings({"unchecked", "rawtypes"})
+ @SuppressWarnings({"unchecked"})
Review comment:
While we're here, it might be worth cleaning up uses of this warning suppression using the `uncheckedCast()` method, as used elsewhere in this change set. There is one cast in this method (line 651) and one on line 633 of `getSizeInBytes()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-eshu commented on a change in pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
pivotal-eshu commented on a change in pull request #5256:
URL: https://github.com/apache/geode/pull/5256#discussion_r445823337
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -242,12 +252,15 @@ public void setAttribute(String name, Object value, boolean notify) {
checkBackingCacheAvailable();
- synchronized (this.changeLock) {
+ synchronized (changeLock) {
// Serialize the value
byte[] serializedValue = serialize(value);
// Store the attribute locally
- if (this.preferDeserializedForm) {
+ if (preferDeserializedForm) {
+ if (notify) {
Review comment:
This seems unnecessary, as the next line code (super.setAttribute(name, value, true);) would override and set the attribute value again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-eshu commented on a change in pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
pivotal-eshu commented on a change in pull request #5256:
URL: https://github.com/apache/geode/pull/5256#discussion_r444393667
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -231,6 +236,11 @@ public void setOwner(Object manager) {
}
}
+ @SuppressWarnings("deprecation")
+ private void setOwnerDeprecated(DeltaSessionManager sessionManager) {
Review comment:
I think the method name does not reflect PreferDeserializedForm. It might be better to be explicit instead of using Deprecated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #5256:
URL: https://github.com/apache/geode/pull/5256#issuecomment-649736064
Sorry for the rebase but apparently Concourse is unable to function properly without 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5256:
URL: https://github.com/apache/geode/pull/5256#issuecomment-648449196
This pull request **fixes 1 alert** when merging 186bf85298b3606d1dff4d58c4673dd05859f1c4 into 91fd5f51ea99250753e249d2ef90a24543be9b7e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-b7b483dfc467397a30d02bdc824d32c41df35b32)
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-eshu commented on a change in pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
pivotal-eshu commented on a change in pull request #5256:
URL: https://github.com/apache/geode/pull/5256#discussion_r445823337
##########
File path: extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
##########
@@ -242,12 +252,15 @@ public void setAttribute(String name, Object value, boolean notify) {
checkBackingCacheAvailable();
- synchronized (this.changeLock) {
+ synchronized (changeLock) {
// Serialize the value
byte[] serializedValue = serialize(value);
// Store the attribute locally
- if (this.preferDeserializedForm) {
+ if (preferDeserializedForm) {
+ if (notify) {
Review comment:
This seems unnecessary, as the next line code (super.setAttribute(name, value, true);) would override and set the attribute value again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5256: GEODE-8217: Deserialize attribute before update and remove.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5256:
URL: https://github.com/apache/geode/pull/5256#issuecomment-649762229
This pull request **fixes 1 alert** when merging 374d4d2302b06ee6fd811f528aa2854aa94bdfe7 into 561533c53cf44e53c42f26cd988eae6821af6769 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-bb5ca721854e66197c7c3d4cfecd6c7653ab5586)
**fixed alerts:**
* 1 for Cast from abstract to concrete collection
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org