kirklund commented on a change in pull request #7167: URL: https://github.com/apache/geode/pull/7167#discussion_r766245881
########## File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/DelegatingObjectInputFilterFactory.java ########## @@ -0,0 +1,57 @@ +/* + * 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.Objects.requireNonNull; + +import java.util.Set; + +import org.apache.geode.internal.serialization.filter.impl.DelegatingObjectInputFilter; +import org.apache.geode.internal.serialization.filter.impl.ObjectInputFilterApi; +import org.apache.geode.internal.serialization.filter.impl.ObjectInputFilterApiFactory; +import org.apache.geode.internal.serialization.filter.impl.ReflectionObjectInputFilterApiFactory; + +public class DelegatingObjectInputFilterFactory implements ObjectInputFilterFactory { + + private final Runnable precondition; + private final ObjectInputFilterApiFactory apiFactory; + + public DelegatingObjectInputFilterFactory(Runnable precondition) { + this(new ReflectionObjectInputFilterApiFactory(), precondition); + } + + private DelegatingObjectInputFilterFactory(ObjectInputFilterApiFactory apiFactory, + Runnable precondition) { + this.apiFactory = requireNonNull(apiFactory, "apiFactory is required"); + this.precondition = requireNonNull(precondition, "precondition is required"); + } + + @Override + public ObjectInputFilter create(SerializableObjectConfig config, Set<String> sanctionedClasses) { + if (config.getValidateSerializableObjects()) { + precondition.run(); Review comment: The factories in this API package are primarily there to provide a layer of indirection to facilitate good unit testing and separation of concerns. You could completely rewrite this entire package in several different styles. This style emphasizes TDD and unit testing of everything (despite the fact that many of my tests are in the next PR). The precondition runnable could be better organized in a decorator that wraps DelegatingObjectInputFilterFactory. The reason this runnable exists is because it needs to be executed after `if (config.getValidateSerializableObjects()) {`. It also needs to be injected from geode-core because that's where GemFireConfigException lives. I believe the serialization filtering code can be generic enough to live in geode-serialization which is where I created it. The more modular we make Geode, the more we'll have some interesting bits like this. Much of the structure is to minimize if-blocks or other logic in the calling code for separation of concerns. None of this has to do with polymorphism. It only has to do with coding to interfaces, writing unit tests, separating concerns, and making code that is readable. I do not believe that there is any problem with creating a factory instance that will be almost immediately garbaged collected. The instance will not be promoted to any of the memory spaces that put pressure on memory or garbage collection. Long-lived instances are always the problem, not short-lived ones unless you create a large number of instances. One-offs like this that occur once during the lifecycle of a Java process will be no slower or more pressuring on memory than reorganizing the code as a static block. Most code can be organized in quite a few different ways. This code could certainly be inlined as static code but it would be problematic for testing code that depends on it. I won't claim that this is the perfect or best structuring possible, but given the goals I have for this code, I think this is the best we can do within a reasonable amount of time. If you'd like to learn more about this style of coding, I recommend taking turns pairing with Dale and me so that you start to pick up how we use factories and functional programming to facilitate unit testing. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
