Seemingly stable label boolean expression support.

Driven by a test case.

git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@33620 71c3de6d-444a-0410-be80-ed276b4c234a

core/pom.xml

@@ -111,9 +111,9 @@ THE SOFTWARE.
         </executions>
       </plugin>
       <plugin>
-        <groupId>org.apache.maven.plugins</groupId>
-        <artifactId>maven-antlr-plugin</artifactId>
-        <version>2.0-beta-1</version>
+        <groupId>org.codehaus.mojo</groupId>
+        <artifactId>antlr-maven-plugin</artifactId>
+        <version>2.1</version>
         <executions>
           <execution>
             <id>cron</id>

core/src/main/grammar/labelExpr.g

@@ -31,40 +31,76 @@ options {
   defaultErrorHandler=false;
 }
 
+// order of precedence is as per http://en.wikipedia.org/wiki/Logical_connective#Order_of_precedence
+
 expr
 returns [Label l]
-  : l=term1
-  | x=term1 IFF y=term1 EOF
-    { l=x.iff(y); }
+  : l=term1 EOF
   ;
 
 term1
 returns [Label l]
-  : l=term2
-  | x=term2 AND y=term2 EOF
-    { l=x.and(y); }
+{ Label r; }
+  : l=term2( IFF r=term2 {l=l.iff(r);} )?
   ;
 
 term2
 returns [Label l]
-  : LPAREN l=expr RPAREN
+{ Label r; }
+  : l=term3( IMPLIES r=term3 {l=l.implies(r);} )?
+  ;
+
+term3
+returns [Label l]
+{ Label r; }
+  : l=term4 ( OR r=term4 {l=l.or(r);} )?
+  ;
+
+term4
+returns [Label l]
+{ Label r; }
+  : l=term5 ( AND r=term5 {l=l.and(r);} )?
+  ;
+
+term5
+returns [Label l]
+{ Label x; }
+  : l=term6
+  | NOT x=term6
+    { l=x.not(); }
+  ;
+
+term6
+returns [Label l]
+options { generateAmbigWarnings=false; }
+  : LPAREN l=term1 RPAREN
+    { l=l.paren(); }
   | a:ATOM
+    { LabelAtom.isValidName(a.getText()) }?
     { l=LabelAtom.get(a.getText()); }
   ;
 
 class LabelExpressionLexer extends Lexer;
 
-AND:    "&&";
-OR:     "||";
+AND:    "&";
+OR:     "|";
+NOT:    "!";
+IMPLIES:"->";
 IFF:    "<->";
 LPAREN: "(";
 RPAREN: ")";
 
-ATOM
-    :   ('0'..'9'|'a'..'z'|'A'..'Z')+
+protected
+IDENTIFIER_PART
+    :   ~( '&' | '|' | '!' | '-' | '<' | '>' | '(' | ')' | ' ' | '\t')
     ;
 
+ATOM
+/* the real check of valid identifier happens in LabelAtom.get() */
+    :   (IDENTIFIER_PART)+
+    ;
 
 WS
   : (' '|'\t')+
+    { $setType(Token.SKIP); }
   ;

core/src/main/java/hudson/model/AbstractProject.java

@@ -23,6 +23,7 @@
  */
 package hudson.model;
 
+import antlr.ANTLRException;
 import hudson.AbortException;
 import hudson.CopyOnWrite;
 import hudson.FeedAdapter;
@@ -1678,7 +1679,12 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
         }
 
         public FormValidation doCheckAssignedLabelString(@QueryParameter String value) {
-            // TODO: we can do a lot better than this
+            try {
+                Label.parseExpression(value);
+            } catch (ANTLRException e) {
+                return FormValidation.error(e,"Invalid boolean expression");
+            }
+            // TODO: if there's an atom in the expression that is empty, report it
             if (Hudson.getInstance().getLabel(value).isEmpty())
                 return FormValidation.warning("There's no slave/cloud that matches this assignment");
             return FormValidation.ok();

core/src/main/java/hudson/model/Hudson.java

@@ -25,6 +25,7 @@
  */
 package hudson.model;
 
+import antlr.ANTLRException;
 import com.thoughtworks.xstream.XStream;
 import hudson.BulkChange;
 import hudson.DNSMultiCast;
@@ -1373,7 +1374,11 @@ public final class Hudson extends Node implements ItemGroup<TopLevelItem>, Stapl
                 return l;
 
             // non-existent
-            labels.putIfAbsent(expr,Label.parseExpression(expr));
+            try {
+                labels.putIfAbsent(expr,Label.parseExpression(expr));
+            } catch (ANTLRException e) {
+                throw new IllegalArgumentException("Invalid label expression: "+expr,e);
+            }
         }
     }
 
