Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-22975

Exception parsing "implies" expression - Label.parseExpression("a->b")

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major Major
    • core
    • None
    • 2.313

      While writing integration tests for a Jenkins plugin I identified the following Bugs and inconsistencies regarding Label parsing and handling.
      I wrote additional tests reproducing the findings.

      1. Parsing "a->b" fails with an Exception. With blanks "a -> b" it works, but Label.getName() again returns "a->b" without blanks - which cannot be parsed!

        /**
         * Implies parsing fails with Exception. "a->b" cannot be parsed (see labelB).
         * On the other hand Implies.getName() returns exactly this String for a valid
         * instance. See labelA parsed from "a -> b" with blanks (which works).
         */
        @Test
        public void testParseExpressionImplies() throws IOException, ANTLRException {
          //This one works
          Label labelA = Label.parseExpression("a -> b");
          assertEquals(Implies.class, labelA.getClass());
          assertEquals("a->b", labelA.getName());
          
          //TokenStreamRecognitionException: line 1:3: unexpected char: '>'
          //at hudson.model.labels.LabelExpressionLexer.nextToken(LabelExpressionLexer.java:127)
          Label labelB = Label.parseExpression("a->b");
          
          assertEquals(Implies.class, labelB.getClass());
          assertEquals("a->b", labelB.getName());
        }
      

      2. When assigning an "Implies" instance to a project the result is a "LabelAtom" which is not correct and causes issues!

        /**
         * Setting the Implies expression without blanks does not work at all
         * (see {@link #testParseExpressionImplies}).
         * Setting it with blanks works, but after saving the job the result
         * is a LabelAtom instead of a LabelExpression!
         */
        @Test
        public void testImpliesViaAPI() throws IOException, ANTLRException {
          Label label = Label.parseExpression("a -> b");
          assertEquals(Implies.class, label.getClass());
          assertEquals("a->b", label.getName());
          FreeStyleProject project = j.createFreeStyleProject();
          project.setAssignedLabel(label);
          project.save();
      
          assertEquals("a->b", project.getAssignedLabel().getName());
          assertEquals("\"a->b\"", project.getAssignedLabelString());
      
          //AssertionError: expected:<class hudson.model.labels.LabelExpression$Implies> 
          //                 but was:<class hudson.model.labels.LabelAtom>
          assertEquals(Implies.class, project.getAssignedLabel().getClass());
        }
      

      3. The same problem occurs in the UI when using "a->b" without blanks

        /**
         * Setting the Implies expression in the UI (correctly) without blanks works, but the result
         * is a LabelAtom instead of a LabelExpression!
         */
        @Test
        public void testImpliesViaUI() throws Exception {
          FreeStyleProject project = j.createFreeStyleProject();
          setViaReflection(project, AbstractProject.class, "assignedNode", "a->b");
          project.save();
          
          assertEquals("a->b", project.getAssignedLabel().getName());
          assertEquals("\"a->b\"", project.getAssignedLabelString());
      
          //AssertionError: expected:<class hudson.model.labels.LabelExpression$Implies> 
          //                 but was:<class hudson.model.labels.LabelAtom>
          assertEquals(Implies.class, project.getAssignedLabel().getClass());
        }
      

      3b. With blanks it is OK:

        /**
         * Saving the Implies expression with blanks in the UI works properly.
         */
        @Test
        public void testImpliesViaUIWithBlanks() throws Exception {
          FreeStyleProject project = j.createFreeStyleProject();
          setViaReflection(project, AbstractProject.class, "assignedNode", "a -> b");
          project.save();
          
          assertEquals(Implies.class, project.getAssignedLabel().getClass());
          assertEquals("a->b", project.getAssignedLabel().getName());
          assertEquals("a -> b", project.getAssignedLabelString());
        }
      

      4. I also identified some inconsistency regarding "master" when using UI and API:

        /**
         * Setting "master" as label in UI works properly as expected.
         */
        @Test
        public void testMasterViaUI() throws Exception {
          FreeStyleProject project = j.createFreeStyleProject();
          setViaReflection(project, AbstractProject.class, "assignedNode", "master");
          project.save();
          assertEquals("master", project.getAssignedLabel().getName());
          assertEquals("master", project.getAssignedLabelString());
        }
      
        /**
         * Setting "master" as label via API results in a different, unexpected behavior.
         * project.getAssignedLabelString returns null. 
         */
        @Test
        public void testMasterViaAPI() throws IOException {
          Label label = Label.get("master");
          assertEquals("master", label.getName());
          FreeStyleProject project = j.createFreeStyleProject();
          project.save();
          project.setAssignedLabel(label);
          assertEquals("master", project.getAssignedLabel().getName());
          
          // value is null:
          assertEquals("master", project.getAssignedLabelString());
        }
      

      Here the whole test class:

      import static org.junit.Assert.assertEquals;
      import hudson.model.AbstractProject;
      import hudson.model.FreeStyleProject;
      import hudson.model.Label;
      import hudson.model.labels.LabelExpression.Iff;
      import hudson.model.labels.LabelExpression.Implies;
      
      import java.io.IOException;
      import java.lang.reflect.Field;
      
      import org.junit.Rule;
      import org.junit.Test;
      import org.jvnet.hudson.test.JenkinsRule;
      
      import antlr.ANTLRException;
      
      public class ReproduceJenkinsLabelBugs
      {
      
        @Rule
        public JenkinsRule j = new JenkinsRule();
      
        /**
         * Setting "master" as label in UI works properly as expected.
         */
        @Test
        public void testMasterViaUI() throws Exception {
          FreeStyleProject project = j.createFreeStyleProject();
          setViaReflection(project, AbstractProject.class, "assignedNode", "master");
          project.save();
          assertEquals("master", project.getAssignedLabel().getName());
          assertEquals("master", project.getAssignedLabelString());
        }
      
        /**
         * Setting "master" as label via API results in a different, unexpected behavior.
         * project.getAssignedLabelString returns null. 
         */
        @Test
        public void testMasterViaAPI() throws IOException {
          Label label = Label.get("master");
          assertEquals("master", label.getName());
          FreeStyleProject project = j.createFreeStyleProject();
          project.save();
          project.setAssignedLabel(label);
          assertEquals("master", project.getAssignedLabel().getName());
          
          // value is null:
          assertEquals("master", project.getAssignedLabelString());
        }
      
        /**
         * Iff parsing works properly 
         */
        @Test
        public void testParseExpressionIff() throws IOException, ANTLRException {
          Label label = Label.parseExpression("a<->b");
          assertEquals(Iff.class, label.getClass());
          assertEquals("a<->b", label.getName());
          
          label = Label.parseExpression("a <-> b");
          assertEquals(Iff.class, label.getClass());
          assertEquals("a<->b", label.getName());
        }
        
        /**
         * Implies parsing fails with Exception. "a->b" cannot be parsed (see labelB).
         * On the other hand Implies.getName() returns exactly this String for a valid
         * instance. See labelA parsed from "a -> b" with blanks (which works).
         */
        @Test
        public void testParseExpressionImplies() throws IOException, ANTLRException {
          //This one works
          Label labelA = Label.parseExpression("a -> b");
          assertEquals(Implies.class, labelA.getClass());
          assertEquals("a->b", labelA.getName());
          
          //TokenStreamRecognitionException: line 1:3: unexpected char: '>'
          //at hudson.model.labels.LabelExpressionLexer.nextToken(LabelExpressionLexer.java:127)
          Label labelB = Label.parseExpression("a->b");
          
          assertEquals(Implies.class, labelB.getClass());
          assertEquals("a->b", labelB.getName());
          
        }
        
        /**
         * Setting the Implies expression without blanks does not work at all
         * (see {@link #testParseExpressionImplies}).
         * Setting it with blanks works, but after saving the job the result
         * is a LabelAtom instead of a LabelExpression!
         */
        @Test
        public void testImpliesViaAPI() throws IOException, ANTLRException {
          Label label = Label.parseExpression("a -> b");
          assertEquals(Implies.class, label.getClass());
          assertEquals("a->b", label.getName());
          FreeStyleProject project = j.createFreeStyleProject();
          project.setAssignedLabel(label);
          project.save();
      
          assertEquals("a->b", project.getAssignedLabel().getName());
          assertEquals("\"a->b\"", project.getAssignedLabelString());
      
          //AssertionError: expected:<class hudson.model.labels.LabelExpression$Implies> 
          //                 but was:<class hudson.model.labels.LabelAtom>
          assertEquals(Implies.class, project.getAssignedLabel().getClass());
        }
      
        /**
         * Setting the Implies expression in the UI (correctly) without blanks works, but the result
         * is a LabelAtom instead of a LabelExpression!
         */
        @Test
        public void testImpliesViaUI() throws Exception {
          FreeStyleProject project = j.createFreeStyleProject();
          setViaReflection(project, AbstractProject.class, "assignedNode", "a->b");
          project.save();
          
          assertEquals("a->b", project.getAssignedLabel().getName());
          assertEquals("\"a->b\"", project.getAssignedLabelString());
      
          //AssertionError: expected:<class hudson.model.labels.LabelExpression$Implies> 
          //                 but was:<class hudson.model.labels.LabelAtom>
          assertEquals(Implies.class, project.getAssignedLabel().getClass());
        }
      
        /**
         * Saving the Implies expression with blanks in the UI works properly.
         */
        @Test
        public void testImpliesViaUIWithBlanks() throws Exception {
          FreeStyleProject project = j.createFreeStyleProject();
          setViaReflection(project, AbstractProject.class, "assignedNode", "a -> b");
          project.save();
          
          assertEquals(Implies.class, project.getAssignedLabel().getClass());
          assertEquals("a->b", project.getAssignedLabel().getName());
          assertEquals("a -> b", project.getAssignedLabelString());
        }
      
        private void setViaReflection(Object object, Class<?> clazz, String fieldName, Object value) throws Exception
        {
          Field field = clazz.getDeclaredField(fieldName);
          field.setAccessible(true);
          field.set(object, value);
        }
        
      }
      

          [JENKINS-22975] Exception parsing "implies" expression - Label.parseExpression("a->b")

          Daniel Beck added a comment -

          See comments in linked issue, this was likely broken in 2010.

          Daniel Beck added a comment - See comments in linked issue, this was likely broken in 2010.

          I found this issue interesting so I set up a little repo to work on it, to reproduce the problem outside of the somewhat ‘heavy’ environment of the full jenkins source code. I basically copied the smallest amount of code (including the grammar) to observe the issue. It’s available there.
          Here’s what I can tell from the issue, complementing what’s already been found by Daniel.
          In labelExpr.g, 7 ‘operators’ (or 'special tokens') are defined: AND, OR, NOT, IMPLIES, IFF, LPAREN, RPAREN. They’re made of a combination of the following characters: &|!->(). These characters are all excluded from what’s allowed in a “LabelAtom” token, except -, which is the issue.
          When the lexer starts parsing a full label and starts a new token (for instance after a white space) with character ‘-‘, the token is either IMPLIES or ATOM. If the following character is >, the token is IMPLIES and should be complete (= a white space should follow). If it’s another character, the token must be an ATOM. Now if the lexer has already determined that it’s in an ATOM and finds a - followed by a >, it doesn’t understand that it should keep - and > together as an IMPLIES and backtrack before the - to close the ATOM it was reading. Instead it considers it’s an ATOM, and since > is not allowed in an ATOM the whole parsing fails.
          Even though I (believe I) understand the issue I don’t see yet how to solve it by modifying the grammar.
          One naive way to solve the problem is by forcefully looking for -> not preceded by space in the label, and add an extra space before feeding the parser/lexer. See function prepareLabelForParsing in TestGrammar.java.

          Dominique Brice added a comment - I found this issue interesting so I set up a little repo to work on it, to reproduce the problem outside of the somewhat ‘heavy’ environment of the full jenkins source code. I basically copied the smallest amount of code (including the grammar) to observe the issue. It’s available there . Here’s what I can tell from the issue, complementing what’s already been found by Daniel. In labelExpr.g, 7 ‘operators’ (or 'special tokens') are defined: AND, OR, NOT, IMPLIES, IFF, LPAREN, RPAREN. They’re made of a combination of the following characters: &|!->(). These characters are all excluded from what’s allowed in a “LabelAtom” token, except -, which is the issue. When the lexer starts parsing a full label and starts a new token (for instance after a white space) with character ‘-‘, the token is either IMPLIES or ATOM. If the following character is >, the token is IMPLIES and should be complete (= a white space should follow). If it’s another character, the token must be an ATOM. Now if the lexer has already determined that it’s in an ATOM and finds a - followed by a >, it doesn’t understand that it should keep - and > together as an IMPLIES and backtrack before the - to close the ATOM it was reading. Instead it considers it’s an ATOM, and since > is not allowed in an ATOM the whole parsing fails. Even though I (believe I) understand the issue I don’t see yet how to solve it by modifying the grammar. One naive way to solve the problem is by forcefully looking for -> not preceded by space in the label, and add an extra space before feeding the parser/lexer. See function prepareLabelForParsing in TestGrammar.java .

          Related side-effect: when you enter windows -> x64 (i.e. with spaces, so that the config field does not complain), then you get the usual "Label is serviced by 3 nodes" message underneath the expression field.

          However, clicking on this link leads to a URL with the canonical version of the expression (i.e. with spaces stripped), so the URL looks like /label/windows->x64/ — this silently fails and shows a page with zero nodes.

          Changing the URL to have spaces in the expression, i.e. /label/windows%20->%20x64/ works as expected, and the three matching nodes (in this case) are shown as expected.

          Christopher Orr added a comment - Related side-effect: when you enter windows -> x64 (i.e. with spaces, so that the config field does not complain), then you get the usual "Label is serviced by 3 nodes" message underneath the expression field. However, clicking on this link leads to a URL with the canonical version of the expression (i.e. with spaces stripped), so the URL looks like /label/windows->x64/ — this silently fails and shows a page with zero nodes. Changing the URL to have spaces in the expression, i.e. /label/windows%20->%20x64/ works as expected, and the three matching nodes (in this case) are shown as expected.

          Wadeck Follonier added a comment - https://github.com/jenkinsci/jenkins/pull/5722 should correct this behavior

            wfollonier Wadeck Follonier
            alexanderlink alexanderlink
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved: