Throw errors if PluginContainer.withId() or `PluginContainer.withTy…

…pe()` are used with a plain rule source +review REVIEW-5214
gradle · Oct 9, 2014 · 3749ec1 · 3749ec1
1 parent 92332d8
commit 3749ec1
Show file tree

Hide file tree

Showing 8 changed files with 48 additions and 8 deletions.
diff --git a/design-docs/unified-configuration-and-task-model.md b/design-docs/unified-configuration-and-task-model.md
@@ -410,7 +410,7 @@ A new API for querying applied plugins that supports both `Plugin` implementing
 - ~~`Plugin` impl can include nested rule source class~~
 - ~~A useful error message is presented to the user if they try to apply a rule source plugin as a regular plugin, i. e. `apply plugin: RuleSourcePlugin` or `apply { plugin RuleSourcePlugin }`~~
 - Can use `PluginRegistry` and ids to check if both `Plugin` implementing classes and rule source classes are applied to a project
-- A useful error message is presented when using `PluginContainer.withId()` or `PluginContainer.withType()` to check if a rule source plugin is applied   
+- ~~A useful error message is presented when using `PluginContainer.withId()` or `PluginContainer.withType()` to check if a rule source plugin is applied~~   
 
 
 ## Story: Model DSL rule uses a typed model element as input via name

