From 415f23216f7df8d9e212d504188ed0569780a440 Mon Sep 17 00:00:00 2001 From: John Blum Date: Thu, 31 Oct 2013 15:08:12 -0700 Subject: [PATCH] Additional refactoring to the PdxDiskStoreAwareBeanFactoryPostProcessor class based on code review by David Turnaski in PR #31 for JIRA issue SGF-197. --- ...iskStoreAwareBeanFactoryPostProcessor.java | 191 +++--------------- ...toreAwareBeanFactoryPostProcessorTest.java | 76 +++---- 2 files changed, 53 insertions(+), 214 deletions(-) diff --git a/src/main/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessor.java b/src/main/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessor.java index 26f3bddc..9d7f1450 100644 --- a/src/main/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessor.java +++ b/src/main/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessor.java @@ -17,15 +17,10 @@ package org.springframework.data.gemfire.config; import org.springframework.beans.BeansException; -import org.springframework.beans.PropertyValue; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.BeanFactoryPostProcessor; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; -import org.springframework.beans.factory.support.AbstractBeanDefinition; -import org.springframework.data.gemfire.DiskStoreFactoryBean; -import org.springframework.data.gemfire.RegionLookupFactoryBean; import org.springframework.data.gemfire.util.ArrayUtils; -import org.springframework.data.gemfire.wan.AsyncEventQueueFactoryBean; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -34,26 +29,21 @@ import com.gemstone.gemfire.cache.Region; import com.gemstone.gemfire.cache.asyncqueue.AsyncEventQueue; /** - * The PdxDiskStoreAwareBeanFactoryPostProcessor class is a BeanFactoryPostProcessor that modifies all Region bean - * definitions in the Spring BeanFactory to form a dependency on the Cache's PDX Disk Store bean. A persistent Region - * may contain PDX typed data, in which case, the PDX type meta-data stored to disk needs to be loaded before the Region - * having PDX data is loaded from disk. + * The PdxDiskStoreAwareBeanFactoryPostProcessor class is a BeanFactoryPostProcessor that modifies all Async Event Queue, + * Region and Disk Store beans in the Spring container to form a dependency on the Cache's PDX Disk Store bean. + * A persistent Region may contain PDX typed data, in which case, the PDX type meta-data stored to disk needs to be + * loaded before the Region having PDX data is loaded from disk. *

