From 71ecea8aceef93a58c8eeb95b09955bd0155d75b Mon Sep 17 00:00:00 2001 From: =?utf8?q?Peter=20Pu=C5=A1k=C3=A1r?= Date: Thu, 20 Jan 2022 09:54:43 +0100 Subject: [PATCH] Add missing identifier for list node in fields RESTCONF query MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Robert Varga --- .../utils/parser/NetconfFieldsTranslator.java | 23 +++++- .../parser/AbstractFieldsTranslatorTest.java | 9 +++ .../parser/NetconfFieldsTranslatorTest.java | 74 ++++++++++++++----- .../parser/WriterFieldsTranslatorTest.java | 29 ++++++-- 4 files changed, 104 insertions(+), 31 deletions(-) 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 ece8bc6365..1cbe5cd7ce 100644 --- 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 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 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.
* - 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 9c40db25aa..57a287786d 100644 --- 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 { assertLeafList(result); } + @Test + public void testKeyedList() { + final var result = translateFields(identifierJukebox, assertFields("library/artist(name)")); + assertNotNull(result); + assertKeyedList(result); + } + + protected abstract void assertKeyedList(List result); + protected abstract void assertLeafList(@NonNull List 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 f20d07bb5c..bae3814ccd 100644 --- 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 result) { + assertEquals(1, result.size()); + } + @Override protected void assertDoublePath(final List result) { assertEquals(2, result.size()); @@ -55,23 +60,35 @@ public class NetconfFieldsTranslatorTest extends AbstractFieldsTranslatorTest 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 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 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 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 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 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 467383ea0f..41dac3c34b 100644 --- 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> 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> 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> 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> 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> 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> 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> 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> 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> result) { - // FIXME: add assertions + assertEquals(1, result.size()); + assertEquals(Set.of(PROTOCOLS_Q_NAME), result.get(0)); } } -- 2.36.6