From: Robert Varga Date: Thu, 2 Jun 2022 18:24:23 +0000 (+0200) Subject: AbstractNetconfSessionNegotiator uses only NetconfHelloMessage X-Git-Tag: v4.0.0~45 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=60a15ce528aa431e60e14e6f414486e35fcdb1c6;p=netconf.git AbstractNetconfSessionNegotiator uses only NetconfHelloMessage We have a bit over-zealous use of NetconfSessionPreferences. We really just need the hello message for negotiation -- reducing the need to have a generic parameter, which is only causing issues with SpotBugs. JIRA: NETCONF-590 Change-Id: I7437fda181a447fe740aeed514c0eb3a779002e8 Signed-off-by: Robert Varga --- diff --git a/netconf/netconf-client/src/main/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiator.java b/netconf/netconf-client/src/main/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiator.java index 5ee52bd073..bbd17ce7ba 100644 --- a/netconf/netconf-client/src/main/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiator.java +++ b/netconf/netconf-client/src/main/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiator.java @@ -11,7 +11,6 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Interner; import com.google.common.collect.Interners; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.netty.channel.Channel; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; @@ -39,8 +38,7 @@ import org.w3c.dom.NodeList; // Non-final for mocking class NetconfClientSessionNegotiator - extends AbstractNetconfSessionNegotiator { + extends AbstractNetconfSessionNegotiator { private static final Logger LOG = LoggerFactory.getLogger(NetconfClientSessionNegotiator.class); private static final XPathExpression SESSION_ID_X_PATH = XMLNetconfUtil @@ -53,15 +51,16 @@ class NetconfClientSessionNegotiator private static final Interner> INTERNER = Interners.newWeakInterner(); + private final NetconfMessage startExi; + NetconfClientSessionNegotiator(final NetconfClientSessionPreferences sessionPreferences, final Promise promise, final Channel channel, final Timer timer, final NetconfClientSessionListener sessionListener, final long connectionTimeoutMillis) { - super(sessionPreferences, promise, channel, timer, sessionListener, connectionTimeoutMillis); + super(sessionPreferences.getHelloMessage(), promise, channel, timer, sessionListener, connectionTimeoutMillis); + startExi = sessionPreferences.getStartExiMessage(); } @SuppressWarnings("checkstyle:IllegalCatch") - @SuppressFBWarnings(value = "BC_UNCONFIRMED_CAST", - justification = "SpotBugs does not understand generic cast of sessionPreferences") @Override protected void handleMessage(final NetconfHelloMessage netconfMessage) throws NetconfDocumentedException { if (!ifNegotiatedAlready()) { @@ -79,10 +78,9 @@ class NetconfClientSessionNegotiator // If exi should be used, try to initiate exi communication // Call negotiationSuccessFul after exi negotiation is finished successfully or not - final NetconfMessage startExiMessage = sessionPreferences.getStartExiMessage(); - if (shouldUseExi(netconfMessage) && startExiMessage instanceof NetconfStartExiMessage) { + if (startExi instanceof NetconfStartExiMessage && shouldUseExi(netconfMessage)) { LOG.debug("Netconf session {} should use exi.", session); - tryToInitiateExi(session, (NetconfStartExiMessage) startExiMessage); + tryToInitiateExi(session, (NetconfStartExiMessage) startExi); } else { // Exi is not supported, release session immediately LOG.debug("Netconf session {} isn't capable of using exi.", session); @@ -111,11 +109,8 @@ class NetconfClientSessionNegotiator }); } - @SuppressFBWarnings(value = "BC_UNCONFIRMED_CAST", - justification = "SpotBugs does not understand generic cast of sessionPreferences") private boolean shouldUseExi(final NetconfHelloMessage helloMsg) { - return containsExi10Capability(helloMsg.getDocument()) - && containsExi10Capability(sessionPreferences.getHelloMessage().getDocument()); + return containsExi10Capability(helloMsg.getDocument()) && containsExi10Capability(localHello().getDocument()); } private static boolean containsExi10Capability(final Document doc) { diff --git a/netconf/netconf-impl/src/main/java/org/opendaylight/netconf/impl/NetconfServerSessionNegotiator.java b/netconf/netconf-impl/src/main/java/org/opendaylight/netconf/impl/NetconfServerSessionNegotiator.java index b1f60731a1..071c75f51c 100644 --- a/netconf/netconf-impl/src/main/java/org/opendaylight/netconf/impl/NetconfServerSessionNegotiator.java +++ b/netconf/netconf-impl/src/main/java/org/opendaylight/netconf/impl/NetconfServerSessionNegotiator.java @@ -7,7 +7,6 @@ */ package org.opendaylight.netconf.impl; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.netty.channel.Channel; import io.netty.channel.local.LocalAddress; import io.netty.util.Timer; @@ -25,15 +24,17 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; public final class NetconfServerSessionNegotiator - extends AbstractNetconfSessionNegotiator { + extends AbstractNetconfSessionNegotiator { private static final Logger LOG = LoggerFactory.getLogger(NetconfServerSessionNegotiator.class); private static final String UNKNOWN = "unknown"; + private final long sessionId; + protected NetconfServerSessionNegotiator(final NetconfServerSessionPreferences sessionPreferences, final Promise promise, final Channel channel, final Timer timer, final NetconfServerSessionListener sessionListener, final long connectionTimeoutMillis) { - super(sessionPreferences, promise, channel, timer, sessionListener, connectionTimeoutMillis); + super(sessionPreferences.getHelloMessage(), promise, channel, timer, sessionListener, connectionTimeoutMillis); + sessionId = sessionPreferences.getSessionId(); } @Override @@ -45,8 +46,6 @@ public final class NetconfServerSessionNegotiator } @Override - @SuppressFBWarnings(value = "BC_UNCONFIRMED_CAST_OF_RETURN_VALUE", - justification = "SpotBugs does not grok generic return of getSessionPreferences()") protected NetconfServerSession getSession(final NetconfServerSessionListener sessionListener, final Channel channel, final NetconfHelloMessage message) { final var additionalHeader = message.getAdditionalHeader(); @@ -57,7 +56,7 @@ public final class NetconfServerSessionNegotiator }); LOG.debug("Additional header from hello parsed as {} from {}", parsedHeader, additionalHeader); - return new NetconfServerSession(sessionListener, channel, getSessionPreferences().getSessionId(), parsedHeader); + return new NetconfServerSession(sessionListener, channel, sessionId, parsedHeader); } /** diff --git a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java index bbe563b5b1..3949db62d2 100644 --- a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java +++ b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java @@ -21,10 +21,10 @@ import io.netty.util.concurrent.Promise; import java.util.Optional; import java.util.concurrent.TimeUnit; import org.checkerframework.checker.lock.qual.GuardedBy; +import org.eclipse.jdt.annotation.NonNull; import org.opendaylight.netconf.api.NetconfDocumentedException; import org.opendaylight.netconf.api.NetconfMessage; import org.opendaylight.netconf.api.NetconfSessionListener; -import org.opendaylight.netconf.api.NetconfSessionPreferences; import org.opendaylight.netconf.api.messages.NetconfHelloMessage; import org.opendaylight.netconf.api.xml.XmlNetconfConstants; import org.opendaylight.netconf.nettyutil.handler.FramingMechanismHandlerFactory; @@ -38,8 +38,8 @@ import org.slf4j.LoggerFactory; import org.w3c.dom.Document; import org.w3c.dom.NodeList; -public abstract class AbstractNetconfSessionNegotiator