* @author John Blum - * @see org.springframework.beans.factory.config.BeanDefinition * @see org.springframework.beans.factory.config.BeanFactoryPostProcessor * @see org.springframework.beans.factory.config.ConfigurableListableBeanFactory + * @see com.gemstone.gemfire.cache.DiskStore + * @see com.gemstone.gemfire.cache.Region + * @see com.gemstone.gemfire.cache.asyncqueue.AsyncEventQueue * @since 1.3.3 */ -@SuppressWarnings("unused") public class PdxDiskStoreAwareBeanFactoryPostProcessor implements BeanFactoryPostProcessor { - protected static final String DATA_POLICY_PROPERTY = "dataPolicy"; - protected static final String DATA_POLICY_NAME_PROPERTY = "dataPolicyName"; - protected static final String PERSISTENT_KEYWORD = "PERSISTENT"; - protected static final String PERSISTENT_PROPERTY = "persistent"; - protected static final String SHORTCUT_PROPERTY = "shortcut"; - protected static final String[] EMPTY_STRING_ARRAY = new String[0]; private final String pdxDiskStoreName; @@ -69,170 +59,48 @@ public class PdxDiskStoreAwareBeanFactoryPostProcessor implements BeanFactoryPos @Override public void postProcessBeanFactory(final ConfigurableListableBeanFactory beanFactory) throws BeansException { - Assert.state(!beanFactory.isConfigurationFrozen(), - "The BeanFactory configuration meta-data is frozen and cannot be modified further!"); - //postProcessPdxDiskStoreDependencies(beanFactory); postProcessPdxDiskStoreDependencies(beanFactory, AsyncEventQueue.class, DiskStore.class, Region.class); - //postProcessPdxDiskStoreDependencies(beanFactory, AsyncEventQueueFactoryBean.class, DiskStoreFactoryBean.class, - // RegionLookupFactoryBean.class); } - private void postProcessPdxDiskStoreDependencies(final ConfigurableListableBeanFactory beanFactory, final Class... beanTypes) { + /** + * Post processes all beans in the Spring container, application context that may potentially have a dependency, + * or requirement on the PDX-based Disk Store being present before the bean itself can be processed and created. + * For instance, Regions that might contained PDX-based Key types require the PDX type meta-data to be loaded + * before the Region's data (Keys) are accessed. + *

+ * @param beanFactory the BeanFactory used to evaluate beans in context for PDX Disk Store dependencies. + * @param beanTypes an array of Class types indicating the type of beans to evaluate. + * @see org.springframework.beans.factory.config.ConfigurableListableBeanFactory#getBeanNamesForType(Class) + */ + private void postProcessPdxDiskStoreDependencies(final ConfigurableListableBeanFactory beanFactory, + final Class... beanTypes) { for (Class beanType : beanTypes) { for (String beanName : beanFactory.getBeanNamesForType(beanType)) { if (!beanName.equalsIgnoreCase(getPdxDiskStoreName())) { BeanDefinition beanDefinition = beanFactory.getBeanDefinition(beanName); - if (isDiskStore(beanDefinition) || (isPersistent(beanDefinition) - && !hasDiskStoreReference(beanFactory, beanDefinition))) { - addPdxDiskStoreDependency(beanDefinition); - } - } - } - } - } - - // TODO remove - private void postProcessPdxDiskStoreDependencies(final ConfigurableListableBeanFactory beanFactory) { - for (String beanName : beanFactory.getBeanDefinitionNames()) { - // NOTE do not add the PDX Disk Store bean dependency to itself! - if (!beanName.equalsIgnoreCase(getPdxDiskStoreName())) { - BeanDefinition beanDefinition = beanFactory.getBeanDefinition(beanName); - - // NOTE an optimization to only inspect Region bean definitions for persistent Regions (NOTE, the - // persistent Region might also define a Disk Store, or would depend on the DEFAULT Disk Store, - // which may depend on PDX type meta-data). - // NOTE add the dependency to the Disk Store bean definition in case the Disk Store bean is defined - // before the Region that uses it! - // TODO what else depends on a PDX Disk Store besides Regions? - if (isDiskStore(beanDefinition) || isPersistentAsyncEventQueue(beanDefinition) - || isPersistentRegion(beanDefinition)) { + // NOTE for simplicity sake, we add a bean dependency to any bean definition for a bean + // (Async Event Queue, Region, Disk Store or otherwise) that may potentially require + // the PDX Disk Store to exist. + // NOTE this logic could be optimized to include Disk Store, persistent Async Event Queues + // and persistent Regions (either by way of 'persistent' attribute, Data Policy + // or [Client]RegionShortcut) that also does not have an explicit Disk Store reference. addPdxDiskStoreDependency(beanDefinition); } } } } - private boolean isDiskStore(final BeanDefinition beanDefinition) { - return (beanDefinition instanceof AbstractBeanDefinition - && isDiskStore((AbstractBeanDefinition) beanDefinition)); - } - - private boolean isDiskStore(final AbstractBeanDefinition beanDefinition) { - return (beanDefinition.hasBeanClass() - && (DiskStoreFactoryBean.class.isAssignableFrom(beanDefinition.getBeanClass())) - || DiskStore.class.isAssignableFrom(beanDefinition.getBeanClass())); - } - - // TODO remove - private boolean isPersistentAsyncEventQueue(final BeanDefinition beanDefinition) { - return (beanDefinition instanceof AbstractBeanDefinition - && isPersistentAsyncEventQueue((AbstractBeanDefinition) beanDefinition)); - } - - // TODO remove - private boolean isPersistentAsyncEventQueue(final AbstractBeanDefinition beanDefinition) { - return (isAsyncEventQueue(beanDefinition) && isPersistent(beanDefinition)); - } - - // TODO remove - private boolean isAsyncEventQueue(final AbstractBeanDefinition beanDefinition) { - return (beanDefinition.hasBeanClass() - && AsyncEventQueueFactoryBean.class.isAssignableFrom(beanDefinition.getBeanClass())); - } - - // TODO remove - private boolean isPersistentRegion(final BeanDefinition beanDefinition) { - return (beanDefinition instanceof AbstractBeanDefinition - && isPersistentRegion((AbstractBeanDefinition) beanDefinition)); - } - - // TODO remove - private boolean isPersistentRegion(final AbstractBeanDefinition beanDefinition) { - return (isRegion(beanDefinition) && isPersistent(beanDefinition)); - } - - // TODO remove - // NOTE Class.isAssignableFrom is not null-safe, hence the AbstractBeanDefinition.hasBeanClass call! - private boolean isRegion(final AbstractBeanDefinition beanDefinition) { - return (beanDefinition.hasBeanClass() - && RegionLookupFactoryBean.class.isAssignableFrom(beanDefinition.getBeanClass())); - } - - private boolean hasDiskStoreReference(final ConfigurableListableBeanFactory beanFactory, - final BeanDefinition beanDefinition) { - String diskStoreName = getPropertyValue(beanDefinition, "diskStoreName"); - return (StringUtils.hasText(diskStoreName) && beanFactory.containsBeanDefinition(diskStoreName)); - } - - // TODO will property placeholders be a problem or will the PropertyPlaceholderConfigurer BeanFactoryPostProcessor - // execute before this BeanFactoryPostProcessor??? - private boolean isPersistent(final BeanDefinition beanDefinition) { - boolean persistent = getPropertyValue(beanDefinition, DATA_POLICY_PROPERTY, DATA_POLICY_NAME_PROPERTY) - .contains(PERSISTENT_KEYWORD); - - persistent |= Boolean.parseBoolean(getPropertyValue(beanDefinition, PERSISTENT_PROPERTY)); - - persistent |= getPropertyValue(beanDefinition, SHORTCUT_PROPERTY).contains(PERSISTENT_KEYWORD); - - return persistent; - } - /** - * Gets the value of a potentially multi-named property on a BeanDefinition returning the first non-null value. + * Adds the PDX Disk Store bean name as a dependency at the beginning of the list of dependencies declared + * by the Bean. *

- * @param beanDefinition the BeanDefinition of the Bean with the property identified by name(s). - * @param propertyNames a String array containing all the possible names of the property. - * @return the first non-null value of the property, which might have multi-names (aliases). - * @see #getPropertyValue(org.springframework.beans.factory.config.BeanDefinition, String) - */ - private String getPropertyValue(final BeanDefinition beanDefinition, final String... propertyNames) { - String propertyValue = null; - - for (String propertyName : propertyNames) { - propertyValue = getPropertyValue(beanDefinition, propertyName); - if (!isNull(propertyValue)) { - break; - } - } - - return String.valueOf(propertyValue); - } - - /** - * Gets the value of the property identified by name from the Bean's BeanDefinition. - *

- * @param beanDefinition the BeanDefinition containing the configuration meta-data and values of the properties - * for the Bean. - * @param propertyName a String identifying the property by name. - * @return a String value of the named property for the Bean. - * @see #getPropertyValue(org.springframework.beans.factory.config.BeanDefinition, String...) - */ - private String getPropertyValue(final BeanDefinition beanDefinition, final String propertyName) { - PropertyValue propertyValue = beanDefinition.getPropertyValues().getPropertyValue(propertyName); - return String.valueOf(propertyValue != null ? propertyValue.getValue() : null); - } - - /** - * Determines whether the specified String value is null. The String value is null if it is a null references - * or is equal to the "null" String irrespective of case or whitespace. - *

- * @param value the String to evaluate for null value. - * @return a boolean indicating whether the String value is null. - */ - private boolean isNull(final String value) { - return (value == null || "null".equalsIgnoreCase(value.trim())); - } - - /** - * Adds the PDX Disk Store dependency to the beginning of the list of dependencies declared by the Bean. - *

- * @param beanDefinition the BeanDefinition to add the dependency to the PDX Disk Store on. + * @param beanDefinition the BeanDefinition to add the PDX Disk Store dependency to. + * @see #getDependsOn(org.springframework.beans.factory.config.BeanDefinition) * @see org.springframework.beans.factory.config.BeanDefinition#setDependsOn(String[]) */ private void addPdxDiskStoreDependency(final BeanDefinition beanDefinition) { String[] newDependsOn = (String[]) ArrayUtils.insert(getDependsOn(beanDefinition), 0, getPdxDiskStoreName()); - beanDefinition.setDependsOn(newDependsOn); } @@ -243,6 +111,7 @@ public class PdxDiskStoreAwareBeanFactoryPostProcessor implements BeanFactoryPos * @param beanDefinition the BeanDefinition of the Bean containing the dependencies. * @return an array of Bean names that this Bean depends on, or an empty String array if the dependencies * are undefined. + * @see #addPdxDiskStoreDependency(org.springframework.beans.factory.config.BeanDefinition) * @see org.springframework.beans.factory.config.BeanDefinition#getDependsOn() */ private String[] getDependsOn(final BeanDefinition beanDefinition) { diff --git a/src/test/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessorTest.java b/src/test/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessorTest.java index 5d71f944..88384710 100644 --- a/src/test/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessorTest.java +++ b/src/test/java/org/springframework/data/gemfire/config/PdxDiskStoreAwareBeanFactoryPostProcessorTest.java @@ -41,10 +41,8 @@ import org.springframework.beans.factory.support.AbstractBeanDefinition; import org.springframework.beans.factory.support.BeanDefinitionBuilder; import org.springframework.data.gemfire.CacheFactoryBean; -import com.gemstone.gemfire.cache.DataPolicy; import com.gemstone.gemfire.cache.DiskStore; import com.gemstone.gemfire.cache.Region; -import com.gemstone.gemfire.cache.RegionShortcut; import com.gemstone.gemfire.cache.asyncqueue.AsyncEventQueue; import com.gemstone.gemfire.internal.cache.PartitionedRegion; @@ -56,13 +54,13 @@ import com.gemstone.gemfire.internal.cache.PartitionedRegion; * @see org.junit.Test * @see org.mockito.Mockito * @see org.springframework.data.gemfire.config.PdxDiskStoreAwareBeanFactoryPostProcessor + * @see com.gemstone.gemfire.cache.DiskStore + * @see com.gemstone.gemfire.cache.Region + * @see com.gemstone.gemfire.cache.asyncqueue.AsyncEventQueue * @since 1.3.3 */ public class PdxDiskStoreAwareBeanFactoryPostProcessorTest { - protected static final boolean PERSISTENT = true; - protected static final boolean NOT_PERSISTENT = false; - protected static String[] toStringArray(final Collection collection) { return collection.toArray(new String[collection.size()]); } @@ -74,7 +72,7 @@ public class PdxDiskStoreAwareBeanFactoryPostProcessorTest { protected static boolean isBeanType(final BeanDefinition beanDefinition, final Class beanType) { return (beanDefinition instanceof AbstractBeanDefinition && ((AbstractBeanDefinition) beanDefinition).hasBeanClass() - && beanType.isAssignableFrom(((AbstractBeanDefinition) beanDefinition).getBeanClass())); + && beanType.isAssignableFrom(((AbstractBeanDefinition) beanDefinition).getBeanClass())); } protected ConfigurableListableBeanFactory createMockBeanFactory(final Map beanDefinitions) { @@ -154,47 +152,24 @@ public class PdxDiskStoreAwareBeanFactoryPostProcessorTest { return createBeanDefinitionBuilder(CacheFactoryBean.class).getBeanDefinition(); } - protected BeanDefinition defineAsyncEventQueue(Boolean persistent, String... dependencies) { - BeanDefinitionBuilder builder = createBeanDefinitionBuilder(AsyncEventQueue.class, dependencies); - - if (persistent != null) { - builder.addPropertyValue("persistent", persistent.toString()); - } - - return builder.getBeanDefinition(); + protected BeanDefinition defineAsyncEventQueue(String... dependencies) { + return createBeanDefinitionBuilder(AsyncEventQueue.class, dependencies).getBeanDefinition(); } protected BeanDefinition defineDiskStore(String... dependencies) { return createBeanDefinitionBuilder(DiskStore.class, dependencies).getBeanDefinition(); } - protected BeanDefinition defineRegion(Class beanClass, Boolean persistent, DataPolicy dataPolicy, - RegionShortcut shortcut, String... dependencies) { - BeanDefinitionBuilder builder = createBeanDefinitionBuilder(beanClass, dependencies); - - if (persistent != null) { - builder.addPropertyValue("persistent", persistent.toString()); - } - - if (dataPolicy != null) { - builder.addPropertyValue("dataPolicy", dataPolicy.toString()); - } - - if (shortcut != null) { - builder.addPropertyValue("shortcut", shortcut.toString()); - } - - return builder.getBeanDefinition(); + protected BeanDefinition defineRegion(Class regionClass, String... dependencies) { + return createBeanDefinitionBuilder(regionClass, dependencies).getBeanDefinition(); } - protected BeanDefinition definePartitionedRegion(Boolean persistent, DataPolicy dataPolicy, RegionShortcut shortcut, - String... dependencies) { - return defineRegion(PartitionedRegion.class, persistent, dataPolicy, shortcut, dependencies); + protected BeanDefinition definePartitionedRegion(String... dependencies) { + return defineRegion(PartitionedRegion.class, dependencies); } - protected BeanDefinition defineReplicatedRegion(Boolean persistent, DataPolicy dataPolicy, RegionShortcut shortcut, - String... dependencies) { - return defineRegion(Region.class, persistent, dataPolicy, shortcut, dependencies); + protected BeanDefinition defineReplicatedRegion(String... dependencies) { + return defineRegion(Region.class, dependencies); } @Test(expected = IllegalArgumentException.class) @@ -229,22 +204,18 @@ public class PdxDiskStoreAwareBeanFactoryPostProcessorTest { beanDefinitions.put("gemfireCache", defineCache()); beanDefinitions.put("pdxDiskStore", defineDiskStore()); beanDefinitions.put("someOtherBean", defineBean("org.company.app.domain.SomeOtherBean")); - beanDefinitions.put("queue1", defineAsyncEventQueue(null, "someOtherBean")); + beanDefinitions.put("queue1", defineAsyncEventQueue("someOtherBean")); beanDefinitions.put("overflowDiskStore", defineDiskStore()); - beanDefinitions.put("region1", defineReplicatedRegion(NOT_PERSISTENT, null, RegionShortcut.REPLICATE_OVERFLOW, - "overflowDiskStore")); + beanDefinitions.put("region1", defineReplicatedRegion("overflowDiskStore")); beanDefinitions.put("region2DiskStore", defineDiskStore("someBean")); - beanDefinitions.put("region2", defineReplicatedRegion(PERSISTENT, null, null, "region2DiskStore")); - beanDefinitions.put("colocatedRegion", definePartitionedRegion(null, DataPolicy.PARTITION, - RegionShortcut.PARTITION_REDUNDANT_OVERFLOW, "residentRegion", "overflowDiskStore")); + beanDefinitions.put("region2", defineReplicatedRegion("region2DiskStore")); + beanDefinitions.put("colocatedRegion", definePartitionedRegion("residentRegion", "overflowDiskStore")); beanDefinitions.put("residentRegionDiskStore", defineDiskStore("someBean", "yetAnotherBean")); - beanDefinitions.put("residentRegion", definePartitionedRegion(PERSISTENT, null, null, - "residentRegionDiskStore")); + beanDefinitions.put("residentRegion", definePartitionedRegion("residentRegionDiskStore")); beanDefinitions.put("yetAnotherBean", defineBean("org.company.app.domain.YetAnotherBean", "someBean")); - beanDefinitions.put("queue2", defineAsyncEventQueue(PERSISTENT)); - beanDefinitions.put("region3", definePartitionedRegion(PERSISTENT, null, RegionShortcut.PARTITION_PERSISTENT)); - beanDefinitions.put("region4", definePartitionedRegion(null, DataPolicy.PERSISTENT_PARTITION, null, "queue2")); - beanDefinitions.put("region5", defineReplicatedRegion(null, null, RegionShortcut.REPLICATE_PERSISTENT_OVERFLOW)); + beanDefinitions.put("queue2", defineAsyncEventQueue()); + beanDefinitions.put("region3", definePartitionedRegion()); + beanDefinitions.put("region4", definePartitionedRegion("queue2")); final ConfigurableListableBeanFactory mockBeanFactory = createMockBeanFactory(beanDefinitions); @@ -257,19 +228,18 @@ public class PdxDiskStoreAwareBeanFactoryPostProcessorTest { assertTrue(isEmpty(beanDefinitions.get("gemfireCache").getDependsOn())); assertTrue(isEmpty(beanDefinitions.get("pdxDiskStore").getDependsOn())); assertTrue(isEmpty(beanDefinitions.get("someOtherBean").getDependsOn())); - assertDependencies(beanDefinitions.get("queue1"), "someOtherBean"); + assertDependencies(beanDefinitions.get("queue1"), "pdxDiskStore", "someOtherBean"); assertDependencies(beanDefinitions.get("overflowDiskStore"), "pdxDiskStore"); - assertDependencies(beanDefinitions.get("region1"), "overflowDiskStore"); + assertDependencies(beanDefinitions.get("region1"), "pdxDiskStore", "overflowDiskStore"); assertDependencies(beanDefinitions.get("region2DiskStore"), "pdxDiskStore", "someBean"); assertDependencies(beanDefinitions.get("region2"), "pdxDiskStore", "region2DiskStore"); - assertDependencies(beanDefinitions.get("colocatedRegion"), "residentRegion", "overflowDiskStore"); + assertDependencies(beanDefinitions.get("colocatedRegion"), "pdxDiskStore", "residentRegion", "overflowDiskStore"); assertDependencies(beanDefinitions.get("residentRegionDiskStore"), "pdxDiskStore", "someBean", "yetAnotherBean"); assertDependencies(beanDefinitions.get("residentRegion"), "pdxDiskStore", "residentRegionDiskStore"); assertDependencies(beanDefinitions.get("yetAnotherBean"), "someBean"); assertDependencies(beanDefinitions.get("queue2"), "pdxDiskStore"); assertDependencies(beanDefinitions.get("region3"), "pdxDiskStore"); assertDependencies(beanDefinitions.get("region4"), "pdxDiskStore", "queue2"); - assertDependencies(beanDefinitions.get("region5"), "pdxDiskStore"); } }