diff --git a/subprojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultAppliedPlugins.java b/subprojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultAppliedPlugins.java
@@ -32,7 +32,7 @@ public DefaultAppliedPlugins(PluginAware target, PluginRegistry pluginRegistry)
 
     public void extractModelRulesAndAdd(Class<?> pluginClass) {
         ModelRuleSourceDetector detector = new ModelRuleSourceDetector();
-        if (detector.getDeclaredSources(pluginClass).size() > 0) {
+        if (!detector.getDeclaredSources(pluginClass).isEmpty()) {
             throw new UnsupportedOperationException(String.format("Cannot apply model rules of plugin '%s' as the target '%s' is not model rule aware", pluginClass.getName(), target));
         }
     }

diff --git a/...ore/src/main/groovy/org/gradle/api/internal/plugins/DefaultObjectConfigurationAction.java b/...ore/src/main/groovy/org/gradle/api/internal/plugins/DefaultObjectConfigurationAction.java
@@ -105,7 +105,7 @@ private void applyScript(Object script) {
     }
 
     private void applyPlugin(Class<? extends Plugin> pluginClass) {
-        if (!Plugin.class.isAssignableFrom(pluginClass) && new ModelRuleSourceDetector().getDeclaredSources(pluginClass).size() > 0) {
+        if (!Plugin.class.isAssignableFrom(pluginClass) && !new ModelRuleSourceDetector().getDeclaredSources(pluginClass).isEmpty()) {
             throw new IllegalArgumentException(String.format("'%s' is a rule source only type, use 'type' key instead of 'plugin' key to apply it via PluginAware.apply()", pluginClass.getName()));
         }
         for (Object target : targets) {

diff --git a/...rojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultPluginCollection.java b/...rojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultPluginCollection.java
@@ -23,6 +23,7 @@
 import org.gradle.api.plugins.PluginCollection;
 import org.gradle.api.specs.Spec;
 import org.gradle.api.specs.Specs;
+import org.gradle.model.internal.inspect.ModelRuleSourceDetector;
 
 public class DefaultPluginCollection<T extends Plugin> extends DefaultDomainObjectSet<T> implements PluginCollection<T> {
 
@@ -39,6 +40,10 @@ protected <S extends T> DefaultPluginCollection<S> filtered(CollectionFilter<S>
     }
 
     public <S extends T> PluginCollection<S> withType(Class<S> type) {
+        if (!Plugin.class.isAssignableFrom(type) && !new ModelRuleSourceDetector().getDeclaredSources(type).isEmpty()) {
+            String message = String.format("'%s' is a rule source and not a plugin. Use AppliedPlugins.withPlugin() to perform an action if a rule source is applied.", type.getName());
+            throw new IllegalArgumentException(message);
+        }
         return filtered(createFilter(type));
     }
 

diff --git a/subprojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultPluginContainer.java b/subprojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultPluginContainer.java
@@ -24,6 +24,7 @@
 import org.gradle.api.plugins.UnknownPluginException;
 import org.gradle.api.specs.Spec;
 import org.gradle.internal.UncheckedException;
+import org.gradle.model.internal.inspect.ModelRuleSourceDetector;
 
 import java.util.Collections;
 import java.util.List;
@@ -99,7 +100,7 @@ public DefaultPluginContainer(PluginRegistry pluginRegistry, T pluginAware, List
     }
 
     public Plugin apply(String id) {
-        return addPluginInternal(getTypeForId(id));
+        return addPluginInternal(getPluginTypeForId(id));
     }
 
     public <P extends Plugin> P apply(Class<P> type) {
@@ -116,7 +117,7 @@ public boolean hasPlugin(Class<? extends Plugin> type) {
 
     public Plugin findPlugin(String id) {
         try {
-            return findPlugin(getTypeForId(id));
+            return findPlugin(getPluginTypeForId(id));
         } catch (UnknownPluginException e) {
             return null;
         }
@@ -167,6 +168,15 @@ public <P extends Plugin> P getPlugin(Class<P> type) throws UnknownPluginExcepti
     }
 
     public void withId(final String pluginId, Action<? super Plugin> action) {
+        try {
+            Class<?> typeForId = pluginRegistry.getTypeForId(pluginId);
+            if (!Plugin.class.isAssignableFrom(typeForId) && !new ModelRuleSourceDetector().getDeclaredSources(typeForId).isEmpty()) {
+                String message = String.format("The type for id '%s' (class: '%s') is a rule source and not a plugin. Use AppliedPlugins.withPlugin() to perform an action if a rule source is applied.", pluginId, typeForId.getName());
+                throw new IllegalArgumentException(message);
+            }
+        } catch (UnknownPluginException e) {
+            //just continue if it's impossible to see if the id points to a plain rule source
+        }
         matching(new Spec<Plugin>() {
             public boolean isSatisfiedBy(Plugin element) {
                 try {
@@ -178,7 +188,7 @@ public boolean isSatisfiedBy(Plugin element) {
         }).all(action);
     }
 
-    protected Class<? extends Plugin> getTypeForId(String id) {
+    protected Class<? extends Plugin> getPluginTypeForId(String id) {
         return pluginRegistry.getPluginTypeForId(id);
     }
 

diff --git a/subprojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultPluginRegistry.java b/subprojects/core/src/main/groovy/org/gradle/api/internal/plugins/DefaultPluginRegistry.java
@@ -134,7 +134,7 @@ public Class<?> getTypeForId(String pluginId) {
         return getTypeForId(pluginId, new Spec<Class<?>>() {
             public boolean isSatisfiedBy(Class<?> rawClass) {
                 ModelRuleSourceDetector detector = new ModelRuleSourceDetector();
-                return Plugin.class.isAssignableFrom(rawClass) || detector.getDeclaredSources(rawClass).size() > 0;
+                return Plugin.class.isAssignableFrom(rawClass) || !detector.getDeclaredSources(rawClass).isEmpty();
             }
         }, "Implementation class '%s' specified for plugin '%s' does not implement the Plugin interface and does not define any rule sources.");
     }

diff --git a/...ts/core/src/test/groovy/org/gradle/api/internal/plugins/DefaultPluginContainerTest.groovy b/...ts/core/src/test/groovy/org/gradle/api/internal/plugins/DefaultPluginContainerTest.groovy
@@ -19,6 +19,7 @@ import org.gradle.api.Plugin
 import org.gradle.api.Project
 import org.gradle.api.internal.project.TestPlugin1
 import org.gradle.api.internal.project.TestPlugin2
+import org.gradle.api.internal.project.TestRuleSource
 import org.gradle.api.plugins.UnknownPluginException
 import org.gradle.internal.reflect.DirectInstantiator
 import org.gradle.test.fixtures.file.TestNameTestDirectoryProvider
@@ -109,6 +110,7 @@ public class DefaultPluginContainerTest extends Specification {
 
     def "executes action for plugin with given id"() {
         def plugin = new TestPlugin1()
+        pluginRegistry.getTypeForId("plugin") >> TestPlugin1
         pluginRegistry.getPluginTypeForId("plugin") >> TestPlugin1
         def plugins = []
         container.add(plugin)
@@ -256,4 +258,25 @@ public class DefaultPluginContainerTest extends Specification {
         1 * applicationAction1.execute({ it.plugin == plugin && it.target == project })
         1 * applicationAction2.execute({ it.plugin == plugin && it.target == project })
     }
+
+    def "a useful error message is set when a plain rule source type is passed to withType"() {
+        when:
+        container.withType(TestRuleSource)
+
+        then:
+        IllegalArgumentException e = thrown()
+        e.message == "'$TestRuleSource.name' is a rule source and not a plugin. Use AppliedPlugins.withPlugin() to perform an action if a rule source is applied."
+    }
+
+    def "a useful error message is set when an id for plain rule source type is passed to withId"() {
+        given:
+        pluginRegistry.getTypeForId("custom-rule-source") >> TestRuleSource
+
+        when:
+        container.withId("custom-rule-source") {}
+
+        then:
+        IllegalArgumentException e = thrown()
+        e.message == "The type for id 'custom-rule-source' (class: '$TestRuleSource.name') is a rule source and not a plugin. Use AppliedPlugins.withPlugin() to perform an action if a rule source is applied."
+    }
 }
diff --git a/...core/src/test/groovy/org/gradle/model/internal/inspect/ModelRuleSourceDetectorTest.groovy b/...core/src/test/groovy/org/gradle/model/internal/inspect/ModelRuleSourceDetectorTest.groovy
@@ -22,6 +22,8 @@ import spock.lang.Unroll
 
 class ModelRuleSourceDetectorTest extends Specification {
 
+    private ModelRuleSourceDetector detector = new ModelRuleSourceDetector()
+
     static class HasOneSource {
         @RuleSource
         static class Source {}
@@ -46,7 +48,7 @@ class ModelRuleSourceDetectorTest extends Specification {
     @Unroll
     def "find model rule sources - #clazz"() {
         expect:
-        new ModelRuleSourceDetector().getDeclaredSources(clazz) == expected.toSet()
+        detector.getDeclaredSources(clazz) == expected.toSet()
 
         where:
         clazz         | expected