@@ -1381,11 +1386,18 @@ public final class Hudson extends Node implements ItemGroup<TopLevelItem>, Stapl
      * Returns the label atom of the given name.
      */
     public LabelAtom getLabelAtom(String name) {
-        if(name==null)  return null;
-        Label l = getLabel(name);
-        if (l instanceof LabelAtom)
-            return (LabelAtom)l;
-        return null;
+        if (name==null)  return null;
+        if (!LabelAtom.isValidName(name))
+            throw new IllegalArgumentException("Illegal character in a token: "+name);
+
+        while(true) {
+            Label l = labels.get(name);
+            if(l!=null)
+                return (LabelAtom)l;
+
+            // non-existent
+            labels.putIfAbsent(name,new LabelAtom(name));
+        }
     }
 
     /**

core/src/main/java/hudson/model/Label.java

@@ -23,17 +23,22 @@
  */
 package hudson.model;
 
+import antlr.ANTLRException;
 import hudson.Util;
 import static hudson.Util.fixNull;
 
 import hudson.model.label.LabelAtom;
 import hudson.model.label.LabelExpression;
+import hudson.model.label.LabelExpressionLexer;
+import hudson.model.label.LabelExpressionParser;
+import hudson.model.label.LabelOperatorPrecedence;
 import hudson.slaves.NodeProvisioner;
 import hudson.slaves.Cloud;
 import hudson.util.VariableResolver;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
 
+import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -326,6 +331,56 @@ public abstract class Label implements Comparable<Label>, ModelObject {
         return new Api(this);
     }
 
