Add missing identifier for list node in fields RESTCONF query

Fields filtering is currently completely failing for NETCONF nodes
when the fields query contains any list node, because we need both
NodeIdentifier (for the entire list) and NodeIdentifierWithPredicates
(for a concrete entry) to parse them correctly.

WriterFieldsTranslator provides both already and this is correctly
working on non-NETCONF nodes. Make this behaviour consistent for both.

JIRA: NETCONF-820
Change-Id: I24d6ec1752ba1a2f551fcd4149c6efde04f1a89c
Signed-off-by: Peter Puškár <ppuskar@frinx.io>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslator.java b/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslator.java
index ece8bc6365a8f388bd41f85070ab385c69c9a9a7..1cbe5cd7ce1ed22eb5806636df27eba73d651e0b 100644 (file)
--- a/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslator.java
+++ b/restconf/restconf-nb/src/main/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslator.java
@@ -120,12 +120,18 @@ public final class NetconfFieldsTranslator extends AbstractFieldsTranslator<Netc
         final List<PathArgument> collectedMixinNodes = new ArrayList<>();
 
         DataSchemaContextNode<?> actualContextNode = currentNode.getChild(childQName);
+        if (actualContextNode == null) {
+            actualContextNode = resolveMixinNode(currentNode, currentNode.getIdentifier().getNodeType());
+            actualContextNode = actualContextNode.getChild(childQName);
+        }
+
         while (actualContextNode != null && actualContextNode.isMixin()) {
-            if (actualContextNode.getDataSchemaNode() instanceof ListSchemaNode) {
-                // we need just a single node identifier from list in the path (key is not available)
+            final var actualDataSchemaNode = actualContextNode.getDataSchemaNode();
+            if (actualDataSchemaNode instanceof ListSchemaNode listSchema && listSchema.getKeyDefinition().isEmpty()) {
+                // we need just a single node identifier from list in the path IFF it is an unkeyed list, otherwise
+                // we need both (which is the default case)
                 actualContextNode = actualContextNode.getChild(childQName);
-                break;
-            } else if (actualContextNode.getDataSchemaNode() instanceof LeafListSchemaNode) {
+            } else if (actualDataSchemaNode instanceof LeafListSchemaNode) {
                 // NodeWithValue is unusable - stop parsing
                 break;
             } else {
@@ -145,6 +151,15 @@ public final class NetconfFieldsTranslator extends AbstractFieldsTranslator<Netc
         return actualContextNode;
     }
 
+    private static DataSchemaContextNode<?> resolveMixinNode(
+            final DataSchemaContextNode<?> node, final @NonNull QName qualifiedName) {
+        DataSchemaContextNode<?> currentNode = node;
+        while (currentNode != null && currentNode.isMixin()) {
+            currentNode = currentNode.getChild(qualifiedName);
+        }
+        return currentNode;
+    }
+
     /**
      * {@link PathArgument} of data element grouped with identifiers of leading mixin nodes and previous node.<br>
      *  - identifiers of mixin nodes on the path to the target node - required for construction of full valid

diff --git a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/AbstractFieldsTranslatorTest.java b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/AbstractFieldsTranslatorTest.java
index 9c40db25aa18a73bfa706d578e25fa47ffa9d523..57a287786d7014e5b346daecf47bb3e9d30ffdd3 100644 (file)
--- a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/AbstractFieldsTranslatorTest.java
+++ b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/AbstractFieldsTranslatorTest.java
@@ -257,6 +257,15 @@ public abstract class AbstractFieldsTranslatorTest<T> {
         assertLeafList(result);
     }
 
+    @Test
+    public void testKeyedList() {
+        final var result = translateFields(identifierJukebox, assertFields("library/artist(name)"));
+        assertNotNull(result);
+        assertKeyedList(result);
+    }
+
+    protected abstract void assertKeyedList(List<T> result);
+
     protected abstract void assertLeafList(@NonNull List<T> result);
 
     /**

diff --git a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslatorTest.java b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslatorTest.java
index f20d07bb5caa516c8636e1271e11fedbf221e33f..bae3814ccd75527fc147e3fe12e3770f4a46ef9e 100644 (file)
--- a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslatorTest.java
+++ b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/NetconfFieldsTranslatorTest.java
@@ -42,6 +42,11 @@ public class NetconfFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Ya
         assertEquals(LIBRARY_Q_NAME, pathArguments.get(0).getNodeType());
     }
 
+    @Override
+    protected void assertKeyedList(List<YangInstanceIdentifier> result) {
+        assertEquals(1, result.size());
+    }
+
     @Override
     protected void assertDoublePath(final List<YangInstanceIdentifier> result) {
         assertEquals(2, result.size());
@@ -55,23 +60,35 @@ public class NetconfFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Ya
 
     @Override
     protected void assertSubPath(final List<YangInstanceIdentifier> result) {
-        // FIXME: NETCONF-820: add assertions
+        assertEquals(1, result.size());
+        final var pathArguments = result.get(0).getPathArguments();
+        assertEquals(6, pathArguments.size());
+        assertEquals(LIBRARY_Q_NAME, pathArguments.get(0).getNodeType());
+        assertEquals(ARTIST_Q_NAME, pathArguments.get(1).getNodeType());
+        assertEquals(ARTIST_Q_NAME, pathArguments.get(2).getNodeType());
+        assertEquals(ALBUM_Q_NAME, pathArguments.get(3).getNodeType());
+        assertEquals(ALBUM_Q_NAME, pathArguments.get(4).getNodeType());
+        assertEquals(NAME_Q_NAME, pathArguments.get(5).getNodeType());
     }
 
     @Override
     protected void assertChildrenPath(final List<YangInstanceIdentifier> result) {
         assertEquals(1, result.size());
         final var pathArguments = result.get(0).getPathArguments();
-        assertEquals(4, pathArguments.size());
+        assertEquals(6, pathArguments.size());
         assertEquals(LIBRARY_Q_NAME, pathArguments.get(0).getNodeType());
         assertEquals(ARTIST_Q_NAME, pathArguments.get(1).getNodeType());
-        assertEquals(ALBUM_Q_NAME, pathArguments.get(2).getNodeType());
-        assertEquals(NAME_Q_NAME, pathArguments.get(3).getNodeType());
+        assertEquals(ARTIST_Q_NAME, pathArguments.get(2).getNodeType());
+        assertEquals(ALBUM_Q_NAME, pathArguments.get(3).getNodeType());
+        assertEquals(ALBUM_Q_NAME, pathArguments.get(4).getNodeType());
+        assertEquals(NAME_Q_NAME, pathArguments.get(5).getNodeType());
     }
 
     @Override
     protected void assertNamespace(final List<YangInstanceIdentifier> result) {
-        // FIXME: add assertions
+        assertEquals(1, result.size());
+        final var augmentedLibraryPath = assertPath(result, AUGMENTED_LIBRARY_Q_NAME);
+        assertEquals(2, augmentedLibraryPath.getPathArguments().size());
     }
 
     @Override
@@ -79,23 +96,41 @@ public class NetconfFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Ya
         assertEquals(3, result.size());
 
         final var tosPath = assertPath(result, TYPE_OF_SERVICE_Q_NAME);
-        assertEquals(2, tosPath.getPathArguments().size());
+        assertEquals(3, tosPath.getPathArguments().size());
 
         final var instanceNamePath = assertPath(result, INSTANCE_NAME_Q_NAME);
-        assertEquals(3, instanceNamePath.getPathArguments().size());
+        assertEquals(5, instanceNamePath.getPathArguments().size());
 
         final var providerPath = assertPath(result, PROVIDER_Q_NAME);
-        assertEquals(3, providerPath.getPathArguments().size());
+        assertEquals(5, providerPath.getPathArguments().size());
     }
 
     @Override
     protected void assertMultipleChildren2(final List<YangInstanceIdentifier> result) {
-        // FIXME: add assertions
+        assertEquals(3, result.size());
+
+        final var tosPath = assertPath(result, TYPE_OF_SERVICE_Q_NAME);
+        assertEquals(3, tosPath.getPathArguments().size());
+
+        final var instanceNamePath = assertPath(result, INSTANCE_NAME_Q_NAME);
+        assertEquals(5, instanceNamePath.getPathArguments().size());
+
+        final var providerPath = assertPath(result, PROVIDER_Q_NAME);
+        assertEquals(5, providerPath.getPathArguments().size());
     }
 
     @Override
     protected void assertMultipleChildren3(final List<YangInstanceIdentifier> result) {
-        // FIXME: add assertions
+        assertEquals(3, result.size());
+
+        final var instanceNamePath = assertPath(result, INSTANCE_NAME_Q_NAME);
+        assertEquals(5, instanceNamePath.getPathArguments().size());
+
+        final var tosPath = assertPath(result, TYPE_OF_SERVICE_Q_NAME);
+        assertEquals(3, tosPath.getPathArguments().size());
+
+        final var nextServicePath = assertPath(result, NEXT_SERVICE_Q_NAME);
+        assertEquals(4, nextServicePath.getPathArguments().size());
     }
 
     @Override
@@ -103,16 +138,16 @@ public class NetconfFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Ya
         assertEquals(4, result.size());
 
         final var instanceNamePath = assertPath(result, INSTANCE_NAME_Q_NAME);
-        assertEquals(3, instanceNamePath.getPathArguments().size());
+        assertEquals(5, instanceNamePath.getPathArguments().size());
 
         final var tosPath = assertPath(result, TYPE_OF_SERVICE_Q_NAME);
-        assertEquals(2, tosPath.getPathArguments().size());
+        assertEquals(3, tosPath.getPathArguments().size());
 
         final var nextServicePath = assertPath(result, NEXT_SERVICE_Q_NAME);
-        assertEquals(3, nextServicePath.getPathArguments().size());
+        assertEquals(4, nextServicePath.getPathArguments().size());
 
         final var providerPath = assertPath(result, PROVIDER_Q_NAME);
-        assertEquals(3, providerPath.getPathArguments().size());
+        assertEquals(5, providerPath.getPathArguments().size());
     }
 
     @Override
@@ -120,16 +155,16 @@ public class NetconfFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Ya
         assertEquals(4, result.size());
 
         final var instanceNamePath = assertPath(result, INSTANCE_NAME_Q_NAME);
-        assertEquals(3, instanceNamePath.getPathArguments().size());
+        assertEquals(5, instanceNamePath.getPathArguments().size());
 
         final var tosPath = assertPath(result, TYPE_OF_SERVICE_Q_NAME);
-        assertEquals(2, tosPath.getPathArguments().size());
+        assertEquals(3, tosPath.getPathArguments().size());
 
         final var nextServicePath = assertPath(result, NEXT_SERVICE_Q_NAME);
-        assertEquals(3, nextServicePath.getPathArguments().size());
+        assertEquals(4, nextServicePath.getPathArguments().size());
 
         final var providerPath = assertPath(result, PROVIDER_Q_NAME);
-        assertEquals(3, providerPath.getPathArguments().size());
+        assertEquals(5, providerPath.getPathArguments().size());
     }
 
     @Override
@@ -147,8 +182,9 @@ public class NetconfFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Ya
     protected void assertListFieldUnderList(final List<YangInstanceIdentifier> result) {
         assertEquals(1, result.size());
         assertEquals(List.of(
-            // FIXME: this does not look right: where are the corresponding NodeIdentifiers?
+            NodeIdentifier.create(SERVICES_Q_NAME),
             NodeIdentifierWithPredicates.of(SERVICES_Q_NAME),
+            NodeIdentifier.create(INSTANCE_Q_NAME),
             NodeIdentifierWithPredicates.of(INSTANCE_Q_NAME)),
             result.get(0).getPathArguments());
     }

diff --git a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/WriterFieldsTranslatorTest.java b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/WriterFieldsTranslatorTest.java
index 467383ea0fb9967a0406b6c5b6c7cb06a768a6da..41dac3c34bebc3e798a5cb261f6f8875596e2587 100644 (file)
--- a/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/WriterFieldsTranslatorTest.java
+++ b/restconf/restconf-nb/src/test/java/org/opendaylight/restconf/nb/rfc8040/utils/parser/WriterFieldsTranslatorTest.java
@@ -66,7 +66,7 @@ public class WriterFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Set
 
     @Override
     protected void assertMultipleChildren1(final List<Set<QName>> result) {
-        assertEquals(result.size(), 3);
+        assertEquals(3, result.size());
         assertEquals(Set.of(SERVICES_Q_NAME), result.get(0));
         assertEquals(Set.of(TYPE_OF_SERVICE_Q_NAME, INSTANCE_Q_NAME), result.get(1));
         assertEquals(Set.of(INSTANCE_NAME_Q_NAME, PROVIDER_Q_NAME), result.get(2));
@@ -74,7 +74,7 @@ public class WriterFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Set
 
     @Override
     protected void assertMultipleChildren2(final List<Set<QName>> result) {
-        assertEquals(result.size(), 3);
+        assertEquals(3, result.size());
         assertEquals(Set.of(SERVICES_Q_NAME), result.get(0));
         assertEquals(Set.of(TYPE_OF_SERVICE_Q_NAME, INSTANCE_Q_NAME), result.get(1));
         assertEquals(Set.of(INSTANCE_NAME_Q_NAME, PROVIDER_Q_NAME), result.get(2));
@@ -82,7 +82,7 @@ public class WriterFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Set
 
     @Override
     protected void assertMultipleChildren3(final List<Set<QName>> result) {
-        assertEquals(result.size(), 3);
+        assertEquals(3, result.size());
         assertEquals(Set.of(SERVICES_Q_NAME), result.get(0));
         assertEquals(Set.of(TYPE_OF_SERVICE_Q_NAME, INSTANCE_Q_NAME, NEXT_DATA_Q_NAME), result.get(1));
         assertEquals(Set.of(INSTANCE_NAME_Q_NAME, NEXT_SERVICE_Q_NAME), result.get(2));
@@ -90,7 +90,7 @@ public class WriterFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Set
 
     @Override
     protected void assertMultipleChildren4(final List<Set<QName>> result) {
-        assertEquals(result.size(), 3);
+        assertEquals(3, result.size());
         assertEquals(Set.of(SERVICES_Q_NAME), result.get(0));
         assertEquals(Set.of(TYPE_OF_SERVICE_Q_NAME, INSTANCE_Q_NAME, NEXT_DATA_Q_NAME), result.get(1));
         assertEquals(Set.of(INSTANCE_NAME_Q_NAME, PROVIDER_Q_NAME, NEXT_SERVICE_Q_NAME), result.get(2));
@@ -98,7 +98,7 @@ public class WriterFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Set
 
     @Override
     protected void assertMultipleChildren5(final List<Set<QName>> result) {
-        assertEquals(result.size(), 3);
+        assertEquals(3, result.size());
         assertEquals(Set.of(SERVICES_Q_NAME), result.get(0));
         assertEquals(Set.of(TYPE_OF_SERVICE_Q_NAME, INSTANCE_Q_NAME, NEXT_DATA_Q_NAME), result.get(1));
         assertEquals(Set.of(INSTANCE_NAME_Q_NAME, PROVIDER_Q_NAME, NEXT_SERVICE_Q_NAME), result.get(2));
@@ -106,16 +106,29 @@ public class WriterFieldsTranslatorTest extends AbstractFieldsTranslatorTest<Set
 
     @Override
     protected void assertAugmentedChild(final List<Set<QName>> result) {
-        // FIXME: add assertions
+        assertEquals(2, result.size());
+        assertEquals(Set.of(PLAYER_Q_NAME), result.get(0));
+        assertEquals(Set.of(SPEED_Q_NAME), result.get(1));
     }
 
     @Override
     protected void assertListFieldUnderList(final List<Set<QName>> result) {
-        // FIXME: add assertions
+        assertEquals(2, result.size());
+        assertEquals(Set.of(SERVICES_Q_NAME), result.get(0));
+        assertEquals(Set.of(INSTANCE_Q_NAME), result.get(1));
+    }
+
+    @Override
+    protected void assertKeyedList(final List<Set<QName>> result) {
+        assertEquals(3, result.size());
+        assertEquals(Set.of(LIBRARY_Q_NAME), result.get(0));
+        assertEquals(Set.of(ARTIST_Q_NAME), result.get(1));
+        assertEquals(Set.of(NAME_Q_NAME), result.get(2));
     }
 
     @Override
     protected void assertLeafList(final List<Set<QName>> result) {
-        // FIXME: add assertions
+        assertEquals(1, result.size());
+        assertEquals(Set.of(PROTOCOLS_Q_NAME), result.get(0));
     }
 }