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/01/22 00:17:54 UTC
[GitHub] [geode] kirklund opened a new pull request #7299: DRAFT: GEODE-9980: Skip filter reflection if enableGlobalSerialFilter is false
kirklund opened a new pull request #7299:
URL: https://github.com/apache/geode/pull/7299
Improve error handling in ReflectiveFacadeGlobalSerialFilter classes. Handle
java.lang.ClassNotFoundException sun.misc.ObjectInputFilter.
Fix minor details in LocatorLauncher, InternalDataSerializer and tests.
Create ReflectiveFacadeGlobalSerialFilterFactoryIntegrationTest for validating
pre-8u121 expectations.
--
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] jinmeiliao commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r796269504
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeObjectInputFilter.java
##########
@@ -78,4 +77,36 @@ public String toString() {
ObjectInputFilterApi getObjectInputFilterApi() {
return api;
}
+
+ private void handleExceptionThrownByApi(ReflectiveOperationException e)
Review comment:
Yes
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792246812
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -38,7 +38,9 @@
private final SerializableObjectConfig serializableObjectConfig;
private final FilterPatternFactory filterPatternFactory;
private final Supplier<Set<String>> sanctionedClassesSupplier;
- private final Consumer<String> logger;
+ private final Consumer<String> infoLogger;
+ private final Consumer<String> warnLogger;
+ private final Consumer<String> errorLogger;
Review comment:
Fixed
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfigurationTest.java
##########
@@ -29,38 +29,60 @@
private SerializableObjectConfig config;
private GlobalSerialFilter globalSerialFilter;
- private Consumer<String> logger;
+ private Consumer<String> infoLogger;
+ private Consumer<String> warnLogger;
+ private Consumer<String> errorLogger;
@Before
public void setUp() {
config = mock(SerializableObjectConfig.class);
globalSerialFilter = mock(GlobalSerialFilter.class);
- logger = uncheckedCast(mock(Consumer.class));
+ infoLogger = uncheckedCast(mock(Consumer.class));
+ warnLogger = uncheckedCast(mock(Consumer.class));
+ errorLogger = uncheckedCast(mock(Consumer.class));
}
@Test
- public void configureLogs_whenUnsupportedOperationExceptionIsThrown_withCause() {
+ public void configureLogsInfo_whenOperationIsSuccessful() {
Review comment:
Fixed
--
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] demery-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792020273
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfigurationOnOlderJdkIntegrationTest.java
##########
@@ -0,0 +1,134 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static java.util.Collections.emptySet;
+import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+
+import java.util.function.Consumer;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+/**
+ * Note: This is a manual test that requires running on an older version of Java 8.
Review comment:
Could this test be written in a way that does not require manual intervention, such as by exercising the filters in a subprocess launched with an older JRE?
--
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] demery-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792077305
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -41,8 +49,9 @@ public void constructsDelegatingGlobalSerialFilter() {
public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
throws InvocationTargetException, IllegalAccessException {
ObjectInputFilterApi api = mock(ObjectInputFilterApi.class);
- GlobalSerialFilterFactory factory = new ReflectiveFacadeGlobalSerialFilterFactory(api);
- GlobalSerialFilter filter = factory.create("pattern", emptySet());
+ ReflectiveFacadeGlobalSerialFilterFactory factory =
+ new ReflectiveFacadeGlobalSerialFilterFactory();
+ GlobalSerialFilter filter = factory.create(api, "pattern", emptySet());
Review comment:
This tests an implementation detail, and leaves the actual responsibility (invoked via the other `create()` method) untested.
My sense is that it's just as easy to pass the api through the constructor, and that there's no harm making that constructor visible for even non-test code. Even if you want to restrict that constructor only to tests, invoking the real `create()` here exercises the responsibility more fully (everything except supplying the api used in production).
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r797137624
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/StreamSerialFilterFactory.java
##########
@@ -17,8 +17,8 @@
import java.util.Set;
@FunctionalInterface
-public interface ObjectInputFilterFactory {
+public interface StreamSerialFilterFactory {
- ObjectInputFilter create(SerializableObjectConfig serializableObjectConfig,
- Set<String> sanctionedClasses);
+ StreamSerialFilter create(SerializableObjectConfig serializableObjectConfig,
Review comment:
Reformat parameters
--
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] demery-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792077811
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -57,8 +66,9 @@ public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
public void delegatesToObjectInputFilterApiToSetSerialFilter()
Review comment:
Similar comments to the previous test re: the name and testing an implementation detail.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r791837623
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfigurationTest.java
##########
@@ -29,38 +29,60 @@
private SerializableObjectConfig config;
private GlobalSerialFilter globalSerialFilter;
- private Consumer<String> logger;
+ private Consumer<String> infoLogger;
+ private Consumer<String> warnLogger;
+ private Consumer<String> errorLogger;
@Before
public void setUp() {
config = mock(SerializableObjectConfig.class);
globalSerialFilter = mock(GlobalSerialFilter.class);
- logger = uncheckedCast(mock(Consumer.class));
+ infoLogger = uncheckedCast(mock(Consumer.class));
+ warnLogger = uncheckedCast(mock(Consumer.class));
+ errorLogger = uncheckedCast(mock(Consumer.class));
}
@Test
- public void configureLogs_whenUnsupportedOperationExceptionIsThrown_withCause() {
+ public void configureLogsInfo_whenOperationIsSuccessful() {
Review comment:
I think we should remove `configure` from the beginning of these test names since it's a `FunctionalInterface` with just one method.
--
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] demery-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792054961
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -38,7 +38,9 @@
private final SerializableObjectConfig serializableObjectConfig;
private final FilterPatternFactory filterPatternFactory;
private final Supplier<Set<String>> sanctionedClassesSupplier;
- private final Consumer<String> logger;
+ private final Consumer<String> infoLogger;
+ private final Consumer<String> warnLogger;
+ private final Consumer<String> errorLogger;
Review comment:
I think so, for several reasons:
- This API locks the implementation into accepting only strings. At some point, the implementation may want to use a richer logging API. (On the other hand: YAGNI.)
- The name `accept()` feels oddly passive, as if code is politely requesting that the logger please consider doing a thing. A logging API would make it clear that the code is telling the logger to log something.
- A logging API allows a test to observe all of the same interactions.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r793154064
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -57,8 +66,9 @@ public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
public void delegatesToObjectInputFilterApiToSetSerialFilter()
Review comment:
Note: I removed tests from ReflectiveFacadeGlobalSerialFilterFactoryTest that were redundant with tests in ReflectiveFacadeGlobalSerialFilterTest and *ApiTest.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r793150063
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -41,8 +49,9 @@ public void constructsDelegatingGlobalSerialFilter() {
public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
Review comment:
Fixed up ReflectiveFacadeGlobalSerialFilterFactoryTest
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792246971
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfigurationOnOlderJdkIntegrationTest.java
##########
@@ -0,0 +1,134 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static java.util.Collections.emptySet;
+import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+
+import java.util.function.Consumer;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+/**
+ * Note: This is a manual test that requires running on an older version of Java 8.
Review comment:
Fixed
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest.java
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static org.apache.commons.lang3.JavaVersion.JAVA_1_8;
+import static org.apache.commons.lang3.SystemUtils.isJavaVersionAtMost;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+public class SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest {
+
+ private SerializableObjectConfig serializableObjectConfig;
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Before
+ public void setUp() {
+ serializableObjectConfig = mock(SerializableObjectConfig.class);
+ }
+
+ @Test
+ public void throwsUnsupportedOperationException_whenEnableGlobalSerialFilterIsTrue_onLessThanJava8u121() {
+ assumeThat(isJavaVersionAtMost(JAVA_1_8)).isTrue();
+
+ System.setProperty("geode.enableGlobalSerialFilter", "true");
+
+ // TODO:KIRK: move to acceptance test to validate expected logging
+ // TODO:KIRK: improve consistency between GlobalSerialFilter and ObjectInputFilter
+ // TODO:KIRK: uplift logging severity to warn or error
+
+ FilterConfiguration configuration = new SystemPropertyGlobalSerialFilterConfigurationFactory()
+ .create(serializableObjectConfig);
+ configuration.configure();
+ }
+
+ @Test
+ public void doesNothing_whenEnableGlobalSerialFilterIsFalse_onLessThanJava8u121() {
Review comment:
Fixed
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r791831466
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -38,7 +38,9 @@
private final SerializableObjectConfig serializableObjectConfig;
private final FilterPatternFactory filterPatternFactory;
private final Supplier<Set<String>> sanctionedClassesSupplier;
- private final Consumer<String> logger;
+ private final Consumer<String> infoLogger;
+ private final Consumer<String> warnLogger;
+ private final Consumer<String> errorLogger;
Review comment:
Would it be cleaner and still testable to use a single log4j-api Logger interface instead of three Consumer? The tests need to differentiate between three different log levels: INFO, WARN and ERROR.
--
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] kirklund merged pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund merged pull request #7299:
URL: https://github.com/apache/geode/pull/7299
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r796177955
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeObjectInputFilter.java
##########
@@ -78,4 +77,36 @@ public String toString() {
ObjectInputFilterApi getObjectInputFilterApi() {
return api;
}
+
+ private void handleExceptionThrownByApi(ReflectiveOperationException e)
Review comment:
I can combine them into a new ErrorHandler class.
Do the changes to ManagementAgent look ok?
--
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] kirklund edited a comment on pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund edited a comment on pull request #7299:
URL: https://github.com/apache/geode/pull/7299#issuecomment-1027324608
This new unit test failure seems to indicate that something in unit tests set a process-wide filter which I've been very careful to avoid. The test that fails is asserting that the process-wide filter is null which then fails because toString is not implemented on the JDK's filter that gets returned. I think it would be safest to change the reflective API wrapper to return boolean that either there is a filter or it's null.
I could also store a stack trace that captures whatever sets the filter so I can easily find the code path that sets it. Then if no stack trace is stored, we could also detect when the filter has been set outside Geode's internal filter API which might even be useful outside of precheckin tests.
--
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] kirklund commented on pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on pull request #7299:
URL: https://github.com/apache/geode/pull/7299#issuecomment-1028247811
Rename Geode's ObjectInputFilter to StreamSerialFilter to avoid name collision with JDK
--
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] jchen21 commented on pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
jchen21 commented on pull request #7299:
URL: https://github.com/apache/geode/pull/7299#issuecomment-1030414739
Changes on the invocation handler ([168c701](https://github.com/apache/geode/pull/7299/commits/168c701e54b74aaf118fa14e37e6154efe69425c)) looks good to me.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r797137138
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeStreamSerialFilter.java
##########
@@ -37,8 +37,7 @@
/**
* Constructs instance with the specified collaborators.
*/
- ReflectiveFacadeObjectInputFilter(ObjectInputFilterApi api, String pattern,
- Collection<String> sanctionedClasses, ReflectiveOperationExceptionHandler errorHandler) {
+ ReflectiveFacadeStreamSerialFilter(ObjectInputFilterApi api, String pattern, Collection<String> sanctionedClasses, ReflectiveOperationExceptionHandler errorHandler) {
Review comment:
Reformat linefeeds in parameters
--
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] dschneider-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r791947439
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
globalSerialFilter.setFilter();
// log statement that filter is now configured
- logger.accept("Global serial filter is now configured.");
+ infoLogger.accept("Global serial filter is now configured.");
return true;
} catch (UnsupportedOperationException e) {
- if (hasRootCauseWithMessage(e, IllegalStateException.class,
- "Serial filter can only be set once")) {
-
- // log statement that filter was already configured
- logger.accept("Global serial filter is already configured.");
- }
+ handleUnsupportedOperationException(e);
return false;
}
}
- private static boolean hasRootCauseWithMessage(Throwable throwable,
+ private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+ if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+ "Serial filter can only be set once")) {
+
+ // log statement that filter was already configured
+ warnLogger.accept("Global serial filter is already configured.");
+ }
+ if (hasRootCauseWithMessageContaining(e, ClassNotFoundException.class,
+ "ObjectInputFilter")) {
+
+ // log statement that a global serial filter cannot be configured
+ errorLogger.accept(
+ "Geode was unable to configure a global serialization filter because ObjectInputFilter not found.");
Review comment:
Is this a security issue? Should the security logger be used? I don't actually know when it should be used.
Will user's understand that this error is expected on older jdks and can be fixed by using a newer one? It seems to me like this msg should contain some more info.
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
globalSerialFilter.setFilter();
// log statement that filter is now configured
- logger.accept("Global serial filter is now configured.");
+ infoLogger.accept("Global serial filter is now configured.");
return true;
} catch (UnsupportedOperationException e) {
- if (hasRootCauseWithMessage(e, IllegalStateException.class,
- "Serial filter can only be set once")) {
-
- // log statement that filter was already configured
- logger.accept("Global serial filter is already configured.");
- }
+ handleUnsupportedOperationException(e);
return false;
}
}
- private static boolean hasRootCauseWithMessage(Throwable throwable,
+ private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+ if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+ "Serial filter can only be set once")) {
+
+ // log statement that filter was already configured
+ warnLogger.accept("Global serial filter is already configured.");
Review comment:
Why is this a warning instead of info?
If a user sees this warning what action should they take to figure out if something is wrong?
Here we say "serial filter". In the next msg it is "serialization filter". It seems like these should be consistent.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r791827736
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest.java
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static org.apache.commons.lang3.JavaVersion.JAVA_1_8;
+import static org.apache.commons.lang3.SystemUtils.isJavaVersionAtMost;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+public class SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest {
+
+ private SerializableObjectConfig serializableObjectConfig;
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Before
+ public void setUp() {
+ serializableObjectConfig = mock(SerializableObjectConfig.class);
+ }
+
+ @Test
+ public void throwsUnsupportedOperationException_whenEnableGlobalSerialFilterIsTrue_onLessThanJava8u121() {
+ assumeThat(isJavaVersionAtMost(JAVA_1_8)).isTrue();
+
+ System.setProperty("geode.enableGlobalSerialFilter", "true");
+
+ // TODO:KIRK: move to acceptance test to validate expected logging
+ // TODO:KIRK: improve consistency between GlobalSerialFilter and ObjectInputFilter
+ // TODO:KIRK: uplift logging severity to warn or error
Review comment:
Remove these TODO:KIRK comments.
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -38,7 +38,9 @@
private final SerializableObjectConfig serializableObjectConfig;
private final FilterPatternFactory filterPatternFactory;
private final Supplier<Set<String>> sanctionedClassesSupplier;
- private final Consumer<String> logger;
+ private final Consumer<String> infoLogger;
+ private final Consumer<String> warnLogger;
+ private final Consumer<String> errorLogger;
Review comment:
Would it be cleaner and still testable to use a single log4j-api Logger interface instead of three Consumer? The tests need to differentiate between three different log levels: INFO, WARN and ERROR.
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfigurationTest.java
##########
@@ -29,38 +29,60 @@
private SerializableObjectConfig config;
private GlobalSerialFilter globalSerialFilter;
- private Consumer<String> logger;
+ private Consumer<String> infoLogger;
+ private Consumer<String> warnLogger;
+ private Consumer<String> errorLogger;
@Before
public void setUp() {
config = mock(SerializableObjectConfig.class);
globalSerialFilter = mock(GlobalSerialFilter.class);
- logger = uncheckedCast(mock(Consumer.class));
+ infoLogger = uncheckedCast(mock(Consumer.class));
+ warnLogger = uncheckedCast(mock(Consumer.class));
+ errorLogger = uncheckedCast(mock(Consumer.class));
}
@Test
- public void configureLogs_whenUnsupportedOperationExceptionIsThrown_withCause() {
+ public void configureLogsInfo_whenOperationIsSuccessful() {
Review comment:
I think we should remove `configure` from the beginning of these test names since it's a `FunctionalInterface` with just one method.
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -16,23 +16,31 @@
import static java.util.Collections.emptySet;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.lang.reflect.InvocationTargetException;
+import java.util.function.Supplier;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
public class ReflectiveFacadeGlobalSerialFilterFactoryTest {
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
@Test
public void constructsDelegatingGlobalSerialFilter() {
Review comment:
I just noticed that a previous rename of `DelegatingGlobalSerialFilter` to `ReflectiveFacadeGlobalSerialFIlter` missed a bunch of test names.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r793150143
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -41,8 +49,9 @@ public void constructsDelegatingGlobalSerialFilter() {
public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
throws InvocationTargetException, IllegalAccessException {
ObjectInputFilterApi api = mock(ObjectInputFilterApi.class);
- GlobalSerialFilterFactory factory = new ReflectiveFacadeGlobalSerialFilterFactory(api);
- GlobalSerialFilter filter = factory.create("pattern", emptySet());
+ ReflectiveFacadeGlobalSerialFilterFactory factory =
+ new ReflectiveFacadeGlobalSerialFilterFactory();
+ GlobalSerialFilter filter = factory.create(api, "pattern", emptySet());
Review comment:
Fixed up ReflectiveFacadeGlobalSerialFilterFactoryTest
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -57,8 +66,9 @@ public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
public void delegatesToObjectInputFilterApiToSetSerialFilter()
Review comment:
Fixed up ReflectiveFacadeGlobalSerialFilterFactoryTest
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r791827736
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest.java
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static org.apache.commons.lang3.JavaVersion.JAVA_1_8;
+import static org.apache.commons.lang3.SystemUtils.isJavaVersionAtMost;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+public class SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest {
+
+ private SerializableObjectConfig serializableObjectConfig;
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Before
+ public void setUp() {
+ serializableObjectConfig = mock(SerializableObjectConfig.class);
+ }
+
+ @Test
+ public void throwsUnsupportedOperationException_whenEnableGlobalSerialFilterIsTrue_onLessThanJava8u121() {
+ assumeThat(isJavaVersionAtMost(JAVA_1_8)).isTrue();
+
+ System.setProperty("geode.enableGlobalSerialFilter", "true");
+
+ // TODO:KIRK: move to acceptance test to validate expected logging
+ // TODO:KIRK: improve consistency between GlobalSerialFilter and ObjectInputFilter
+ // TODO:KIRK: uplift logging severity to warn or error
Review comment:
Remove these TODO:KIRK comments.
--
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] kirklund commented on pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on pull request #7299:
URL: https://github.com/apache/geode/pull/7299#issuecomment-1026349954
AnalyzeSerializationSerializablesIntegrationTest failed because I need to update the sanctioned serializables for geode-serialization.
--
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] dschneider-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r793842812
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
globalSerialFilter.setFilter();
// log statement that filter is now configured
- logger.accept("Global serial filter is now configured.");
+ infoLogger.accept("Global serial filter is now configured.");
return true;
} catch (UnsupportedOperationException e) {
- if (hasRootCauseWithMessage(e, IllegalStateException.class,
- "Serial filter can only be set once")) {
-
- // log statement that filter was already configured
- logger.accept("Global serial filter is already configured.");
- }
+ handleUnsupportedOperationException(e);
return false;
}
}
- private static boolean hasRootCauseWithMessage(Throwable throwable,
+ private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+ if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+ "Serial filter can only be set once")) {
+
+ // log statement that filter was already configured
+ warnLogger.accept("Global serial filter is already configured.");
+ }
+ if (hasRootCauseWithMessageContaining(e, ClassNotFoundException.class,
+ "ObjectInputFilter")) {
+
+ // log statement that a global serial filter cannot be configured
+ errorLogger.accept(
+ "Geode was unable to configure a global serialization filter because ObjectInputFilter not found.");
Review comment:
I think you are correct in making in stronger than "info". I think it could also be helpful to tie it back directly to the sys prop that was set. This seems like an important enough security feature that if the user explicitly configured it and we were not able to honor that request that it should case startup to fail. They could then correct their environment so it can be secure or remove the sys prop requesting it. But if failing startup is not an option then "error" is better than "info".
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792248994
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
globalSerialFilter.setFilter();
// log statement that filter is now configured
- logger.accept("Global serial filter is now configured.");
+ infoLogger.accept("Global serial filter is now configured.");
return true;
} catch (UnsupportedOperationException e) {
- if (hasRootCauseWithMessage(e, IllegalStateException.class,
- "Serial filter can only be set once")) {
-
- // log statement that filter was already configured
- logger.accept("Global serial filter is already configured.");
- }
+ handleUnsupportedOperationException(e);
return false;
}
}
- private static boolean hasRootCauseWithMessage(Throwable throwable,
+ private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+ if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+ "Serial filter can only be set once")) {
+
+ // log statement that filter was already configured
+ warnLogger.accept("Global serial filter is already configured.");
+ }
+ if (hasRootCauseWithMessageContaining(e, ClassNotFoundException.class,
+ "ObjectInputFilter")) {
+
+ // log statement that a global serial filter cannot be configured
+ errorLogger.accept(
+ "Geode was unable to configure a global serialization filter because ObjectInputFilter not found.");
Review comment:
@dschneider-pivotal It's used to ensure a secured JVM but it doesn't belong to the security logger. This is actually the same log statement that Bruce originally added years ago but changed to error log level. I think I should change all of these back to info and polish up the wording for consistency and to make sure the user knows what to do if it occurs.
I originally changed it to error just to get the user's attention. If they are trying to rely on security the JVM by setting `-Dgeode.enableGlobalSerialFilter=true` then I thought they might need this statement to be above info level just so they notice it.
Any more thoughts on this before I change 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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r793836381
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
globalSerialFilter.setFilter();
// log statement that filter is now configured
- logger.accept("Global serial filter is now configured.");
+ infoLogger.accept("Global serial filter is now configured.");
return true;
} catch (UnsupportedOperationException e) {
- if (hasRootCauseWithMessage(e, IllegalStateException.class,
- "Serial filter can only be set once")) {
-
- // log statement that filter was already configured
- logger.accept("Global serial filter is already configured.");
- }
+ handleUnsupportedOperationException(e);
return false;
}
}
- private static boolean hasRootCauseWithMessage(Throwable throwable,
+ private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+ if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+ "Serial filter can only be set once")) {
+
+ // log statement that filter was already configured
+ warnLogger.accept("Global serial filter is already configured.");
Review comment:
thanks
--
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] demery-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792048199
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest.java
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static org.apache.commons.lang3.JavaVersion.JAVA_1_8;
+import static org.apache.commons.lang3.SystemUtils.isJavaVersionAtMost;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+public class SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest {
+
+ private SerializableObjectConfig serializableObjectConfig;
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Before
+ public void setUp() {
+ serializableObjectConfig = mock(SerializableObjectConfig.class);
+ }
+
+ @Test
+ public void throwsUnsupportedOperationException_whenEnableGlobalSerialFilterIsTrue_onLessThanJava8u121() {
+ assumeThat(isJavaVersionAtMost(JAVA_1_8)).isTrue();
+
+ System.setProperty("geode.enableGlobalSerialFilter", "true");
+
+ // TODO:KIRK: move to acceptance test to validate expected logging
+ // TODO:KIRK: improve consistency between GlobalSerialFilter and ObjectInputFilter
+ // TODO:KIRK: uplift logging severity to warn or error
+
+ FilterConfiguration configuration = new SystemPropertyGlobalSerialFilterConfigurationFactory()
+ .create(serializableObjectConfig);
+ configuration.configure();
+ }
+
+ @Test
+ public void doesNothing_whenEnableGlobalSerialFilterIsFalse_onLessThanJava8u121() {
Review comment:
`doesNotThrow` seems more accurate.
--
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] jchen21 commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792047728
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfigurationOnOlderJdkIntegrationTest.java
##########
@@ -0,0 +1,134 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static java.util.Collections.emptySet;
+import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+
+import java.util.function.Consumer;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+/**
+ * Note: This is a manual test that requires running on an older version of Java 8.
Review comment:
Or maybe mock a java.lang.ClassNotFoundException: sun.misc.ObjectInputFilter?
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792249281
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -41,8 +49,9 @@ public void constructsDelegatingGlobalSerialFilter() {
public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
throws InvocationTargetException, IllegalAccessException {
ObjectInputFilterApi api = mock(ObjectInputFilterApi.class);
- GlobalSerialFilterFactory factory = new ReflectiveFacadeGlobalSerialFilterFactory(api);
- GlobalSerialFilter filter = factory.create("pattern", emptySet());
+ ReflectiveFacadeGlobalSerialFilterFactory factory =
+ new ReflectiveFacadeGlobalSerialFilterFactory();
+ GlobalSerialFilter filter = factory.create(api, "pattern", emptySet());
Review comment:
Yeah, after reviewing this test class myself, I'm really not happy with where it's currently at.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r791841023
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -16,23 +16,31 @@
import static java.util.Collections.emptySet;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.lang.reflect.InvocationTargetException;
+import java.util.function.Supplier;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
public class ReflectiveFacadeGlobalSerialFilterFactoryTest {
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
@Test
public void constructsDelegatingGlobalSerialFilter() {
Review comment:
I just noticed that a previous rename of `DelegatingGlobalSerialFilter` to `ReflectiveFacadeGlobalSerialFIlter` missed a bunch of test names.
--
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] jinmeiliao commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r796158668
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/UnableToSetSerialFilterException.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.internal.serialization.filter;
+
+/**
+ * Checked exception thrown when there's a failure using Java's ObjectInputFilter. All uses of this
+ * exception are caught and rethrown before reaching the user.
+ */
+public class UnableToSetSerialFilterException extends Exception {
Review comment:
Seems like several usage of this exception is to catch this and turn it into a RuntimeException. In my experience, checked exception are used when caller can do something about this exception. If caller can not do anything about this exception, would it be better to make this an unchecked exception?
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeObjectInputFilter.java
##########
@@ -78,4 +77,36 @@ public String toString() {
ObjectInputFilterApi getObjectInputFilterApi() {
return api;
}
+
+ private void handleExceptionThrownByApi(ReflectiveOperationException e)
Review comment:
this looks like repeated code with `ReflectiveFacadeGlobalSerialFilter`, anyway to consolidate them?
--
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] kirklund commented on pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on pull request #7299:
URL: https://github.com/apache/geode/pull/7299#issuecomment-1026085748
I have one local commit that I believe should be pushed before merging this PR. Startup of a Locator or Server should fail rather than just log a message if the user specifies `-Dgeode.enableGlobalSerialFilter` but a global serialization filter cannot be configured for any reason.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r796177271
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/UnableToSetSerialFilterException.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.internal.serialization.filter;
+
+/**
+ * Checked exception thrown when there's a failure using Java's ObjectInputFilter. All uses of this
+ * exception are caught and rethrown before reaching the user.
+ */
+public class UnableToSetSerialFilterException extends Exception {
Review comment:
After using unchecked exceptions for internal-only exceptions resulted in some of these slipping out to customers, we came up with the rule that internal exceptions should be checked. It's ok for external User API exceptions to be unchecked though.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r793149989
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
globalSerialFilter.setFilter();
// log statement that filter is now configured
- logger.accept("Global serial filter is now configured.");
+ infoLogger.accept("Global serial filter is now configured.");
return true;
} catch (UnsupportedOperationException e) {
- if (hasRootCauseWithMessage(e, IllegalStateException.class,
- "Serial filter can only be set once")) {
-
- // log statement that filter was already configured
- logger.accept("Global serial filter is already configured.");
- }
+ handleUnsupportedOperationException(e);
return false;
}
}
- private static boolean hasRootCauseWithMessage(Throwable throwable,
+ private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+ if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+ "Serial filter can only be set once")) {
+
+ // log statement that filter was already configured
+ warnLogger.accept("Global serial filter is already configured.");
Review comment:
I changed the logging statements to be consistent. I left this one at WARNING level and lowered the ERROR statement to WARNING as well. I also added information for the User to address the cause of the warnings.
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r793135968
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -16,23 +16,31 @@
import static java.util.Collections.emptySet;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.lang.reflect.InvocationTargetException;
+import java.util.function.Supplier;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
public class ReflectiveFacadeGlobalSerialFilterFactoryTest {
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
@Test
public void constructsDelegatingGlobalSerialFilter() {
Review comment:
Fixed
--
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] demery-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792069425
##########
File path: geode-serialization/src/test/java/org/apache/geode/internal/serialization/filter/ReflectiveFacadeGlobalSerialFilterFactoryTest.java
##########
@@ -41,8 +49,9 @@ public void constructsDelegatingGlobalSerialFilter() {
public void delegatesToObjectInputFilterApiToCreateObjectInputFilter()
Review comment:
This test name seems to describe what the created filter does, not what the factory does. Consider something like `createsFilterThat…`
--
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] kirklund commented on pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on pull request #7299:
URL: https://github.com/apache/geode/pull/7299#issuecomment-1021763286
Pushed several fixes and marked those comments as fixed and resolved.
I still need to resolve:
* fixing log statements and log levels in GlobalSerialFilterConfiguration
* testing implementation details instead of behavior in ReflectiveFacadeGlobalSerialFilterFactoryTest
--
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] kirklund commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r792227822
##########
File path: geode-serialization/src/integrationTest/java/org/apache/geode/internal/serialization/filter/SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest.java
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.internal.serialization.filter;
+
+import static org.apache.commons.lang3.JavaVersion.JAVA_1_8;
+import static org.apache.commons.lang3.SystemUtils.isJavaVersionAtMost;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assumptions.assumeThat;
+import static org.mockito.Mockito.mock;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+public class SystemPropertyGlobalSerialFilterConfigurationFactoryIntegrationTest {
+
+ private SerializableObjectConfig serializableObjectConfig;
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Before
+ public void setUp() {
+ serializableObjectConfig = mock(SerializableObjectConfig.class);
+ }
+
+ @Test
+ public void throwsUnsupportedOperationException_whenEnableGlobalSerialFilterIsTrue_onLessThanJava8u121() {
+ assumeThat(isJavaVersionAtMost(JAVA_1_8)).isTrue();
+
+ System.setProperty("geode.enableGlobalSerialFilter", "true");
+
+ // TODO:KIRK: move to acceptance test to validate expected logging
+ // TODO:KIRK: improve consistency between GlobalSerialFilter and ObjectInputFilter
+ // TODO:KIRK: uplift logging severity to warn or error
Review comment:
Removed
--
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] kirklund commented on pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter
Posted by GitBox <gi...@apache.org>.
kirklund commented on pull request #7299:
URL: https://github.com/apache/geode/pull/7299#issuecomment-1027324608
This new unit test failure seems to indicate that something in unit tests set a process-wide filter which I've been very careful to avoid. The test that fails is asserting that the process-wide filter is null which then fails because toString is not implemented on the JDK's filter that gets returned. I think it would be safest to change the reflective API wrapper to return boolean that either there is a filter or it's 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