From 40aa815cd86523859ee4931a4e4e0e4c62c8ecfe Mon Sep 17 00:00:00 2001 From: Andrei Solntsev Date: Tue, 9 May 2017 22:42:10 +0300 Subject: [PATCH 1/3] cleanup code --- .../src/play/plugins/PluginCollection.java | 61 ++++++------------- .../play/plugins/PluginCollectionTest.java | 31 ++++------ 2 files changed, 32 insertions(+), 60 deletions(-) diff --git a/framework/src/play/plugins/PluginCollection.java b/framework/src/play/plugins/PluginCollection.java index cb2a172800..75cdeaed50 100644 --- a/framework/src/play/plugins/PluginCollection.java +++ b/framework/src/play/plugins/PluginCollection.java @@ -7,19 +7,7 @@ import java.lang.reflect.Method; import java.lang.reflect.Type; import java.net.URL; -import java.util.AbstractCollection; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.Enumeration; -import java.util.HashSet; -import java.util.Iterator; -import java.util.List; -import java.util.ListIterator; -import java.util.Map; -import java.util.Set; -import java.util.SortedSet; -import java.util.TreeSet; +import java.util.*; import play.Logger; import play.Play; @@ -39,6 +27,9 @@ import play.test.TestEngine; import play.vfs.VirtualFile; +import static java.util.Collections.emptyList; +import static java.util.Objects.hash; + /** * Class handling all plugins used by Play. * @@ -113,7 +104,7 @@ private LoadingPluginInfo(String name, int index, URL url) { @Override public String toString() { - return "LoadingPluginInfo{" + "name='" + name + '\'' + ", index=" + index + ", url=" + url + '}'; + return String.format("LoadingPluginInfo{name='%s', index=%s, url=%s}", name, index, url); } @Override @@ -136,36 +127,18 @@ public boolean equals(Object o) { return false; LoadingPluginInfo that = (LoadingPluginInfo) o; - - if (index != that.index) - return false; - if (name != null ? !name.equals(that.name) : that.name != null) - return false; - - return true; + return Objects.equals(index, that.index) && Objects.equals(name, that.name); } @Override public int hashCode() { - int result = name != null ? name.hashCode() : 0; - result = 31 * result + index; - return result; + return hash(name, index); } } - /** - * Enable found plugins - */ public void loadPlugins() { Logger.trace("Loading plugins"); - // Play! plugins - Enumeration urls = null; - try { - urls = Play.classloader.getResources(play_plugins_resourceName); - } catch (Exception e) { - Logger.error("Error loading play.plugins", e); - return; - } + List urls = loadPlayPluginDescriptors(); // First we build one big SortedSet of all plugins to load (sorted based // on index) @@ -183,11 +156,9 @@ public void loadPlugins() { // think of a reasonable use case for // loading the same plugin multiple times at the same priority. SortedSet pluginsToLoad = new TreeSet<>(); - while (urls != null && urls.hasMoreElements()) { - URL url = urls.nextElement(); + for (URL url : urls) { Logger.trace("Found one plugins descriptor, %s", url); - try { - BufferedReader reader = new BufferedReader(new InputStreamReader(url.openStream(), "utf-8")); + try (BufferedReader reader = new BufferedReader(new InputStreamReader(url.openStream(), "utf-8"))) { String line; while ((line = reader.readLine()) != null) { if (line.trim().length() == 0) { @@ -200,7 +171,6 @@ public void loadPlugins() { } catch (Exception e) { Logger.error(e, "Error interpreting %s", url); } - } for (LoadingPluginInfo info : pluginsToLoad) { @@ -217,7 +187,7 @@ public void loadPlugins() { Logger.error(ex, "Error loading plugin %s", info.toString()); } } - // Mow we must call onLoad for all plugins - and we must detect if a + // Now we must call onLoad for all plugins - and we must detect if a // plugin // disables another plugin the old way, by removing it from // Play.plugins. @@ -234,6 +204,15 @@ public void loadPlugins() { } + List loadPlayPluginDescriptors() { + try { + return Collections.list(Play.classloader.getResources(play_plugins_resourceName)); + } catch (Exception e) { + Logger.error(e, "Error loading play.plugins"); + return emptyList(); + } + } + /** * Reloads all loaded plugins that is application-supplied. * diff --git a/framework/test-src/play/plugins/PluginCollectionTest.java b/framework/test-src/play/plugins/PluginCollectionTest.java index da277a63a1..fdffb946e4 100644 --- a/framework/test-src/play/plugins/PluginCollectionTest.java +++ b/framework/test-src/play/plugins/PluginCollectionTest.java @@ -1,11 +1,7 @@ package play.plugins; -import java.util.Arrays; -import java.util.Collection; - import org.junit.Before; import org.junit.Test; - import play.*; import play.data.parsing.TempFilePlugin; import play.data.validation.ValidationPlugin; @@ -16,19 +12,16 @@ import play.jobs.JobsPlugin; import play.libs.WS; import play.test.TestEngine; + +import java.util.Arrays; +import java.util.Collection; + import static org.fest.assertions.Assertions.assertThat; -/** - * Created by IntelliJ IDEA. - * User: mortenkjetland - * Date: 3/3/11 - * Time: 12:14 AM - * To change this template use File | Settings | File Templates. - */ public class PluginCollectionTest { @Before - public void init(){ + public void init() { new PlayBuilder().build(); } @@ -55,7 +48,7 @@ public void verifyLoading() { @Test public void verifyLoadingFromFilesWithBlankLines() throws Exception { //create custom PluginCollection that fakes that TestPlugin is application plugin - PluginCollection pc = new PluginCollection(){ + PluginCollection pc = new PluginCollection() { @Override protected boolean isLoadedByApplicationClassloader(PlayPlugin plugin) { //return true only if This is our TestPlugin @@ -77,9 +70,9 @@ protected boolean isLoadedByApplicationClassloader(PlayPlugin plugin) { } @Test - public void verifyReloading() throws Exception{ + public void verifyReloading() throws Exception { //create custom PluginCollection that fakes that TestPlugin is application plugin - PluginCollection pc = new PluginCollection(){ + PluginCollection pc = new PluginCollection() { @Override protected boolean isLoadedByApplicationClassloader(PlayPlugin plugin) { //return true only if This is our TestPlugin @@ -113,7 +106,7 @@ protected boolean isLoadedByApplicationClassloader(PlayPlugin plugin) { @SuppressWarnings({"deprecation"}) @Test - public void verifyUpdatePlayPluginsList(){ + public void verifyUpdatePlayPluginsList() { assertThat(Play.plugins).isEmpty(); PluginCollection pc = new PluginCollection(); @@ -126,7 +119,7 @@ public void verifyUpdatePlayPluginsList(){ @SuppressWarnings({"deprecation"}) @Test - public void verifyThatDisabelingPluginsTheOldWayStillWorks(){ + public void verifyThatDisablingPluginsTheOldWayStillWorks() { PluginCollection pc = new PluginCollection(); @@ -173,8 +166,8 @@ class LegacyPlugin extends PlayPlugin { public void onLoad() { //find TestPlugin in Play.plugins-list and remove it to disable it PlayPlugin pluginToRemove = null; - for( PlayPlugin pp : Play.plugins){ - if( pp.getClass().equals( TestPlugin.class)){ + for (PlayPlugin pp : Play.plugins) { + if ( pp.getClass().equals( TestPlugin.class)) { pluginToRemove = pp; break; } From 02a75fb6950b53361ef5acd6fd07756cab836151 Mon Sep 17 00:00:00 2001 From: Andrei Solntsev Date: Tue, 9 May 2017 23:31:57 +0300 Subject: [PATCH 2/3] convert long comment to a test (aka executable documentation!) --- .../src/play/plugins/PluginCollection.java | 14 +------- .../play/plugins/PluginCollectionTest.java | 33 ++++++++++++++++--- .../plugins/custom-play.plugins.duplicate | 2 ++ 3 files changed, 31 insertions(+), 18 deletions(-) create mode 100644 framework/test-src/play/plugins/custom-play.plugins.duplicate diff --git a/framework/src/play/plugins/PluginCollection.java b/framework/src/play/plugins/PluginCollection.java index 75cdeaed50..bc7e3d1bde 100644 --- a/framework/src/play/plugins/PluginCollection.java +++ b/framework/src/play/plugins/PluginCollection.java @@ -140,21 +140,9 @@ public void loadPlugins() { Logger.trace("Loading plugins"); List urls = loadPlayPluginDescriptors(); - // First we build one big SortedSet of all plugins to load (sorted based - // on index) + // First we build one big SortedSet of all plugins to load (sorted based on index) // This must be done to make sure the enhancing is happening // when loading plugins using other classes that must be enhanced. - // Data structure is a SortedSet instead of a List to avoid including - // the same class+index twice -- - // this happened in the past under a range of circumstances, including: - // 1. Class path on NTFS or other case insensitive file system includes - // play.plugins directory 2x - // (C:/myproject/conf;c:/myproject/conf) - // 2. - // https://play.lighthouseapp.com/projects/57987/tickets/176-app-playplugins-loaded-twice-conf-on-2-classpaths - // I can see loading the same plugin with different indexes, but I can't - // think of a reasonable use case for - // loading the same plugin multiple times at the same priority. SortedSet pluginsToLoad = new TreeSet<>(); for (URL url : urls) { Logger.trace("Found one plugins descriptor, %s", url); diff --git a/framework/test-src/play/plugins/PluginCollectionTest.java b/framework/test-src/play/plugins/PluginCollectionTest.java index fdffb946e4..ab3ff80b61 100644 --- a/framework/test-src/play/plugins/PluginCollectionTest.java +++ b/framework/test-src/play/plugins/PluginCollectionTest.java @@ -13,10 +13,12 @@ import play.libs.WS; import play.test.TestEngine; -import java.util.Arrays; import java.util.Collection; +import static java.util.Arrays.asList; import static org.fest.assertions.Assertions.assertThat; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; public class PluginCollectionTest { @@ -69,6 +71,27 @@ protected boolean isLoadedByApplicationClassloader(PlayPlugin plugin) { } + /** + * Avoid including the same class+index twice. + * + * This happened in the past under a range of circumstances, including: + * 1. Class path on NTFS or other case insensitive file system includes + * play.plugins directory 2x (C:/myproject/conf;c:/myproject/conf) + * 2. https://play.lighthouseapp.com/projects/57987/tickets/176-app-playplugins-loaded-twice-conf-on-2-classpaths + */ + @Test + public void skipsDuplicatePlugins() { + PluginCollection pc = spy(new PluginCollection()); + when(pc.loadPlayPluginDescriptors()).thenReturn(asList( + getClass().getResource("custom-play.plugins"), + getClass().getResource("custom-play.plugins.duplicate")) + ); + pc.loadPlugins(); + assertThat(pc.getAllPlugins()).containsExactly( + pc.getPluginInstance(CorePlugin.class), + pc.getPluginInstance(TestPlugin.class)); + } + @Test public void verifyReloading() throws Exception { //create custom PluginCollection that fakes that TestPlugin is application plugin @@ -181,12 +204,12 @@ class PluginWithTests extends PlayPlugin { @Override public Collection getUnitTests() { - return Arrays.asList(new Class[]{PluginUnit.class}); + return asList(new Class[]{PluginUnit.class}); } @Override public Collection getFunctionalTests() { - return Arrays.asList(new Class[]{PluginFunc.class}); + return asList(new Class[]{PluginFunc.class}); } } @@ -194,12 +217,12 @@ class PluginWithTests2 extends PlayPlugin { @Override public Collection getUnitTests() { - return Arrays.asList(new Class[]{PluginUnit2.class}); + return asList(new Class[]{PluginUnit2.class}); } @Override public Collection getFunctionalTests() { - return Arrays.asList(new Class[]{PluginFunc2.class}); + return asList(new Class[]{PluginFunc2.class}); } } diff --git a/framework/test-src/play/plugins/custom-play.plugins.duplicate b/framework/test-src/play/plugins/custom-play.plugins.duplicate new file mode 100644 index 0000000000..39ab6591bf --- /dev/null +++ b/framework/test-src/play/plugins/custom-play.plugins.duplicate @@ -0,0 +1,2 @@ +0:play.CorePlugin +1:play.plugins.TestPlugin From cf0d3785ecdbf78a001bf76ae7451b3aa5f79a41 Mon Sep 17 00:00:00 2001 From: Andrei Solntsev Date: Wed, 10 May 2017 09:08:04 +0300 Subject: [PATCH 3/3] #1137 can load play plugins from a single descriptor --- documentation/manual/configuration.textile | 11 +++++++++++ framework/src/play/plugins/PluginCollection.java | 9 +++++++-- .../test-src/play/plugins/PluginCollectionTest.java | 10 ++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/documentation/manual/configuration.textile b/documentation/manual/configuration.textile index 1968376c33..ede17e4710 100644 --- a/documentation/manual/configuration.textile +++ b/documentation/manual/configuration.textile @@ -984,6 +984,17 @@ Default: none. h2(#play). Play run-time +h3(#play.plugins.descriptor). play.plugins.descriptor + +File to load play plugins from. +* If given, play with load plugins ONLY from this file. @since play 1.5 +* If not given, play will find ALL files "play.plugins" from classpath and load plugins from them (this is the default behaviour). + +bc. play.plugins.descriptor=conf/my-play-app.plugins + +Default: none. + + h3(#play.bytecodeCache). play.bytecodeCache Used to disable the bytecode cache in @dev@ mode; has no effect in @prod@ mode. diff --git a/framework/src/play/plugins/PluginCollection.java b/framework/src/play/plugins/PluginCollection.java index bc7e3d1bde..9878d69ccf 100644 --- a/framework/src/play/plugins/PluginCollection.java +++ b/framework/src/play/plugins/PluginCollection.java @@ -1,6 +1,7 @@ package play.plugins; import java.io.BufferedReader; +import java.io.File; import java.io.InputStreamReader; import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; @@ -189,13 +190,17 @@ public void loadPlugins() { // Must update Play.plugins-list one last time updatePlayPluginsList(); - } List loadPlayPluginDescriptors() { try { + String playPluginsDescriptor = Play.configuration.getProperty("play.plugins.descriptor"); + if (playPluginsDescriptor != null) { + return Collections.singletonList(new File(playPluginsDescriptor).toURI().toURL()); + } return Collections.list(Play.classloader.getResources(play_plugins_resourceName)); - } catch (Exception e) { + } + catch (Exception e) { Logger.error(e, "Error loading play.plugins"); return emptyList(); } diff --git a/framework/test-src/play/plugins/PluginCollectionTest.java b/framework/test-src/play/plugins/PluginCollectionTest.java index ab3ff80b61..25fa853f30 100644 --- a/framework/test-src/play/plugins/PluginCollectionTest.java +++ b/framework/test-src/play/plugins/PluginCollectionTest.java @@ -13,6 +13,7 @@ import play.libs.WS; import play.test.TestEngine; +import java.io.File; import java.util.Collection; import static java.util.Arrays.asList; @@ -92,6 +93,15 @@ public void skipsDuplicatePlugins() { pc.getPluginInstance(TestPlugin.class)); } + @Test + public void canLoadPlayPluginsFromASingleDescriptor() throws Exception { + Play.configuration.setProperty("play.plugins.descriptor", "test-src/play/plugins/custom-play.plugins"); + PluginCollection pc = new PluginCollection(); + assertThat(pc.loadPlayPluginDescriptors()).containsExactly( + new File("test-src/play/plugins/custom-play.plugins").toURI().toURL() + ); + } + @Test public void verifyReloading() throws Exception { //create custom PluginCollection that fakes that TestPlugin is application plugin