, L extends NetconfSessionListener> +public abstract class AbstractNetconfSessionNegotiator, + L extends NetconfSessionListener> extends ChannelInboundHandlerAdapter implements NetconfSessionNegotiator { /** * Possible states for Finite State Machine. @@ -51,7 +51,7 @@ public abstract class AbstractNetconfSessionNegotiator

promise, + protected AbstractNetconfSessionNegotiator(final NetconfHelloMessage hello, final Promise promise, final Channel channel, final Timer timer, final L sessionListener, final long connectionTimeoutMillis) { - this.channel = requireNonNull(channel); + this.localHello = requireNonNull(hello); this.promise = requireNonNull(promise); - this.sessionPreferences = sessionPreferences; + this.channel = requireNonNull(channel); this.timer = timer; this.sessionListener = sessionListener; this.connectionTimeoutMillis = connectionTimeoutMillis; } + protected final @NonNull NetconfHelloMessage localHello() { + return localHello; + } + protected final void startNegotiation() { if (ifNegotiatedAlready()) { LOG.debug("Negotiation on channel {} already started", channel); @@ -101,17 +105,12 @@ public abstract class AbstractNetconfSessionNegotiator

promise; private EmbeddedChannel channel; - private AbstractNetconfSessionNegotiator negotiator; + private TestSessionNegotiator negotiator; @Before public void setUp() throws Exception { @@ -55,10 +55,9 @@ public class Netconf539Test { channel.pipeline().addLast(NETCONF_MESSAGE_FRAME_ENCODER, FramingMechanismHandlerFactory.createHandler(FramingMechanism.EOM)); channel.pipeline().addLast(NETCONF_MESSAGE_AGGREGATOR, new NetconfEOMAggregator()); - final NetconfHelloMessage serverHello = NetconfHelloMessage.createClientHello(Collections - .singleton(XmlNetconfConstants.URN_IETF_PARAMS_NETCONF_BASE_1_1), Optional.empty()); - negotiator = new TestSessionNegotiator(new NetconfSessionPreferences(serverHello), promise, channel, - new HashedWheelTimer(), listener, 100L); + final NetconfHelloMessage serverHello = NetconfHelloMessage.createClientHello( + Set.of(XmlNetconfConstants.URN_IETF_PARAMS_NETCONF_BASE_1_1), Optional.empty()); + negotiator = new TestSessionNegotiator(serverHello, promise, channel, new HashedWheelTimer(), listener, 100L); } @Test @@ -75,11 +74,11 @@ public class Netconf539Test { final Document helloDocument = XmlFileLoader.xmlFileToDocument(fileName); negotiator.startNegotiation(); final NetconfHelloMessage helloMessage = new NetconfHelloMessage(helloDocument); - final AbstractNetconfSession session = negotiator.getSessionForHelloMessage(helloMessage); - Assert.assertNotNull(session); - Assert.assertTrue("NetconfChunkAggregator was not installed in the Netconf pipeline", + final TestingNetconfSession session = negotiator.getSessionForHelloMessage(helloMessage); + assertNotNull(session); + assertTrue("NetconfChunkAggregator was not installed in the Netconf pipeline", channel.pipeline().get(NETCONF_MESSAGE_AGGREGATOR) instanceof NetconfChunkAggregator); - Assert.assertTrue("ChunkedFramingMechanismEncoder was not installed in the Netconf pipeline", + assertTrue("ChunkedFramingMechanismEncoder was not installed in the Netconf pipeline", channel.pipeline().get(NETCONF_MESSAGE_FRAME_ENCODER) instanceof ChunkedFramingMechanismEncoder); } } \ No newline at end of file diff --git a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/TestSessionNegotiator.java b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/TestSessionNegotiator.java index d5e895515b..61be232134 100644 --- a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/TestSessionNegotiator.java +++ b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/TestSessionNegotiator.java @@ -5,39 +5,30 @@ * 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.netconf.nettyutil; import io.netty.channel.Channel; import io.netty.util.Timer; import io.netty.util.concurrent.Promise; -import org.opendaylight.netconf.api.NetconfDocumentedException; import org.opendaylight.netconf.api.NetconfSessionListener; -import org.opendaylight.netconf.api.NetconfSessionPreferences; import org.opendaylight.netconf.api.messages.NetconfHelloMessage; -final class TestSessionNegotiator extends - AbstractNetconfSessionNegotiator> { - - - TestSessionNegotiator(final NetconfSessionPreferences sessionPreferences, - final Promise promise, final Channel channel, - final Timer timer, - final NetconfSessionListener sessionListener, - final long connectionTimeoutMillis) { - super(sessionPreferences, promise, channel, timer, sessionListener, connectionTimeoutMillis); +final class TestSessionNegotiator + extends AbstractNetconfSessionNegotiator> { + TestSessionNegotiator(final NetconfHelloMessage hello, final Promise promise, + final Channel channel, final Timer timer, + final NetconfSessionListener sessionListener, final long connectionTimeoutMillis) { + super(hello, promise, channel, timer, sessionListener, connectionTimeoutMillis); } @Override - protected TestingNetconfSession getSession(final NetconfSessionListener sessionListener, final Channel channel, - final NetconfHelloMessage message) - throws NetconfDocumentedException { + protected TestingNetconfSession getSession(final NetconfSessionListener sessionListener, + final Channel channel, final NetconfHelloMessage message) { return new TestingNetconfSession(sessionListener, channel, 0L); } @Override - protected void handleMessage(final NetconfHelloMessage netconfHelloMessage) throws Exception { - + protected void handleMessage(final NetconfHelloMessage netconfHelloMessage) { + // No-op } } \ No newline at end of file