From 7ddbcbbe9fcc2c1c2e9693807457ba339d813f19 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Tue, 3 Oct 2023 10:12:10 +0200 Subject: [PATCH 1/1] Eliminate HandlingPriority.CANNOT_HANDLE This is an ugly special case. It really is used only as a return from canHandle(), which in turn can use a @Nullable return to represent this. Change-Id: I87ce5f1a4391e04de78377c1d4d27b55e2fe80e2 Signed-off-by: Robert Varga --- .../server/mdsal/operations/RuntimeRpc.java | 2 +- .../mdsal/operations/RuntimeRpcTest.java | 12 ++-- .../operations/AbstractNetconfOperation.java | 5 +- .../api/operations/HandlingPriority.java | 55 ++----------------- .../api/operations/NetconfOperation.java | 12 ++-- .../osgi/NetconfOperationRouterImpl.java | 35 ++++++------ .../netconf/server/ConcurrentClientsTest.java | 13 ++--- .../AbstractNetconfOperationTest.java | 6 +- .../api/operations/HandlingPriorityTest.java | 13 ++--- 9 files changed, 49 insertions(+), 104 deletions(-) diff --git a/plugins/netconf-server-mdsal/src/main/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpc.java b/plugins/netconf-server-mdsal/src/main/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpc.java index db2b4fe2d1..2c5b8d6e0f 100644 --- a/plugins/netconf-server-mdsal/src/main/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpc.java +++ b/plugins/netconf-server-mdsal/src/main/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpc.java @@ -79,7 +79,7 @@ public final class RuntimeRpc extends AbstractSingletonNetconfOperation { .flatMap(module -> getRpcDefinitionFromModule(module, xmlNamespace, netconfOperationName)); if (rpcDef.isEmpty()) { LOG.debug("Cannot handle rpc: {}, {}", netconfOperationName, namespace); - return HandlingPriority.CANNOT_HANDLE; + return null; } return HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY; } diff --git a/plugins/netconf-server-mdsal/src/test/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpcTest.java b/plugins/netconf-server-mdsal/src/test/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpcTest.java index 905c26650f..501b005138 100644 --- a/plugins/netconf-server-mdsal/src/test/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpcTest.java +++ b/plugins/netconf-server-mdsal/src/test/java/org/opendaylight/netconf/server/mdsal/operations/RuntimeRpcTest.java @@ -8,13 +8,13 @@ package org.opendaylight.netconf.server.mdsal.operations; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; -import com.google.common.base.Preconditions; import com.google.common.io.CharSource; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -188,7 +188,7 @@ public class RuntimeRpcTest { final Document rpcDocument = XmlFileLoader.xmlFileToDocument("messages/mapping/rpcs/rpc-void-output.xml"); final HandlingPriority priority = rpc.canHandle(rpcDocument); - Preconditions.checkState(priority != HandlingPriority.CANNOT_HANDLE); + assertNotNull(priority); final Document response = rpc.handle(rpcDocument, NetconfOperationChainedExecution.EXECUTION_TERMINATION_POINT); @@ -202,7 +202,7 @@ public class RuntimeRpcTest { final Document rpcDocument = XmlFileLoader.xmlFileToDocument("messages/mapping/rpcs/rpc-nonvoid.xml"); final HandlingPriority priority = rpc.canHandle(rpcDocument); - Preconditions.checkState(priority != HandlingPriority.CANNOT_HANDLE); + assertNotNull(priority); final Document response = rpc.handle(rpcDocument, NetconfOperationChainedExecution.EXECUTION_TERMINATION_POINT); verifyResponse(response, XmlFileLoader.xmlFileToDocument("messages/mapping/rpcs/rpc-nonvoid-control.xml")); @@ -215,7 +215,7 @@ public class RuntimeRpcTest { final Document rpcDocument = XmlFileLoader.xmlFileToDocument("messages/mapping/rpcs/rpc-container.xml"); final HandlingPriority priority = rpc.canHandle(rpcDocument); - Preconditions.checkState(priority != HandlingPriority.CANNOT_HANDLE); + assertNotNull(priority); final Document response = rpc.handle(rpcDocument, NetconfOperationChainedExecution.EXECUTION_TERMINATION_POINT); verifyResponse(response, XmlFileLoader.xmlFileToDocument("messages/mapping/rpcs/rpc-container-control.xml")); @@ -228,7 +228,7 @@ public class RuntimeRpcTest { final Document rpcDocument = XmlFileLoader.xmlFileToDocument("messages/mapping/rpcs/rpc-nonvoid.xml"); final HandlingPriority priority = rpc.canHandle(rpcDocument); - Preconditions.checkState(priority != HandlingPriority.CANNOT_HANDLE); + assertNotNull(priority); final DocumentedException e = assertThrows(DocumentedException.class, () -> rpc.handle(rpcDocument, NetconfOperationChainedExecution.EXECUTION_TERMINATION_POINT)); @@ -244,7 +244,7 @@ public class RuntimeRpcTest { final Document rpcDocument = XmlFileLoader.xmlFileToDocument("messages/mapping/rpcs/rpc-void-input-output.xml"); final HandlingPriority priority = rpc.canHandle(rpcDocument); - Preconditions.checkState(priority != HandlingPriority.CANNOT_HANDLE); + assertNotNull(priority); final Document response = rpc.handle(rpcDocument, NetconfOperationChainedExecution.EXECUTION_TERMINATION_POINT); diff --git a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperation.java b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperation.java index 2d3c8b53fe..99f8311195 100644 --- a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperation.java +++ b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperation.java @@ -40,8 +40,7 @@ public abstract class AbstractNetconfOperation implements NetconfOperation { protected HandlingPriority canHandle(final String operationName, final String operationNamespace) { return operationName.equals(getOperationName()) && operationNamespace.equals(getOperationNamespace()) - ? getHandlingPriority() - : HandlingPriority.CANNOT_HANDLE; + ? getHandlingPriority() : null; } public static final class OperationNameAndNamespace { @@ -74,7 +73,7 @@ public abstract class AbstractNetconfOperation implements NetconfOperation { NamespaceURN.BASE); } - protected HandlingPriority getHandlingPriority() { + protected @NonNull HandlingPriority getHandlingPriority() { return HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY; } diff --git a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/HandlingPriority.java b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/HandlingPriority.java index aa076eb4d6..5fe4adbce7 100644 --- a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/HandlingPriority.java +++ b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/HandlingPriority.java @@ -8,67 +8,24 @@ package org.opendaylight.netconf.server.api.operations; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkState; -import com.google.common.base.MoreObjects; -import java.util.Objects; -import java.util.Optional; import org.eclipse.jdt.annotation.NonNull; -public final class HandlingPriority implements Comparable { - // FIXME: remote this constant - public static final HandlingPriority CANNOT_HANDLE = new HandlingPriority(null); - public static final HandlingPriority HANDLE_WITH_DEFAULT_PRIORITY = new HandlingPriority(Integer.MIN_VALUE); - public static final HandlingPriority HANDLE_WITH_MAX_PRIORITY = new HandlingPriority(Integer.MAX_VALUE); - - private final Integer priority; - - private HandlingPriority(final Integer priority) { - this.priority = priority; - } - - public static @NonNull HandlingPriority of(final int priority) { - return new HandlingPriority(priority); - } - - /** - * Get priority number. - * - * @return priority number or Optional.absent otherwise - */ - public Optional getPriority() { - return Optional.ofNullable(priority); - } +public record HandlingPriority(int priority) implements Comparable { + public static final @NonNull HandlingPriority HANDLE_WITH_DEFAULT_PRIORITY = + new HandlingPriority(Integer.MIN_VALUE); + public static final @NonNull HandlingPriority HANDLE_WITH_MAX_PRIORITY = new HandlingPriority(Integer.MAX_VALUE); public HandlingPriority increasePriority(final int priorityIncrease) { - checkState(priority != null, "Unable to increase priority for %s", this); checkArgument(priorityIncrease > 0, "Negative increase"); checkArgument(Long.valueOf(priority) + priorityIncrease < Integer.MAX_VALUE, "Resulting priority cannot be higher than %s", Integer.MAX_VALUE); - return of(priority + priorityIncrease); + return new HandlingPriority(priority + priorityIncrease); } @Override @SuppressWarnings("checkstyle:parameterName") public int compareTo(final HandlingPriority o) { - if (priority == null) { - return o.priority == null ? 0 : -1; - } - return o.priority == null ? 1 : Integer.compare(priority, o.priority); - } - - @Override - public boolean equals(final Object obj) { - return this == obj || obj instanceof HandlingPriority other && Objects.equals(priority, other.priority); - } - - @Override - public int hashCode() { - return Objects.hashCode(priority) ; - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this).add("priority", priority).toString(); + return Integer.compare(priority, o.priority); } } diff --git a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/NetconfOperation.java b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/NetconfOperation.java index 395a0c0524..c6f1af826b 100644 --- a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/NetconfOperation.java +++ b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/api/operations/NetconfOperation.java @@ -8,6 +8,7 @@ package org.opendaylight.netconf.server.api.operations; +import org.eclipse.jdt.annotation.Nullable; import org.opendaylight.netconf.api.DocumentedException; import org.w3c.dom.Document; @@ -24,16 +25,15 @@ import org.w3c.dom.Document; * the results. */ public interface NetconfOperation { - /** - * Singleton operations should return - * HandlingPriority.HANDLE_WITH_MAX_PRIORITY, last operations - * HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY. + * Singleton operations should return {@link HandlingPriority#HANDLE_WITH_MAX_PRIORITY}, last operations + * {@link HandlingPriority#HANDLE_WITH_DEFAULT_PRIORITY}. * * @param message request message - * @return {@code handling priority} + * @return A {@link HandlingPriority}, or {@code null} if the message is not handled + * @throws DocumentedException if the message is found to have structural errors */ - HandlingPriority canHandle(Document message) throws DocumentedException; + @Nullable HandlingPriority canHandle(Document message) throws DocumentedException; /** * Execute current netconf operation and trigger execution of subsequent diff --git a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/osgi/NetconfOperationRouterImpl.java b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/osgi/NetconfOperationRouterImpl.java index fd65b91396..6b644c8385 100644 --- a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/osgi/NetconfOperationRouterImpl.java +++ b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/osgi/NetconfOperationRouterImpl.java @@ -7,7 +7,6 @@ */ package org.opendaylight.netconf.server.osgi; -import static com.google.common.base.Preconditions.checkState; import static java.util.Objects.requireNonNull; import com.google.common.collect.ImmutableSet; @@ -148,23 +147,23 @@ public class NetconfOperationRouterImpl implements NetconfOperationRouter, AutoC private TreeMap getSortedNetconfOperationsWithCanHandle( final Document message, final NetconfServerSession session) throws DocumentedException { - final TreeMap sortedPriority = new TreeMap<>(); - - for (final NetconfOperation netconfOperation : allNetconfOperations) { - final HandlingPriority handlingPriority = netconfOperation.canHandle(message); - if (netconfOperation instanceof DefaultNetconfOperation defaultOperation) { - defaultOperation.setNetconfSession(session); - } - if (netconfOperation instanceof SessionAwareNetconfOperation sessionAwareOperation) { - sessionAwareOperation.setSession(session); - } - if (!handlingPriority.equals(HandlingPriority.CANNOT_HANDLE)) { - - checkState(!sortedPriority.containsKey(handlingPriority), - "Multiple %s available to handle message %s with priority %s, %s and %s", - NetconfOperation.class.getName(), message, handlingPriority, netconfOperation, sortedPriority - .get(handlingPriority)); - sortedPriority.put(handlingPriority, netconfOperation); + final var sortedPriority = new TreeMap(); + for (var netconfOperation : allNetconfOperations) { + final var handlingPriority = netconfOperation.canHandle(message); + if (handlingPriority != null) { + final var existing = sortedPriority.putIfAbsent(handlingPriority, netconfOperation); + if (existing != null) { + throw new IllegalStateException( + "Multiple %s available to handle message %s with priority %s, %s and %s".formatted( + NetconfOperation.class.getName(), message, handlingPriority, netconfOperation, existing)); + } + + if (netconfOperation instanceof DefaultNetconfOperation defaultOperation) { + defaultOperation.setNetconfSession(session); + } + if (netconfOperation instanceof SessionAwareNetconfOperation sessionAwareOperation) { + sessionAwareOperation.setSession(session); + } } } return sortedPriority; diff --git a/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/ConcurrentClientsTest.java b/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/ConcurrentClientsTest.java index 701bd82436..e978fe8d6d 100644 --- a/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/ConcurrentClientsTest.java +++ b/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/ConcurrentClientsTest.java @@ -212,7 +212,7 @@ public class ConcurrentClientsTest { } } - assertEquals(CONCURRENCY, testingNetconfOperation.getMessageCount()); + assertEquals(CONCURRENCY, testingNetconfOperation.counter.get()); } public static Set getOnlyExiServerCaps() { @@ -236,26 +236,21 @@ public class ConcurrentClientsTest { @Override public HandlingPriority canHandle(final Document message) { return XmlUtil.toString(message).contains(NetconfStartExiMessageProvider.START_EXI) - ? HandlingPriority.CANNOT_HANDLE : - HandlingPriority.HANDLE_WITH_MAX_PRIORITY; + ? null : HandlingPriority.HANDLE_WITH_MAX_PRIORITY; } @SuppressWarnings("checkstyle:IllegalCatch") @Override public Document handle(final Document requestMessage, final NetconfOperationChainedExecution subsequentOperation) throws DocumentedException { + LOG.info("Handling netconf message from test {}", XmlUtil.toString(requestMessage)); + counter.getAndIncrement(); try { - LOG.info("Handling netconf message from test {}", XmlUtil.toString(requestMessage)); - counter.getAndIncrement(); return XmlUtil.readXmlToDocument(""); } catch (Exception e) { throw new RuntimeException(e); } } - - public long getMessageCount() { - return counter.get(); - } } /** diff --git a/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperationTest.java b/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperationTest.java index 2933ad4774..490f93e9c4 100644 --- a/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperationTest.java +++ b/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/AbstractNetconfOperationTest.java @@ -8,7 +8,6 @@ package org.opendaylight.netconf.server.api.operations; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; @@ -35,7 +34,7 @@ public class AbstractNetconfOperationTest { @Override protected String getOperationName() { - return null; + return "edit-config"; } @Override @@ -62,8 +61,7 @@ public class AbstractNetconfOperationTest { public void testAbstractNetconfOperation() throws Exception { Document helloMessage = XmlFileLoader.xmlFileToDocument("netconfMessages/edit_config.xml"); assertEquals(new SessionIdType(Uint32.ONE), netconfOperation.sessionId()); - assertNotNull(netconfOperation.canHandle(helloMessage)); - assertEquals(HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY, netconfOperation.getHandlingPriority()); + assertEquals(HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY, netconfOperation.canHandle(helloMessage)); netconfOperation.handle(helloMessage, operation); assertTrue(netconfOperation.handleRun); diff --git a/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/HandlingPriorityTest.java b/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/HandlingPriorityTest.java index 15fe60ac62..ac75de18c5 100644 --- a/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/HandlingPriorityTest.java +++ b/protocol/netconf-server/src/test/java/org/opendaylight/netconf/server/api/operations/HandlingPriorityTest.java @@ -18,23 +18,20 @@ public class HandlingPriorityTest { public void testHandlingPriority() { assertEquals(0, HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY.compareTo(HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY)); - assertEquals(-1, HandlingPriority.CANNOT_HANDLE.compareTo(HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY)); - assertEquals(1, HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY.compareTo(HandlingPriority.CANNOT_HANDLE)); assertEquals(-1, HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY.compareTo(HandlingPriority.HANDLE_WITH_MAX_PRIORITY)); assertEquals(1, HandlingPriority.HANDLE_WITH_MAX_PRIORITY.compareTo(HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY)); - assertEquals(0, HandlingPriority.of(Integer.MIN_VALUE) + assertEquals(0, new HandlingPriority(Integer.MIN_VALUE) .compareTo(HandlingPriority.HANDLE_WITH_DEFAULT_PRIORITY)); - HandlingPriority prio = HandlingPriority.of(10); - assertTrue(prio.increasePriority(1).compareTo(HandlingPriority.of(11)) == 0); + HandlingPriority prio = new HandlingPriority(10); + assertTrue(prio.increasePriority(1).compareTo(new HandlingPriority(11)) == 0); - assertFalse(HandlingPriority.CANNOT_HANDLE.getPriority().isPresent()); assertFalse(HandlingPriority.HANDLE_WITH_MAX_PRIORITY.equals(new Object())); - assertEquals(HandlingPriority.HANDLE_WITH_MAX_PRIORITY, HandlingPriority.of(Integer.MAX_VALUE)); + assertEquals(HandlingPriority.HANDLE_WITH_MAX_PRIORITY, new HandlingPriority(Integer.MAX_VALUE)); assertEquals(HandlingPriority.HANDLE_WITH_MAX_PRIORITY.hashCode(), - HandlingPriority.of(Integer.MAX_VALUE).hashCode()); + new HandlingPriority(Integer.MAX_VALUE).hashCode()); } } -- 2.36.6