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 <robert.varga@pantheon.tech>

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 5ee52bd073ac1f125176282127bce30faec4c3d3..bbd17ce7bac5f18fc8c90b4c209da9da8d85542a 100644 (file)
--- 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<NetconfClientSessionPreferences, NetconfClientSession,
-                NetconfClientSessionListener> {
+        extends AbstractNetconfSessionNegotiator<NetconfClientSession, NetconfClientSessionListener> {
     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<Set<String>> INTERNER = Interners.newWeakInterner();
 
+    private final NetconfMessage startExi;
+
     NetconfClientSessionNegotiator(final NetconfClientSessionPreferences sessionPreferences,
             final Promise<NetconfClientSession> 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 b1f60731a1b388794ad5a802757f2ce9534e8565..071c75f51c2d9954ab0094baa74be41961b40c3e 100644 (file)
--- 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<NetconfServerSessionPreferences, NetconfServerSession,
-                NetconfServerSessionListener> {
+        extends AbstractNetconfSessionNegotiator<NetconfServerSession, NetconfServerSessionListener> {
     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<NetconfServerSession> 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 bbe563b5b19e8ea5b5fed2cf92ac81c74df61e0c..3949db62d2e21b4fc9397a9701f950b16f01b5c2 100644 (file)
--- 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<P extends NetconfSessionPreferences,
-        S extends AbstractNetconfSession<S, L>, L extends NetconfSessionListener<S>>
+public abstract class AbstractNetconfSessionNegotiator<S extends AbstractNetconfSession<S, L>,
+            L extends NetconfSessionListener<S>>
             extends ChannelInboundHandlerAdapter implements NetconfSessionNegotiator<S> {
     /**
      * Possible states for Finite State Machine.
@@ -51,7 +51,7 @@ public abstract class AbstractNetconfSessionNegotiator<P extends NetconfSessionP
     private static final Logger LOG = LoggerFactory.getLogger(AbstractNetconfSessionNegotiator.class);
     private static final String NAME_OF_EXCEPTION_HANDLER = "lastExceptionHandler";
 
-    protected final P sessionPreferences;
+    private final @NonNull NetconfHelloMessage localHello;
     protected final Channel channel;
 
     private final long connectionTimeoutMillis;
@@ -64,17 +64,21 @@ public abstract class AbstractNetconfSessionNegotiator<P extends NetconfSessionP
     @GuardedBy("this")
     private State state = State.IDLE;
 
-    protected AbstractNetconfSessionNegotiator(final P sessionPreferences, final Promise<S> promise,
+    protected AbstractNetconfSessionNegotiator(final NetconfHelloMessage hello, final Promise<S> 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<P extends NetconfSessionP
         return Optional.ofNullable(channel.pipeline().get(SslHandler.class));
     }
 
-    public final P getSessionPreferences() {
-        return sessionPreferences;
-    }
-
     private void start() {
-        final NetconfHelloMessage helloMessage = this.sessionPreferences.getHelloMessage();
-        LOG.debug("Session negotiation started with hello message {} on channel {}", helloMessage, channel);
+        LOG.debug("Session negotiation started with hello message {} on channel {}", localHello, channel);
 
         channel.pipeline().addLast(NAME_OF_EXCEPTION_HANDLER, new ExceptionHandlingInboundChannelHandler());
 
-        sendMessage(helloMessage);
+        sendMessage(localHello);
 
         replaceHelloMessageOutboundHandler();
         changeState(State.OPEN_WAIT);
@@ -176,8 +175,7 @@ public abstract class AbstractNetconfSessionNegotiator<P extends NetconfSessionP
     }
 
     private boolean shouldUseChunkFraming(final Document doc) {
-        return containsBase11Capability(doc)
-                && containsBase11Capability(sessionPreferences.getHelloMessage().getDocument());
+        return containsBase11Capability(doc) && containsBase11Capability(localHello.getDocument());
     }
 
     /**

diff --git a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java
index a5a4a2395cd853c6af7d460724ae02a1e8569b0b..b343a7e2f81e993c1bad22559ab7c80f888b5d4d 100644 (file)
--- a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java
+++ b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiatorTest.java
@@ -48,7 +48,6 @@ import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 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.api.xml.XmlUtil;
@@ -77,7 +76,6 @@ public class AbstractNetconfSessionNegotiatorTest {
     private NetconfHelloMessage hello;
     private NetconfHelloMessage helloBase11;
     private NetconfXMLToHelloMessageDecoder xmlToHello;
-    private NetconfSessionPreferences prefs;
 
     @Before
     public void setUp() {
@@ -92,9 +90,8 @@ public class AbstractNetconfSessionNegotiatorTest {
         hello = NetconfHelloMessage.createClientHello(Set.of(), Optional.empty());
         helloBase11 = NetconfHelloMessage.createClientHello(
             Set.of(XmlNetconfConstants.URN_IETF_PARAMS_NETCONF_BASE_1_1), Optional.empty());
-        prefs = new NetconfSessionPreferences(helloBase11);
         doReturn(promise).when(promise).setFailure(any());
-        negotiator = new TestSessionNegotiator(prefs, promise, channel, timer, listener, 100L);
+        negotiator = new TestSessionNegotiator(helloBase11, promise, channel, timer, listener, 100L);
     }
 
     @Test
@@ -134,11 +131,6 @@ public class AbstractNetconfSessionNegotiatorTest {
         verify(closedDetector).close(any(), any());
     }
 
-    @Test
-    public void testGetSessionPreferences() {
-        assertEquals(prefs, negotiator.getSessionPreferences());
-    }
-
     @Test
     public void testGetSessionForHelloMessage() throws Exception {
         enableTimerTask();

diff --git a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/Netconf539Test.java b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/Netconf539Test.java
index 7eebcd5009a7a0fdf6d8ab2b416f88886539e1da..6b5de4b9de0cd50e443ed511a9cf6c325d4a6e5d 100644 (file)
--- a/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/Netconf539Test.java
+++ b/netconf/netconf-netty-util/src/test/java/org/opendaylight/netconf/nettyutil/Netconf539Test.java
@@ -7,6 +7,8 @@
  */
 package org.opendaylight.netconf.nettyutil;
 
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 import static org.opendaylight.netconf.nettyutil.AbstractChannelInitializer.NETCONF_MESSAGE_AGGREGATOR;
 import static org.opendaylight.netconf.nettyutil.AbstractChannelInitializer.NETCONF_MESSAGE_FRAME_ENCODER;
 
@@ -14,16 +16,14 @@ import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.embedded.EmbeddedChannel;
 import io.netty.util.HashedWheelTimer;
 import io.netty.util.concurrent.Promise;
-import java.util.Collections;
 import java.util.Optional;
-import org.junit.Assert;
+import java.util.Set;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 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.ChunkedFramingMechanismEncoder;
@@ -43,7 +43,7 @@ public class Netconf539Test {
     private Promise<TestingNetconfSession> 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 d5e895515b2f2bb50ca1bf29605b6a08b652def3..61be232134df2b8fba5de6958b688403470afae9 100644 (file)
--- 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<NetconfSessionPreferences,
-        TestingNetconfSession, NetconfSessionListener<TestingNetconfSession>> {
-
-
-    TestSessionNegotiator(final NetconfSessionPreferences sessionPreferences,
-                          final Promise<TestingNetconfSession> promise, final Channel channel,
-                          final Timer timer,
-                          final NetconfSessionListener<TestingNetconfSession> sessionListener,
-                          final long connectionTimeoutMillis) {
-        super(sessionPreferences, promise, channel, timer, sessionListener, connectionTimeoutMillis);
+final class TestSessionNegotiator
+        extends AbstractNetconfSessionNegotiator<TestingNetconfSession, NetconfSessionListener<TestingNetconfSession>> {
+    TestSessionNegotiator(final NetconfHelloMessage hello, final Promise<TestingNetconfSession> promise,
+            final Channel channel, final Timer timer,
+            final NetconfSessionListener<TestingNetconfSession> 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<TestingNetconfSession> 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