+    /**
+     * Returns the label that represents "this&amp;rhs"
+     */
+    public Label and(Label rhs) {
+        return new LabelExpression.And(this,rhs);
+    }
+
+    /**
+     * Returns the label that represents "this|rhs"
+     */
+    public Label or(Label rhs) {
+        return new LabelExpression.Or(this,rhs);
+    }
+
+    /**
+     * Returns the label that represents "this&lt;->rhs"
+     */
+    public Label iff(Label rhs) {
+        return new LabelExpression.Iff(this,rhs);
+    }
+
+    /**
+     * Returns the label that represents "this->rhs"
+     */
+    public Label implies(Label rhs) {
+        return new LabelExpression.Implies(this,rhs);
+    }
+
+    /**
+     * Returns the label that represents "!this"
+     */
+    public Label not() {
+        return new LabelExpression.Not(this);
+    }
+
+    /**
+     * Returns the label that represents "(this)"
+     * This is a pointless operation for machines, but useful
+     * for humans who find the additional parenthesis often useful
+     */
+    public Label paren() {
+        return new LabelExpression.Paren(this);
+    }
+
+    /**
+     * Precedence of the top most operator.
+     */
+    public abstract LabelOperatorPrecedence precedence();
+
+
     @Override
     public boolean equals(Object that) {
         if (this == that) return true;
@@ -398,14 +453,8 @@ public abstract class Label implements Comparable<Label>, ModelObject {
      *
      * TODO: replace this with a real parser later
      */
-    public static Label parseExpression(String labelExpression)  {
-        Label l = null;
-        for (String atom : labelExpression.split("&&")) {
-            atom = atom.trim();
-            Label r = new LabelAtom(atom);
-            if (l==null)    l = r;
-            else            l = new LabelExpression.And(l,r);
-        }
-        return l;
+    public static Label parseExpression(String labelExpression) throws ANTLRException {
+        LabelExpressionLexer lexer = new LabelExpressionLexer(new StringReader(labelExpression));
+        return new LabelExpressionParser(lexer).expr();
     }
 }

core/src/main/java/hudson/model/label/LabelAtom.java

@@ -43,10 +43,27 @@ public class LabelAtom extends Label {
         return resolver.resolve(name);
     }
 
+    @Override
+    public LabelOperatorPrecedence precedence() {
+        return LabelOperatorPrecedence.ATOM;
+    }
+
     /**
      * Obtains an atom by its {@linkplain #getName() name}.
      */
     public static LabelAtom get(String l) {
         return Hudson.getInstance().getLabelAtom(l);
     }
+
+    public static boolean isValidName(String name) {
+        for (int i=name.length()-1; i>=0; i--) {
+            char ch = name.charAt(i);
+            if (Character.isJavaIdentifierPart(ch))
+                continue;
+            if (ch=='.')
+                continue;
+            return false;
+        }
+        return true;
+    }
 }

core/src/main/java/hudson/model/label/LabelExpression.java

@@ -37,18 +37,142 @@ public abstract class LabelExpression extends Label {
         super(name);
     }
 
-    public static final class And extends LabelExpression {
+    public static class Not extends LabelExpression {
+        private final Label base;
+
+        public Not(Label base) {
+            super('!'+paren(LabelOperatorPrecedence.NOT,base));
+            this.base = base;
+        }
+
+        @Override
+        public boolean matches(VariableResolver<Boolean> resolver) {
+            return !base.matches(resolver);
+        }
+
+        @Override
+        public LabelOperatorPrecedence precedence() {
+            return LabelOperatorPrecedence.NOT;
+        }
+    }
+
+    /**
+     * No-op but useful for preserving the parenthesis in the user input.
+     */
+    public static class Paren extends LabelExpression {
+        private final Label base;
+
+        public Paren(Label base) {
+            super('('+base.getName()+')');
+            this.base = base;
+        }
+
+        @Override
+        public boolean matches(VariableResolver<Boolean> resolver) {
+            return base.matches(resolver);
+        }
+
+        @Override
+        public LabelOperatorPrecedence precedence() {
+            return LabelOperatorPrecedence.ATOM;
+        }
+    }
+
+    /**
+     * Puts the label name into a parenthesis if the given operator will have a higher precedence.
+     */
+    static String paren(LabelOperatorPrecedence op, Label l) {
+        if (op.compareTo(l.precedence())<0)
+            return '('+l.getName()+')';
+        return l.getName();
+    }
+
+    public static abstract class Binary extends LabelExpression {
         private final Label lhs,rhs;
 
-        public And(Label lhs, Label rhs) {
-            super(lhs.getName()+"&&"+rhs.getName());
+        public Binary(Label lhs, Label rhs, LabelOperatorPrecedence op) {
+            super(combine(lhs, rhs, op));
             this.lhs = lhs;
             this.rhs = rhs;
         }
 
+        private static String combine(Label lhs, Label rhs, LabelOperatorPrecedence op) {
+            return paren(op,lhs)+op.str+paren(op,rhs);
+        }
+
+        /**
+         * Note that we evaluate both branches of the expression all the time.
+         * That is, it behaves like "a|b" not "a||b"
+         */
         @Override
         public boolean matches(VariableResolver<Boolean> resolver) {
-            return lhs.matches(resolver) && rhs.matches(resolver);
+            return op(lhs.matches(resolver),rhs.matches(resolver));
+        }
+
+        protected abstract boolean op(boolean a, boolean b);
+    }
+
+    public static final class And extends Binary {
+        public And(Label lhs, Label rhs) {
+            super(lhs, rhs, LabelOperatorPrecedence.AND);
+        }
+
+        @Override
+        protected boolean op(boolean a, boolean b) {
+            return a && b;
+        }
+
+        @Override
+        public LabelOperatorPrecedence precedence() {
+            return LabelOperatorPrecedence.AND;
+        }
+    }
+
+    public static final class Or extends Binary {
+        public Or(Label lhs, Label rhs) {
+            super(lhs, rhs, LabelOperatorPrecedence.OR);
+        }
+
+        @Override
+        protected boolean op(boolean a, boolean b) {
+            return a || b;
+        }
+
+        @Override
+        public LabelOperatorPrecedence precedence() {
+            return LabelOperatorPrecedence.OR;
+        }
+    }
+
+    public static final class Iff extends Binary {
+        public Iff(Label lhs, Label rhs) {
+            super(lhs, rhs, LabelOperatorPrecedence.IFF);
+        }
+
+        @Override
+        protected boolean op(boolean a, boolean b) {
+            return !(a ^ b);
+        }
+
+        @Override
+        public LabelOperatorPrecedence precedence() {
+            return LabelOperatorPrecedence.IFF;
+        }
+    }
+
+    public static final class Implies extends Binary {
+        public Implies(Label lhs, Label rhs) {
+            super(lhs, rhs, LabelOperatorPrecedence.IMPLIES);
+        }
+
+        @Override
+        protected boolean op(boolean a, boolean b) {
+            return !a || b;
+        }
+
+        @Override
+        public LabelOperatorPrecedence precedence() {
+            return LabelOperatorPrecedence.IMPLIES;
         }
     }
 }

core/src/main/java/hudson/model/label/LabelOperatorPrecedence.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2010, InfraDNA, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package hudson.model.label;
+
+/**
+ * Precedence of the top most operator.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.COMPOSITELABEL
+ */
+public enum LabelOperatorPrecedence {
+    ATOM(null), NOT("!"), AND("&"), OR("|"), IMPLIES("->"), IFF("<->");
+
+    /**
+     * String representation of this operator.
+     */
+    public final String str;
+
+    LabelOperatorPrecedence(String str) {
+        this.str = str;
+    }
+}

core/src/main/java/hudson/model/label/package-info.java

+/**
+ * Boolean expression over labels.
+ */
+package hudson.model.label;

test/src/main/java/org/jvnet/hudson/test/HudsonTestCase.java

@@ -507,7 +507,7 @@ public abstract class HudsonTestCase extends TestCase implements RootAction {
     }
 
     public DumbSlave createSlave() throws Exception {
-        return createSlave(null);
+        return createSlave("",null);
     }
 
     /**
@@ -549,17 +549,25 @@ public abstract class HudsonTestCase extends TestCase implements RootAction {
         return new URL("http://localhost:"+localPort+contextPath+"/");
     }
 
+    public DumbSlave createSlave(EnvVars env) throws Exception {
+        return createSlave("",env);
+    }
+
+    public DumbSlave createSlave(Label l, EnvVars env) throws Exception {
+        return createSlave(l.getName(), null);
+    }
+
     /**
      * Creates a slave with certain additional environment variables
      */
-    public DumbSlave createSlave(Label l, EnvVars env) throws Exception {
+    public DumbSlave createSlave(String labels, EnvVars env) throws Exception {
         synchronized (hudson) {
             // this synchronization block is so that we don't end up adding the same slave name more than once.
 
             int sz = hudson.getNodes().size();
 
             DumbSlave slave = new DumbSlave("slave" + sz, "dummy",
-    				createTmpDir().getPath(), "1", Mode.NORMAL, l==null?"":l.getName(), createComputerLauncher(env), RetentionStrategy.NOOP, Collections.EMPTY_LIST);
+    				createTmpDir().getPath(), "1", Mode.NORMAL, labels==null?"":labels, createComputerLauncher(env), RetentionStrategy.NOOP, Collections.EMPTY_LIST);
     		hudson.addNode(slave);
     		return slave;
     	}

test/src/test/java/hudson/maven/MavenOptsTest.java

@@ -24,7 +24,7 @@ public class MavenOptsTest extends HudsonTestCase {
         MavenModuleSet m = createMavenProject();
         m.setScm(new ExtractResourceSCM(getClass().getResource("maven-opts-echo.zip")));
         m.setGoals("validate");
-        m.setAssignedLabel(createSlave(null, new EnvVars("MAVEN_OPTS", "-Dhudson.mavenOpt.test=foo")).getSelfLabel());
+        m.setAssignedLabel(createSlave(new EnvVars("MAVEN_OPTS", "-Dhudson.mavenOpt.test=foo")).getSelfLabel());
         
         buildAndAssertSuccess(m);
 
@@ -38,7 +38,7 @@ public class MavenOptsTest extends HudsonTestCase {
         m.setScm(new ExtractResourceSCM(getClass().getResource("maven-opts-echo.zip")));
         m.setGoals("validate");
         m.setMavenOpts("-Dhudson.mavenOpt.test=bar");
-        m.setAssignedLabel(createSlave(null, new EnvVars("MAVEN_OPTS", "-Dhudson.mavenOpt.test=foo")).getSelfLabel());
+        m.setAssignedLabel(createSlave(new EnvVars("MAVEN_OPTS", "-Dhudson.mavenOpt.test=foo")).getSelfLabel());
         
         buildAndAssertSuccess(m);
 
@@ -51,7 +51,7 @@ public class MavenOptsTest extends HudsonTestCase {
         m.setScm(new ExtractResourceSCM(getClass().getResource("maven-opts-echo.zip")));
         m.setGoals("validate");
         m.DESCRIPTOR.setGlobalMavenOpts("-Dhudson.mavenOpt.test=bar");
-        m.setAssignedLabel(createSlave(null, new EnvVars("MAVEN_OPTS", "-Dhudson.mavenOpt.test=foo")).getSelfLabel());
+        m.setAssignedLabel(createSlave(new EnvVars("MAVEN_OPTS", "-Dhudson.mavenOpt.test=foo")).getSelfLabel());
         m.setMavenOpts("-Dhudson.mavenOpt.test=baz");
         
         buildAndAssertSuccess(m);

test/src/test/java/hudson/maven/MavenProjectTest.java

@@ -108,7 +108,7 @@ public class MavenProjectTest extends HudsonTestCase {
     public void testMultiModuleSiteBuildOnSlave() throws Exception {
         MavenModuleSet project = createProject("maven-multimodule-site.zip");
         project.setGoals("site");
-        project.setAssignedLabel(createSlave(null, null).getSelfLabel());
+        project.setAssignedLabel(createSlave().getSelfLabel());
 
         buildAndAssertSuccess(project);
 

test/src/test/java/hudson/model/label/LabelExpressionTest.java

@@ -23,6 +23,7 @@
  */
 package hudson.model.label;
 
+import antlr.ANTLRException;
 import hudson.Launcher;
 import hudson.model.AbstractBuild;
 import hudson.model.BuildListener;
@@ -44,9 +45,9 @@ public class LabelExpressionTest extends HudsonTestCase {
      * Verifies the queueing behavior in the presence of the expression.
      */
     public void testQueueBehavior() throws Exception {
-        DumbSlave w32 = createSlave(hudson.getLabel("win 32bit"));
-        DumbSlave w64 = createSlave(hudson.getLabel("win 64bit"));
-        DumbSlave l32 = createSlave(hudson.getLabel("linux 32bit"));
+        DumbSlave w32 = createSlave("win 32bit",null);
+        DumbSlave w64 = createSlave("win 64bit",null);
+        DumbSlave l32 = createSlave("linux 32bit",null);
 
         final SequenceLock seq = new SequenceLock();
 
@@ -59,10 +60,10 @@ public class LabelExpressionTest extends HudsonTestCase {
                 return true;
             }
         });
-        p1.setAssignedLabel(hudson.getLabel("win&&32bit"));
+        p1.setAssignedLabel(hudson.getLabel("win & 32bit"));
 
         FreeStyleProject p2 = createFreeStyleProject();
-        p2.setAssignedLabel(hudson.getLabel("win&&32bit"));
+        p2.setAssignedLabel(hudson.getLabel("win & 32bit"));
 
         FreeStyleProject p3 = createFreeStyleProject();
         p3.setAssignedLabel(hudson.getLabel("win"));
@@ -88,4 +89,52 @@ public class LabelExpressionTest extends HudsonTestCase {
         FreeStyleBuild b2 = assertBuildStatusSuccess(f2);
         assertSame(w32,b2.getBuiltOn());
     }
+
+    /**
+     * Tests the expression parser.
+     */
+    public void testParser() throws Exception {
+        parseAndVerify("foo", "foo");
+        parseAndVerify("32bit.dot", "32bit.dot");
+        parseAndVerify("foo|bar", "foo | bar");
+
+        // user-given parenthesis is preserved
+        parseAndVerify("foo|bar&zot", "foo|bar&zot");
+        parseAndVerify("foo|(bar&zot)", "foo|(bar&zot)");
+
+        parseAndVerify("(foo|bar)&zot", "(foo|bar)&zot");
+        parseAndVerify("foo->bar", "foo ->\tbar");
+        parseAndVerify("!foo<->bar", "!foo <-> bar");
+    }
+
+    private void parseAndVerify(String expected, String expr) throws ANTLRException {
+        assertEquals(expected, LabelExpression.parseExpression(expr).getName());
+    }
+
+    public void testParserError() throws Exception {
+        parseShouldFail("foo bar");
+        parseShouldFail("foo*bar");
+    }
+
+    /**
+     * The name should have parenthesis at the right place to preserve the tree structure.
+     */
+    public void testComposite() {
+        LabelAtom x = hudson.getLabelAtom("x");
+        assertEquals("!!x",x.not().not().getName());
+        assertEquals("(x|x)&x",x.or(x).and(x).getName());
+        assertEquals("x&x|x",x.and(x).or(x).getName());
+    }
+
+    private void parseShouldFail(String expr) {
+        try {
+            LabelExpression.parseExpression(expr);
+            fail();
+        } catch (ANTLRException e) {
+            // expected
+        }
+    }
+
+    public void testFormValidation() throws Exception {
+    }
 }