From c0e01afed2cf5e04c5c0329ebdf65963670c99ad Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Fri, 26 Oct 2012 12:49:31 +0200 Subject: [PATCH] #1 - Restructuring of internals to fix ordering not being applied. Introduced PluginRegistrySupport base class to make sure we create a copy of the plugin list on first access to it as otherwise the sorting calls get lost in the proxy created by the AbstractTypeAwareSupport. Removed MutablePluginRegistry interfaces and implementation along the way as it causes more bad than good currently and there doesn't seem to be a real need for it. --- core/pom.xml | 7 ++ .../plugin/core/MutablePluginRegistry.java | 38 -------- .../plugin/core/OrderAwarePluginRegistry.java | 34 ++----- .../plugin/core/PluginRegistrySupport.java | 95 +++++++++++++++++++ .../plugin/core/SimplePluginRegistry.java | 63 +++--------- .../support/AbstractTypeAwareSupport.java | 2 +- ...AbstractMutablePluginRegistryUnitTest.java | 49 ---------- .../OrderAwarePluginRegistryUnitTest.java | 17 ---- .../core/SimplePluginRegistryUnitTest.java | 25 ++--- ...derAwarePluginRegistryIntegrationTest.java | 68 +++++++++++++ pom.xml | 2 +- 11 files changed, 201 insertions(+), 199 deletions(-) delete mode 100644 core/src/main/java/org/springframework/plugin/core/MutablePluginRegistry.java create mode 100644 core/src/main/java/org/springframework/plugin/core/PluginRegistrySupport.java delete mode 100644 core/src/test/java/org/springframework/plugin/core/AbstractMutablePluginRegistryUnitTest.java create mode 100644 core/src/test/java/org/springframework/plugin/core/support/OrderAwarePluginRegistryIntegrationTest.java diff --git a/core/pom.xml b/core/pom.xml index d5b9697..1cafd94 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -37,6 +37,13 @@ test + + cglib + cglib + 2.2.2 + test + + diff --git a/core/src/main/java/org/springframework/plugin/core/MutablePluginRegistry.java b/core/src/main/java/org/springframework/plugin/core/MutablePluginRegistry.java deleted file mode 100644 index d4798fe..0000000 --- a/core/src/main/java/org/springframework/plugin/core/MutablePluginRegistry.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright 2008-2012 the original author or authors. - * - * Licensed 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.springframework.plugin.core; - -/** - * Extension of {@link PluginRegistry} with additional methods to modify the registry. - * - * @author Oliver Gierke - */ -public interface MutablePluginRegistry, S> extends PluginRegistry { - - /** - * Adds a given plugin to the registry. - * - * @param plugin must not be {@literal null}. - */ - MutablePluginRegistry addPlugin(T plugin); - - /** - * Removes a given plugin from the registry. - * - * @param plugin must not be {@literal null}. - */ - boolean removePlugin(T plugin); -} diff --git a/core/src/main/java/org/springframework/plugin/core/OrderAwarePluginRegistry.java b/core/src/main/java/org/springframework/plugin/core/OrderAwarePluginRegistry.java index addcca5..718c6db 100644 --- a/core/src/main/java/org/springframework/plugin/core/OrderAwarePluginRegistry.java +++ b/core/src/main/java/org/springframework/plugin/core/OrderAwarePluginRegistry.java @@ -58,7 +58,7 @@ public class OrderAwarePluginRegistry, S> extends SimplePlug protected OrderAwarePluginRegistry(List plugins, Comparator comparator) { super(plugins); - setComparator(comparator); + this.comparator = (Comparator) (comparator == null ? DEFAULT_COMPARATOR : comparator); } /** @@ -122,34 +122,16 @@ public class OrderAwarePluginRegistry, S> extends SimplePlug return new OrderAwarePluginRegistry(plugins, comparator); } - /** - * Sets the comparator to use. Resorts the contained {@link Plugin}s if any available. - * - * @param comparator the comparator to set - */ - private void setComparator(Comparator comparator) { - - this.comparator = DEFAULT_COMPARATOR; - - if (comparator != null) { - this.comparator = comparator; - } - - if (plugins != null) { - Collections.sort(plugins, comparator); - } - } - - /* + /* * (non-Javadoc) - * @see org.springframework.plugin.core.SimplePluginRegistry#addPlugin(org.springframework.plugin.core.Plugin) + * @see org.springframework.plugin.core.PluginRegistrySupport#initialize(java.util.List) */ @Override - public OrderAwarePluginRegistry addPlugin(T plugin) { + protected List initialize(List plugins) { - super.addPlugin(plugin); - Collections.sort(plugins, comparator); - return this; + List result = super.initialize(plugins); + Collections.sort(result, comparator); + return result; } /** @@ -160,7 +142,7 @@ public class OrderAwarePluginRegistry, S> extends SimplePlug @SuppressWarnings({ "unchecked", "rawtypes" }) public OrderAwarePluginRegistry reverse() { - ArrayList copy = new ArrayList(plugins); + ArrayList copy = new ArrayList(getPlugins()); return create(copy, new InvertibleComparator(comparator, false)); } } diff --git a/core/src/main/java/org/springframework/plugin/core/PluginRegistrySupport.java b/core/src/main/java/org/springframework/plugin/core/PluginRegistrySupport.java new file mode 100644 index 0000000..0643348 --- /dev/null +++ b/core/src/main/java/org/springframework/plugin/core/PluginRegistrySupport.java @@ -0,0 +1,95 @@ +/* + * Copyright 2012 the original author or authors. + * + * Licensed 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.springframework.plugin.core; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; + +import org.springframework.util.Assert; + +/** + * Base class for {@link PluginRegistry} implementations. Implements an initialization mechanism triggered on forst + * invocation of {@link #getPlugins()}. + * + * @author Oliver Gierke + */ +public abstract class PluginRegistrySupport, S> implements PluginRegistry, Iterable { + + private List plugins; + private boolean initialized; + + /** + * Creates a new {@link PluginRegistrySupport} instance using the given plugins. + * + * @param plugins must not be {@literal null}. + */ + @SuppressWarnings("unchecked") + public PluginRegistrySupport(List plugins) { + + Assert.notNull(plugins); + + this.plugins = plugins == null ? new ArrayList() : (List) plugins; + this.initialized = false; + } + + /** + * Returns all registered plugins. Only use this method if you really need to access all plugins. For distinguished + * access to certain plugins favour accessor methods like {link #getPluginFor} over this one. This method should only + * be used for testing purposes to check registry configuration. + * + * @return all plugins of the registry + */ + public List getPlugins() { + + if (!initialized) { + this.plugins = initialize(this.plugins); + this.initialized = true; + } + + return plugins; + } + + /** + * Callback to initialize the plugin {@link List}. Will create a defensive copy of the {@link List} to potentially + * unwrap a {@link List} proxy. Will filter {@literal null} values from the source list as well. + * + * @param plugins must not be {@literal null}. + * @return + */ + protected synchronized List initialize(List plugins) { + + Assert.notNull(plugins); + List result = new ArrayList(); + + for (T plugin : this.plugins) { + if (plugin != null) { + result.add(plugin); + } + } + + return result; + } + + /* + * (non-Javadoc) + * @see java.lang.Iterable#iterator() + */ + @Override + public Iterator iterator() { + return getPlugins().iterator(); + } +} diff --git a/core/src/main/java/org/springframework/plugin/core/SimplePluginRegistry.java b/core/src/main/java/org/springframework/plugin/core/SimplePluginRegistry.java index 2007176..47771f0 100644 --- a/core/src/main/java/org/springframework/plugin/core/SimplePluginRegistry.java +++ b/core/src/main/java/org/springframework/plugin/core/SimplePluginRegistry.java @@ -17,7 +17,6 @@ package org.springframework.plugin.core; import java.util.ArrayList; import java.util.Collections; -import java.util.Iterator; import java.util.List; /** @@ -28,18 +27,15 @@ import java.util.List; * @param the delimiter type * @author Oliver Gierke */ -public class SimplePluginRegistry, S> implements MutablePluginRegistry { - - protected final List plugins; +public class SimplePluginRegistry, S> extends PluginRegistrySupport { /** * Creates a new {@code SimplePluginRegistry}. Will create an empty registry if {@literal null} is provided. * * @param plugins must not be {@literal null}. */ - @SuppressWarnings("unchecked") protected SimplePluginRegistry(List plugins) { - this.plugins = plugins == null ? new ArrayList() : (List) plugins; + super(plugins); } /** @@ -50,7 +46,7 @@ public class SimplePluginRegistry, S> implements MutablePlug * @return */ public static > SimplePluginRegistry create() { - return new SimplePluginRegistry(null); + return create(Collections. emptyList()); } /** @@ -64,26 +60,12 @@ public class SimplePluginRegistry, S> implements MutablePlug return new SimplePluginRegistry(plugins); } - /* - * (non-Javadoc) - * @see org.springframework.plugin.core.MutablePluginRegistry#addPlugin(org.springframework.plugin.core.Plugin) + /* (non-Javadoc) + * @see org.springframework.plugin.core.PluginRegistrySupport#getPlugins() */ - public SimplePluginRegistry addPlugin(T plugin) { - - if (plugin != null) { - this.plugins.add(plugin); - } - - return this; - } - - /* - * (non-Javadoc) - * @see org.springframework.plugin.core.MutablePluginRegistry#removePlugin(org.springframework.plugin.core.Plugin) - */ - public boolean removePlugin(T plugin) { - - return this.plugins.remove(plugin); + @Override + public List getPlugins() { + return Collections.unmodifiableList(super.getPlugins()); } /* @@ -109,7 +91,7 @@ public class SimplePluginRegistry, S> implements MutablePlug List result = new ArrayList(); - for (T plugin : plugins) { + for (T plugin : super.getPlugins()) { if (plugin != null && plugin.supports(delimiter)) { result.add(plugin); } @@ -176,18 +158,7 @@ public class SimplePluginRegistry, S> implements MutablePlug */ public int countPlugins() { - return plugins.size(); - } - - /** - * Returns all registered plugins. Only use this method if you really need to access all plugins. For distinguished - * access to certain plugins favour accessor methods like {link #getPluginFor} over this one. This method should only - * be used for testing purposes to check registry configuration. - * - * @return all plugins of the registry - */ - public List getPlugins() { - return Collections.unmodifiableList(plugins); + return super.getPlugins().size(); } /* @@ -195,22 +166,14 @@ public class SimplePluginRegistry, S> implements MutablePlug * @see org.springframework.plugin.core.PluginRegistry#contains(org.springframework.plugin.core.Plugin) */ public boolean contains(T plugin) { - return this.plugins.contains(plugin); + return super.getPlugins().contains(plugin); } /* * (non-Javadoc) * @see org.springframework.plugin.core.PluginRegistry#hasPluginFor(java.lang.Object) */ - public boolean hasPluginFor(S delimter) { - return null != getPluginFor(delimter); - } - - /* - * (non-Javadoc) - * @see java.lang.Iterable#iterator() - */ - public Iterator iterator() { - return plugins.iterator(); + public boolean hasPluginFor(S delimiter) { + return null != getPluginFor(delimiter); } } diff --git a/core/src/main/java/org/springframework/plugin/core/support/AbstractTypeAwareSupport.java b/core/src/main/java/org/springframework/plugin/core/support/AbstractTypeAwareSupport.java index 48cdfb1..f483966 100644 --- a/core/src/main/java/org/springframework/plugin/core/support/AbstractTypeAwareSupport.java +++ b/core/src/main/java/org/springframework/plugin/core/support/AbstractTypeAwareSupport.java @@ -102,7 +102,7 @@ public abstract class AbstractTypeAwareSupport implements ApplicationContextA * * @author Oliver Gierke */ - private static class BeansOfTypeTargetSource implements TargetSource { + static class BeansOfTypeTargetSource implements TargetSource { private final ListableBeanFactory context; private final Class type; diff --git a/core/src/test/java/org/springframework/plugin/core/AbstractMutablePluginRegistryUnitTest.java b/core/src/test/java/org/springframework/plugin/core/AbstractMutablePluginRegistryUnitTest.java deleted file mode 100644 index 7462603..0000000 --- a/core/src/test/java/org/springframework/plugin/core/AbstractMutablePluginRegistryUnitTest.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * Copyright 2008-2012 the original author or authors. - * - * Licensed 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.springframework.plugin.core; - -import static org.hamcrest.CoreMatchers.*; -import static org.junit.Assert.*; - -import org.junit.Test; -import org.springframework.plugin.core.MutablePluginRegistry; - -/** - * Unit test for implementations of {@link MutablePluginRegistry}. - * - * @author Oliver Gierke - */ -public abstract class AbstractMutablePluginRegistryUnitTest { - - private SamplePlugin plugin = new SamplePluginImplementation(); - - @Test - public void allowsAddingPluginsAfterCreation() throws Exception { - - MutablePluginRegistry registry = getRegistry(); - registry.addPlugin(plugin); - - assertTrue(registry.contains(plugin)); - assertThat(registry.getPlugins().size(), is(1)); - } - - /** - * Return the {@link MutablePluginRegistry} to test. - * - * @return - */ - protected abstract MutablePluginRegistry getRegistry(); -} diff --git a/core/src/test/java/org/springframework/plugin/core/OrderAwarePluginRegistryUnitTest.java b/core/src/test/java/org/springframework/plugin/core/OrderAwarePluginRegistryUnitTest.java index 66a3126..6135006 100644 --- a/core/src/test/java/org/springframework/plugin/core/OrderAwarePluginRegistryUnitTest.java +++ b/core/src/test/java/org/springframework/plugin/core/OrderAwarePluginRegistryUnitTest.java @@ -23,7 +23,6 @@ import java.util.Arrays; import java.util.List; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.springframework.aop.framework.ProxyFactory; import org.springframework.core.Ordered; @@ -49,12 +48,6 @@ public class OrderAwarePluginRegistryUnitTest extends SimplePluginRegistryUnitTe secondPlugin = new SecondImplementation(); } - @Override - protected OrderAwarePluginRegistry getRegistry() { - - return OrderAwarePluginRegistry.create(); - } - @Test public void honorsOrderOnAddPlugins() throws Exception { @@ -63,16 +56,6 @@ public class OrderAwarePluginRegistryUnitTest extends SimplePluginRegistryUnitTe assertOrder(registry, secondPlugin, firstPlugin); } - @Test - @Ignore - public void assertsOrderOnAddingPlugins() throws Exception { - - MutablePluginRegistry registry = OrderAwarePluginRegistry.create(Arrays.asList(firstPlugin)); - registry.addPlugin(secondPlugin); - - assertOrder(registry, secondPlugin, firstPlugin); - } - private void assertOrder(PluginRegistry registry, TestPlugin... plugins) { List result = registry.getPluginsFor(null); diff --git a/core/src/test/java/org/springframework/plugin/core/SimplePluginRegistryUnitTest.java b/core/src/test/java/org/springframework/plugin/core/SimplePluginRegistryUnitTest.java index 2b4a102..fce09d9 100644 --- a/core/src/test/java/org/springframework/plugin/core/SimplePluginRegistryUnitTest.java +++ b/core/src/test/java/org/springframework/plugin/core/SimplePluginRegistryUnitTest.java @@ -19,21 +19,19 @@ package org.springframework.plugin.core; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import org.junit.Before; import org.junit.Test; -import org.springframework.plugin.core.MutablePluginRegistry; -import org.springframework.plugin.core.Plugin; -import org.springframework.plugin.core.SimplePluginRegistry; /** * Unit test for {@link SimplePluginRegistry}. * * @author Oliver Gierke */ -public class SimplePluginRegistryUnitTest extends AbstractMutablePluginRegistryUnitTest { +public class SimplePluginRegistryUnitTest { SamplePlugin plugin; @@ -49,16 +47,6 @@ public class SimplePluginRegistryUnitTest extends AbstractMutablePluginRegistryU registry = SimplePluginRegistry.create(); } - /* - * (non-Javadoc) - * @see org.springframework.plugin.core.AbstractMutablePluginRegistryUnitTest#getRegistry() - */ - @Override - protected MutablePluginRegistry getRegistry() { - - return SimplePluginRegistry.create(); - } - /** * Asserts that the registry contains the plugin it was initialized with. * @@ -67,7 +55,7 @@ public class SimplePluginRegistryUnitTest extends AbstractMutablePluginRegistryU @Test public void assertRegistryInitialized() throws Exception { - registry.addPlugin(plugin); + registry = SimplePluginRegistry.create(Arrays.asList(plugin)); assertThat(registry.countPlugins(), is(1)); assertTrue(registry.contains(plugin)); @@ -79,7 +67,7 @@ public class SimplePluginRegistryUnitTest extends AbstractMutablePluginRegistryU @Test public void assertFindsEmailNotificationProvider() { - registry.addPlugin(plugin); + registry = SimplePluginRegistry.create(Arrays.asList(plugin)); String delimiter = "FOO"; @@ -135,7 +123,10 @@ public class SimplePluginRegistryUnitTest extends AbstractMutablePluginRegistryU @Test public void handlesAddingNullPluginsCorrecty() throws Exception { - registry.addPlugin(null); + List plugins = new ArrayList(); + plugins.add(null); + + registry = SimplePluginRegistry.create(plugins); assertThat(registry.countPlugins(), is(0)); } diff --git a/core/src/test/java/org/springframework/plugin/core/support/OrderAwarePluginRegistryIntegrationTest.java b/core/src/test/java/org/springframework/plugin/core/support/OrderAwarePluginRegistryIntegrationTest.java new file mode 100644 index 0000000..2c36ba8 --- /dev/null +++ b/core/src/test/java/org/springframework/plugin/core/support/OrderAwarePluginRegistryIntegrationTest.java @@ -0,0 +1,68 @@ +package org.springframework.plugin.core.support; + +import java.util.List; + +import org.springframework.aop.framework.ProxyFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.ApplicationContext; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; +import org.springframework.plugin.core.OrderAwarePluginRegistry; +import org.springframework.plugin.core.Plugin; +import org.springframework.plugin.core.support.AbstractTypeAwareSupport.BeansOfTypeTargetSource; + +//@RunWith(SpringJUnit4ClassRunner.class) +public class OrderAwarePluginRegistryIntegrationTest { + + @Autowired + ApplicationContext context; + + // @Test + public void considersJdkProxiedOrderedImplementation() { + BeansOfTypeTargetSource targetSource = new AbstractTypeAwareSupport.BeansOfTypeTargetSource(context, + TestPlugin.class, false); + + ProxyFactory factory = new ProxyFactory(List.class, targetSource); + List beans = (List) factory.getProxy(); + OrderAwarePluginRegistry registry = OrderAwarePluginRegistry.create(beans); + + List plugins = registry.getPlugins(); + } + + private static interface TestPlugin extends Plugin { + + } + + @Order(5) + private static class FirstImplementation implements TestPlugin { + + @Override + public boolean supports(String delimiter) { + + return true; + } + } + + @Order(1) + private static class SecondImplementation implements TestPlugin { + + @Override + public boolean supports(String delimiter) { + + return true; + } + } + + private static class ThirdImplementation implements TestPlugin, Ordered { + + @Override + public int getOrder() { + return 3; + } + + @Override + public boolean supports(String delimiter) { + return true; + } + } +} diff --git a/pom.xml b/pom.xml index 61240f1..37eefb8 100644 --- a/pom.xml +++ b/pom.xml @@ -44,7 +44,7 @@ - 3.0.7.RELEASE + 3.1.2.RELEASE