Use correct key-arg splitting

The splitter we have operates on spaces and trims results to get
rid of whitespace -- probably as a consequence of us historically
having bugs there.

We currently break if someone uses double-quoted whitespace trimming,
as we do not treat '\n' as a separator and then attempt to interpret
it as a node-identifier.

Add an explicit SEP equivalent CharMatcher and use a Splitter on that,
resulting in '\t' and '\n' being correctly trimmed.

JIRA: YANGTOOLS-1200
Change-Id: Ifa3085fffcbbe24204e9d6c0d86ed8c41bd61065
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 5fe0d9e2a63cc9cf3be8c39f32305cf9e2a8be4e)

diff --git a/yang/yang-parser-antlr/src/main/antlr4/org/opendaylight/yangtools/yang/parser/antlr/YangStatementLexer.g4 b/yang/yang-parser-antlr/src/main/antlr4/org/opendaylight/yangtools/yang/parser/antlr/YangStatementLexer.g4
index 58f7807e794077607eef3feecab8056cdefed5f2..94ebabc6ed73a2be22092315d730bf3749205c70 100644 (file)
--- a/yang/yang-parser-antlr/src/main/antlr4/org/opendaylight/yangtools/yang/parser/antlr/YangStatementLexer.g4
+++ b/yang/yang-parser-antlr/src/main/antlr4/org/opendaylight/yangtools/yang/parser/antlr/YangStatementLexer.g4
@@ -47,6 +47,7 @@ PLUS : '+' -> type(PLUS);
 LINE_COMMENT : '//' .*? '\r'? ('\n' | EOF) -> skip;
 BLOCK_COMMENT : '/*' .*? '*/' -> skip;
 
+// Note: if this changes, also update KeyStatementSupport's Splitter
 SEP: [ \n\r\t]+ -> type(SEP);
 
 // Special-cased identifier string

diff --git a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/key/KeyStatementSupport.java b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/key/KeyStatementSupport.java
index 3383496cfd8a65332c14bd0f46a4b9cb56272a7c..76af29a27e52d76ecc90aafbaf2f2ba8a9a78fde 100644 (file)
--- a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/key/KeyStatementSupport.java
+++ b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/key/KeyStatementSupport.java
@@ -9,6 +9,7 @@ package org.opendaylight.yangtools.yang.parser.rfc7950.stmt.key;
 
 import static com.google.common.base.Verify.verify;
 
+import com.google.common.base.CharMatcher;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -22,6 +23,7 @@ import org.opendaylight.yangtools.yang.model.api.meta.DeclaredStatement;
 import org.opendaylight.yangtools.yang.model.api.meta.EffectiveStatement;
 import org.opendaylight.yangtools.yang.model.api.stmt.KeyEffectiveStatement;
 import org.opendaylight.yangtools.yang.model.api.stmt.KeyStatement;
+import org.opendaylight.yangtools.yang.parser.antlr.YangStatementLexer;
 import org.opendaylight.yangtools.yang.parser.rfc7950.stmt.BaseStatementSupport;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContextUtils;
@@ -30,7 +32,27 @@ import org.opendaylight.yangtools.yang.parser.spi.source.SourceException;
 
 public final class KeyStatementSupport
         extends BaseStatementSupport<Set<QName>, KeyStatement, KeyEffectiveStatement> {
-    private static final Splitter LIST_KEY_SPLITTER = Splitter.on(' ').omitEmptyStrings().trimResults();
+    /**
+     * This is equivalent to {@link YangStatementLexer#SEP}'s definition. Currently equivalent to the non-repeating
+     * part of:
+     *
+     * <p>
+     * {@code SEP: [ \n\r\t]+ -> type(SEP);}.
+     */
+    private static final CharMatcher SEP = CharMatcher.anyOf(" \n\r\t").precomputed();
+
+    /**
+     * Splitter corresponding to {@code key-arg} ABNF as defined
+     * in <a href="https://tools.ietf.org/html/rfc6020#section-12">RFC6020, section 12</a>:
+     *
+     * <p>
+     * {@code key-arg             = node-identifier *(sep node-identifier)}
+     *
+     * <p>
+     * We also account for {@link #SEP} not handling repetition by ignoring empty strings.
+     */
+    private static final Splitter KEY_ARG_SPLITTER = Splitter.on(SEP).omitEmptyStrings();
+
     private static final SubstatementValidator SUBSTATEMENT_VALIDATOR = SubstatementValidator.builder(
         YangStmtMapping.KEY)
         .build();
@@ -48,7 +70,7 @@ public final class KeyStatementSupport
     public ImmutableSet<QName> parseArgumentValue(final StmtContext<?, ?, ?> ctx, final String value) {
         final Builder<QName> builder = ImmutableSet.builder();
         int tokens = 0;
-        for (String keyToken : LIST_KEY_SPLITTER.split(value)) {
+        for (String keyToken : KEY_ARG_SPLITTER.split(value)) {
             builder.add(StmtContextUtils.parseNodeIdentifier(ctx, keyToken));
             tokens++;
         }

diff --git a/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT1200Test.java b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT1200Test.java
new file mode 100644 (file)
index 0000000..5ae4351
--- /dev/null
+++ b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT1200Test.java
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2020 PANTHEON.tech, s.r.o. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.yangtools.yang.stmt;
+
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+
+import java.util.List;
+import org.junit.Test;
+import org.opendaylight.yangtools.yang.common.QName;
+import org.opendaylight.yangtools.yang.model.api.DataSchemaNode;
+import org.opendaylight.yangtools.yang.model.api.ListSchemaNode;
+
+public class YT1200Test {
+    private static final QName FOO = QName.create("urn:foo", "foo");
+
+    @Test
+    public void testKeyParsing() throws Exception {
+        final DataSchemaNode foo = StmtTestUtils.parseYangSource("/bugs/YT1200/foo.yang").getDataChildByName(FOO);
+        assertThat(foo, instanceOf(ListSchemaNode.class));
+        assertEquals(List.of(FOO, QName.create(FOO, "bar"), QName.create(FOO, "baz")),
+            ((ListSchemaNode) foo).getKeyDefinition());
+    }
+}

diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT1200/foo.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT1200/foo.yang
new file mode 100644 (file)
index 0000000..16ff1ad
--- /dev/null
+++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT1200/foo.yang
@@ -0,0 +1,20 @@
+module foo {
+  namespace urn:foo;
+  prefix foo;
+
+  list foo {
+    key "foo \t\nbar
+         baz";
+    leaf foo {
+      type string;
+    }
+
+    leaf bar {
+      type string;
+    }
+
+    leaf baz {
+      type string;
+    }
+  }
+}