From a34186bbf19de1addcc51ea2bbcbc4ec04bb4367 Mon Sep 17 00:00:00 2001
From: Kohsuke Kawaguchi <kk@kohsuke.org>
Date: Thu, 30 Jun 2011 08:54:51 -0700
Subject: [PATCH] Added a better form validation for non-existent/typo in
 labels

---
 changelog.html                                |  2 +
 .../java/hudson/model/AbstractProject.java    | 11 ++-
 core/src/main/java/hudson/model/Label.java    | 71 +++++++++++++++++++
 .../java/hudson/model/labels/LabelAtom.java   | 21 ++++++
 .../hudson/model/labels/LabelExpression.java  | 36 +++++++++-
 .../hudson/model/labels/LabelVisitor.java     | 24 +++++++
 .../hudson/model/Messages.properties          |  1 +
 .../model/labels/LabelExpressionTest.java     | 15 ++++
 8 files changed, 176 insertions(+), 5 deletions(-)
 create mode 100644 core/src/main/java/hudson/model/labels/LabelVisitor.java

diff --git a/changelog.html b/changelog.html
index 37b6c40ca7..0d69a9a3d3 100644
--- a/changelog.html
+++ b/changelog.html
@@ -75,6 +75,8 @@ Upcoming changes</a>
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-10066">issue 10066</a>)
   <li class=rfe>
     Groovy script console is now syntax highlighted.
+  <li class=rfe>
+    Improved the form validation to the "restrict where jobs can run" field.
   <li class=rfe>
     Text area to enter description is now syntax highlighted.
 </ul>
diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java
index d97505ff43..c111792991 100644
--- a/core/src/main/java/hudson/model/AbstractProject.java
+++ b/core/src/main/java/hudson/model/AbstractProject.java
@@ -1859,9 +1859,16 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
                 return FormValidation.error(e,
                         Messages.AbstractProject_AssignedLabelString_InvalidBooleanExpression(e.getMessage()));
             }
-            // TODO: if there's an atom in the expression that is empty, report it
-            if (Jenkins.getInstance().getLabel(value).isEmpty())
+            Label l = Jenkins.getInstance().getLabel(value);
+            if (l.isEmpty()) {
+                for (LabelAtom a : l.listAtoms()) {
+                    if (a.isEmpty()) {
+                        LabelAtom nearest = LabelAtom.findNearest(a.getName());
+                        return FormValidation.warning(Messages.AbstractProject_AssignedLabelString_NoMatch_DidYouMean(a.getName(),nearest.getDisplayName()));
+                    }
+                }
                 return FormValidation.warning(Messages.AbstractProject_AssignedLabelString_NoMatch());
+            }
             return FormValidation.ok();
         }
 
diff --git a/core/src/main/java/hudson/model/Label.java b/core/src/main/java/hudson/model/Label.java
index 68235cddc2..e03abcc52c 100644
--- a/core/src/main/java/hudson/model/Label.java
+++ b/core/src/main/java/hudson/model/Label.java
@@ -29,9 +29,17 @@ import static hudson.Util.fixNull;
 
 import hudson.model.labels.LabelAtom;
 import hudson.model.labels.LabelExpression;
+import hudson.model.labels.LabelExpression.And;
+import hudson.model.labels.LabelExpression.Binary;
+import hudson.model.labels.LabelExpression.Iff;
+import hudson.model.labels.LabelExpression.Implies;
+import hudson.model.labels.LabelExpression.Not;
+import hudson.model.labels.LabelExpression.Or;
+import hudson.model.labels.LabelExpression.Paren;
 import hudson.model.labels.LabelExpressionLexer;
 import hudson.model.labels.LabelExpressionParser;
 import hudson.model.labels.LabelOperatorPrecedence;
+import hudson.model.labels.LabelVisitor;
 import hudson.slaves.NodeProvisioner;
 import hudson.slaves.Cloud;
 import hudson.util.QuotedStringTokenizer;
@@ -358,6 +366,22 @@ public abstract class Label extends Actionable implements Comparable<Label>, Mod
         return new Api(this);
     }
 
+    /**
+     * Accepts a visitor and call its respective "onXYZ" method based no the actual type of 'this'.
+     */
+    public abstract <V,P> V accept(LabelVisitor<V,P> visitor, P param);
+
+    /**
+     * Lists up all the atoms contained in in this label.
+     *
+     * @since 1.420
+     */
+    public Set<LabelAtom> listAtoms() {
+        Set<LabelAtom> r = new HashSet<LabelAtom>();
+        accept(ATOM_COLLECTOR,r);
+        return r;
+    }
+
     /**
      * Returns the label that represents "this&amp;rhs"
      */
@@ -484,4 +508,51 @@ public abstract class Label extends Actionable implements Comparable<Label>, Mod
         LabelExpressionLexer lexer = new LabelExpressionLexer(new StringReader(labelExpression));
         return new LabelExpressionParser(lexer).expr();
     }
+
+    /**
+     * Collects all the atoms in the expression.
+     */
+    private static final LabelVisitor<Void,Set<LabelAtom>> ATOM_COLLECTOR = new LabelVisitor<Void,Set<LabelAtom>>() {
+        @Override
+        public Void onAtom(LabelAtom a, Set<LabelAtom> param) {
+            param.add(a);
+            return null;
+        }
+
+        @Override
+        public Void onParen(Paren p, Set<LabelAtom> param) {
+            return p.base.accept(this,param);
+        }
+
+        @Override
+        public Void onNot(Not p, Set<LabelAtom> param) {
+            return p.base.accept(this,param);
+        }
+
+        @Override
+        public Void onAnd(And p, Set<LabelAtom> param) {
+            return onBinary(p,param);
+        }
+
+        @Override
+        public Void onOr(Or p, Set<LabelAtom> param) {
+            return onBinary(p,param);
+        }
+
+        @Override
+        public Void onIff(Iff p, Set<LabelAtom> param) {
+            return onBinary(p,param);
+        }
+
+        @Override
+        public Void onImplies(Implies p, Set<LabelAtom> param) {
+            return onBinary(p,param);
+        }
+
+        private Void onBinary(Binary b, Set<LabelAtom> param) {
+            b.lhs.accept(this,param);
+            b.rhs.accept(this,param);
+            return null;
+        }
+    };
 }
diff --git a/core/src/main/java/hudson/model/labels/LabelAtom.java b/core/src/main/java/hudson/model/labels/LabelAtom.java
index 5c85c17618..e43eef3cb6 100644
--- a/core/src/main/java/hudson/model/labels/LabelAtom.java
+++ b/core/src/main/java/hudson/model/labels/LabelAtom.java
@@ -33,6 +33,7 @@ import hudson.XmlFile;
 import hudson.model.Action;
 import hudson.model.Descriptor.FormException;
 import hudson.model.Failure;
+import hudson.util.EditDistance;
 import jenkins.model.Jenkins;
 import hudson.model.Label;
 import hudson.model.Saveable;
@@ -48,8 +49,10 @@ import org.kohsuke.stapler.export.Exported;
 import javax.servlet.ServletException;
 import java.io.File;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import java.util.Vector;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -143,6 +146,16 @@ public class LabelAtom extends Label implements Saveable {
         return resolver.resolve(name);
     }
 
