#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

@@ -37,6 +37,13 @@
 			<scope>test</scope>
 		</dependency>
 		
+		<dependency>
+			<groupId>cglib</groupId>
+			<artifactId>cglib</artifactId>
+			<version>2.2.2</version>
+			<scope>test</scope>
+		</dependency>
+
 	</dependencies>
 
 </project>

core/src/main/java/org/springframework/plugin/core/MutablePluginRegistry.java

@@ -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<T extends Plugin<S>, S> extends PluginRegistry<T, S> {
-
-	/**
-	 * Adds a given plugin to the registry.
-	 * 
-	 * @param plugin must not be {@literal null}.
-	 */
-	MutablePluginRegistry<T, S> addPlugin(T plugin);
-
-	/**
-	 * Removes a given plugin from the registry.
-	 * 
-	 * @param plugin must not be {@literal null}.
-	 */
-	boolean removePlugin(T plugin);
-}

core/src/main/java/org/springframework/plugin/core/OrderAwarePluginRegistry.java

@@ -58,7 +58,7 @@ public class OrderAwarePluginRegistry<T extends Plugin<S>, S> extends SimplePlug
 	protected OrderAwarePluginRegistry(List<? extends T> plugins, Comparator<? super T> comparator) {
 
 		super(plugins);
-		setComparator(comparator);
+		this.comparator = (Comparator<? super T>) (comparator == null ? DEFAULT_COMPARATOR : comparator);
 	}
 
 	/**
@@ -122,34 +122,16 @@ public class OrderAwarePluginRegistry<T extends Plugin<S>, S> extends SimplePlug
 		return new OrderAwarePluginRegistry<T, S>(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<? super T> 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<T, S> addPlugin(T plugin) {
+	protected List<T> initialize(List<T> plugins) {
 
-		super.addPlugin(plugin);
-		Collections.sort(plugins, comparator);
-		return this;
+		List<T> result = super.initialize(plugins);
+		Collections.sort(result, comparator);
+		return result;
 	}
 
 	/**
@@ -160,7 +142,7 @@ public class OrderAwarePluginRegistry<T extends Plugin<S>, S> extends SimplePlug
 	@SuppressWarnings({ "unchecked", "rawtypes" })
 	public OrderAwarePluginRegistry<T, S> reverse() {
 
-		ArrayList<T> copy = new ArrayList<T>(plugins);
+		ArrayList<T> copy = new ArrayList<T>(getPlugins());
 		return create(copy, new InvertibleComparator(comparator, false));
 	}
 }

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<T extends Plugin<S>, S> implements PluginRegistry<T, S>, Iterable<T> {
+
+	private List<T> 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<? extends T> plugins) {
+
+		Assert.notNull(plugins);
+
+		this.plugins = plugins == null ? new ArrayList<T>() : (List<T>) 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<T> 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<T> initialize(List<T> plugins) {
+
+		Assert.notNull(plugins);
+		List<T> result = new ArrayList<T>();
+
+		for (T plugin : this.plugins) {
+			if (plugin != null) {
+				result.add(plugin);
+			}
+		}
+
+		return result;
+	}
+
+	/* 
+	 * (non-Javadoc)
+	 * @see java.lang.Iterable#iterator()
+	 */
+	@Override
+	public Iterator<T> iterator() {
+		return getPlugins().iterator();
+	}
+}

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 <S> the delimiter type
  * @author Oliver Gierke
  */
-public class SimplePluginRegistry<T extends Plugin<S>, S> implements MutablePluginRegistry<T, S> {
-
-	protected final List<T> plugins;
+public class SimplePluginRegistry<T extends Plugin<S>, S> extends PluginRegistrySupport<T, S> {
 
 	/**
 	 * 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<? extends T> plugins) {
-		this.plugins = plugins == null ? new ArrayList<T>() : (List<T>) plugins;
+		super(plugins);
 	}
 
 	/**
@@ -50,7 +46,7 @@ public class SimplePluginRegistry<T extends Plugin<S>, S> implements MutablePlug
 	 * @return
 	 */
 	public static <S, T extends Plugin<S>> SimplePluginRegistry<T, S> create() {
-		return new SimplePluginRegistry<T, S>(null);
+		return create(Collections.<T> emptyList());
 	}
 
 	/**
@@ -64,26 +60,12 @@ public class SimplePluginRegistry<T extends Plugin<S>, S> implements MutablePlug
 		return new SimplePluginRegistry<T, S>(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<T, S> 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<T> getPlugins() {
+		return Collections.unmodifiableList(super.getPlugins());
 	}
 
 	/*
@@ -109,7 +91,7 @@ public class SimplePluginRegistry<T extends Plugin<S>, S> implements MutablePlug
 
 		List<T> result = new ArrayList<T>();
 
-		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<T extends Plugin<S>, 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<T> getPlugins() {
-		return Collections.unmodifiableList(plugins);
+		return super.getPlugins().size();
 	}
 
 	/*
@@ -195,22 +166,14 @@ public class SimplePluginRegistry<T extends Plugin<S>, 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<T> iterator() {
-		return plugins.iterator();
+	public boolean hasPluginFor(S delimiter) {
+		return null != getPluginFor(delimiter);
 	}
 }

core/src/main/java/org/springframework/plugin/core/support/AbstractTypeAwareSupport.java

@@ -102,7 +102,7 @@ public abstract class AbstractTypeAwareSupport<T> implements ApplicationContextA
 	 * 
 	 * @author Oliver Gierke
 	 */
-	private static class BeansOfTypeTargetSource implements TargetSource {
+	static class BeansOfTypeTargetSource implements TargetSource {
 
 		private final ListableBeanFactory context;
 		private final Class<?> type;

core/src/test/java/org/springframework/plugin/core/AbstractMutablePluginRegistryUnitTest.java

@@ -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<SamplePlugin, String> 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<SamplePlugin, String> getRegistry();
-}

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<SamplePlugin, String> 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<TestPlugin, String> registry = OrderAwarePluginRegistry.create(Arrays.asList(firstPlugin));
-		registry.addPlugin(secondPlugin);
-
-		assertOrder(registry, secondPlugin, firstPlugin);
-	}
-
 	private void assertOrder(PluginRegistry<TestPlugin, String> registry, TestPlugin... plugins) {
 
 		List<TestPlugin> result = registry.getPluginsFor(null);

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<SamplePlugin, String> 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<SamplePlugin> plugins = new ArrayList<SamplePlugin>();
+		plugins.add(null);
+
+		registry = SimplePluginRegistry.create(plugins);
 
 		assertThat(registry.countPlugins(), is(0));
 	}

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<TestPlugin> beans = (List<TestPlugin>) factory.getProxy();
+		OrderAwarePluginRegistry<TestPlugin, String> registry = OrderAwarePluginRegistry.create(beans);
+
+		List<TestPlugin> plugins = registry.getPlugins();
+	}
+
+	private static interface TestPlugin extends Plugin<String> {
+
+	}
+
+	@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;
+		}
+	}
+}

pom.xml

@@ -44,7 +44,7 @@
     </modules>
     
     <properties>
-        <spring.version>3.0.7.RELEASE</spring.version>
+        <spring.version>3.1.2.RELEASE</spring.version>
     </properties>
 
     <developers>