+    @Override
+    public <V, P> V accept(LabelVisitor<V, P> visitor, P param) {
+        return visitor.onAtom(this,param);
+    }
+
+    @Override
+    public Set<LabelAtom> listAtoms() {
+        return Collections.singleton(this);
+    }
+
     @Override
     public LabelOperatorPrecedence precedence() {
         return LabelOperatorPrecedence.ATOM;
@@ -206,6 +219,14 @@ public class LabelAtom extends Label implements Saveable {
         return Jenkins.getInstance().getLabelAtom(l);
     }
 
+    public static LabelAtom findNearest(String name) {
+        List<String> candidates = new ArrayList<String>();
+        for (LabelAtom a : Jenkins.getInstance().getLabelAtoms()) {
+            candidates.add(a.getName());
+        }
+        return get(EditDistance.findNearest(name, candidates));
+    }
+
     public static boolean needsEscape(String name) {
         try {
             Jenkins.checkGoodName(name);
diff --git a/core/src/main/java/hudson/model/labels/LabelExpression.java b/core/src/main/java/hudson/model/labels/LabelExpression.java
index babb1b4761..861d33be98 100644
--- a/core/src/main/java/hudson/model/labels/LabelExpression.java
+++ b/core/src/main/java/hudson/model/labels/LabelExpression.java
@@ -43,7 +43,7 @@ public abstract class LabelExpression extends Label {
     }
 
     public static class Not extends LabelExpression {
-        private final Label base;
+        public final Label base;
 
         public Not(Label base) {
             super('!'+paren(LabelOperatorPrecedence.NOT,base));
@@ -55,6 +55,11 @@ public abstract class LabelExpression extends Label {
             return !base.matches(resolver);
         }
 
+        @Override
+        public <V, P> V accept(LabelVisitor<V, P> visitor, P param) {
+            return visitor.onNot(this, param);
+        }
+
         @Override
         public LabelOperatorPrecedence precedence() {
             return LabelOperatorPrecedence.NOT;
@@ -65,7 +70,7 @@ public abstract class LabelExpression extends Label {
      * No-op but useful for preserving the parenthesis in the user input.
      */
     public static class Paren extends LabelExpression {
-        private final Label base;
+        public final Label base;
 
         public Paren(Label base) {
             super('('+base.getExpression()+')');
@@ -77,6 +82,11 @@ public abstract class LabelExpression extends Label {
             return base.matches(resolver);
         }
 
+        @Override
+        public <V, P> V accept(LabelVisitor<V, P> visitor, P param) {
+            return visitor.onParen(this, param);
+        }
+
         @Override
         public LabelOperatorPrecedence precedence() {
             return LabelOperatorPrecedence.ATOM;
@@ -93,7 +103,7 @@ public abstract class LabelExpression extends Label {
     }
 
     public static abstract class Binary extends LabelExpression {
-        private final Label lhs,rhs;
+        public final Label lhs,rhs;
 
         public Binary(Label lhs, Label rhs, LabelOperatorPrecedence op) {
             super(combine(lhs, rhs, op));
@@ -127,6 +137,11 @@ public abstract class LabelExpression extends Label {
             return a && b;
         }
 
+        @Override
+        public <V, P> V accept(LabelVisitor<V, P> visitor, P param) {
+            return visitor.onAnd(this, param);
+        }
+
         @Override
         public LabelOperatorPrecedence precedence() {
             return LabelOperatorPrecedence.AND;
@@ -143,6 +158,11 @@ public abstract class LabelExpression extends Label {
             return a || b;
         }
 
+        @Override
+        public <V, P> V accept(LabelVisitor<V, P> visitor, P param) {
+            return visitor.onOr(this, param);
+        }
+
         @Override
         public LabelOperatorPrecedence precedence() {
             return LabelOperatorPrecedence.OR;
@@ -159,6 +179,11 @@ public abstract class LabelExpression extends Label {
             return !(a ^ b);
         }
 
+        @Override
+        public <V, P> V accept(LabelVisitor<V, P> visitor, P param) {
+            return visitor.onIff(this, param);
+        }
+
         @Override
         public LabelOperatorPrecedence precedence() {
             return LabelOperatorPrecedence.IFF;
@@ -175,6 +200,11 @@ public abstract class LabelExpression extends Label {
             return !a || b;
         }
 
+        @Override
+        public <V, P> V accept(LabelVisitor<V, P> visitor, P param) {
+            return visitor.onImplies(this, param);
+        }
+
         @Override
         public LabelOperatorPrecedence precedence() {
             return LabelOperatorPrecedence.IMPLIES;
diff --git a/core/src/main/java/hudson/model/labels/LabelVisitor.java b/core/src/main/java/hudson/model/labels/LabelVisitor.java
new file mode 100644
index 0000000000..fcb2f9b58e
--- /dev/null
+++ b/core/src/main/java/hudson/model/labels/LabelVisitor.java
@@ -0,0 +1,24 @@
+package hudson.model.labels;
+
+/**
+ * Visitor pattern for {@link LabelExpression}.
+ *
+ * @author Kohsuke Kawaguchi
+ * @see LabelExpression#accept(LabelVisitor, Object)
+ * @since 1.420
+ */
+public abstract class LabelVisitor<V,P> {
+    public abstract V onAtom(LabelAtom a, P param);
+
+    public abstract V onParen(LabelExpression.Paren p, P param);
+
+    public abstract V onNot(LabelExpression.Not p, P param);
+
+    public abstract V onAnd(LabelExpression.And p, P param);
+
+    public abstract V onOr(LabelExpression.Or p, P param);
+
+    public abstract V onIff(LabelExpression.Iff p, P param);
+
+    public abstract V onImplies(LabelExpression.Implies p, P param);
+}
diff --git a/core/src/main/resources/hudson/model/Messages.properties b/core/src/main/resources/hudson/model/Messages.properties
index 13131f93f6..3f21070691 100644
--- a/core/src/main/resources/hudson/model/Messages.properties
+++ b/core/src/main/resources/hudson/model/Messages.properties
@@ -28,6 +28,7 @@ AbstractBuild.KeptBecause=kept because of {0}
 
 AbstractItem.NoSuchJobExists=No such job ''{0}'' exists. Perhaps you meant ''{1}''?
 AbstractItem.Pronoun=Job
+AbstractProject.AssignedLabelString_NoMatch_DidYouMean=There''s no slave/cloud that matches this assignment. Did you mean ''{1}'' instead of ''{0}''?
 AbstractProject.NewBuildForWorkspace=Scheduling a new build to get a workspace.
 AbstractProject.AwaitingBuildForWorkspace=Awaiting build to get a workspace.
 AbstractProject.Pronoun=Project
diff --git a/test/src/test/java/hudson/model/labels/LabelExpressionTest.java b/test/src/test/java/hudson/model/labels/LabelExpressionTest.java
index 6579bcc48f..261e8f7728 100644
--- a/test/src/test/java/hudson/model/labels/LabelExpressionTest.java
+++ b/test/src/test/java/hudson/model/labels/LabelExpressionTest.java
@@ -42,6 +42,7 @@ import org.jvnet.hudson.test.TestBuilder;
 import java.io.IOException;
 import java.lang.reflect.Field;
 import java.util.Collections;
+import java.util.concurrent.Callable;
 import java.util.concurrent.Future;
 
 /**
@@ -207,5 +208,19 @@ public class LabelExpressionTest extends HudsonTestCase {
     }
 
     public void testFormValidation() throws Exception {
+        executeOnServer(new Callable<Object>() {
+            public Object call() throws Exception {
+                Label l = jenkins.getLabel("foo");
+                DumbSlave s = createSlave(l);
+                String msg = FreeStyleProject.DESCRIPTOR.doCheckAssignedLabelString("goo").renderHtml();
+                assertTrue(msg.contains("foo"));
+                assertTrue(msg.contains("goo"));
+
+                msg = FreeStyleProject.DESCRIPTOR.doCheckAssignedLabelString("master && goo").renderHtml();
+                assertTrue(msg.contains("foo"));
+                assertTrue(msg.contains("goo"));
+                return null;
+            }
+        });
     }
 }