Add namespace aware parsing for <hello> #50

GregDThomas · 2021-06-12T12:39:21Z

This PR is part of the fix for #49

It
a) Introduces a new Hello.java that represents the <hello> NETCONF element. This is generated by using a namespace aware XML parser, so copes with the "nc:" prefix discussed in the ticket
b) uses the Hello.java to both generate the client <hello> and parse the server <hello>. This means that the library now will correctly identify the session id when the server is using a nc: prefix. Note that before making a changes, I wrote tests around both sending the client <hello> and receiving the server <hello> - so I'm confident it's not had an deleterious effects. I've also tested it by accessing a local device - and is working OK.

Note; this is only part of the fix for the issue. The <rpc-reply> element needs similar work before the namespace prefix issue can be fully resolved.

I like to keep each PR as small as possible, to make it clearer what's going on, and
The <rpc-reply> element is going to be more complex - before I spent lots of effort on it I wanted to have some confidence that my mechanism for solving it would be acceptable

So any feedback grateful appreciated!

ydnath · 2021-06-14T08:13:46Z

@peterjhill please review

peterjhill · 2021-06-14T17:26:03Z

@GregDThomas , could you please squash all the commits into a single commit. In case you haven't done that before, in your fork, you would do

git rebase -i 5c021c7
I think that is the first hash after your feature branch, closest to where you branched.

That will bring up a window with a list of commit hashes and comments.
You can change all the picks to "s" for squash.

Then it will allow you to update the commit message. I would update the message with all the changes in the final commit.

Here is an article on squashing: https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec

peterjhill · 2021-06-14T17:29:56Z

src/main/java/net/juniper/netconf/Device.java

 import com.jcraft.jsch.JSch;
 import com.jcraft.jsch.JSchException;
 import com.jcraft.jsch.Session;
+import java.util.Arrays;


nit (you don't need to change)

I would have added this down around line 33, right above the import for java.util.List.

peterjhill · 2021-06-14T17:32:10Z

src/main/java/net/juniper/netconf/Device.java

     * @param capabilities A list of netconf capabilities
     * @return the hello RPC that represents those capabilities.
     */
    private String createHelloRPC(List<String> capabilities) {


This is cool. I wonder if it could have been part of the Hello.java?

It was an existing method - I just updated it to use the new Hello.java.

I'd be reluctant to add the ]]>]]> message separator as part of the Hello.java - as that class is solely dedicated to parsing/creating XML which the separator isn't (it's purely for NETCONF over SSH).

The remaining code,

Hello.builder() .capabilities(capabilities) .build() .toXML()

could be a static method of Hello.java (perhaps Hello.createXmlOnlyWithCapabilities(capabilities) ?) but am not really sure that's adding anything?

(there's another question in my mind if Device.java should be doing this, anyway? Surely it's NetconfSession that controls the details of the open session, and should therefore be in control of the shape of the <hello> that is sent. Especially if there was ever a desire to support the chunked framing mechanism in NETCONF urn:ietf:params:netconf:base:1.1 - but I think that's definitely a discussion for another day!)

peterjhill · 2021-06-14T17:33:51Z

src/main/java/net/juniper/netconf/NetconfSession.java

 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
+import javax.xml.parsers.DocumentBuilder;


Not sure why you rearranged these? Any chance you could revert?

You'll need to keep the addition of the net....Hello; import. The others look like you just moved them around.

The re-arrange was my IDE being "helpful" - it's defaulting to alphabetical order. Apologies, I'll reformat to try and keep to the project style.

I've also added a small .editorconfig file (https://editorconfig.org/) that in addition to some basic configs in line with the project, also define some IntelliJ specific configurations so that IntelliJ users, at least, won't trip up over the same thing!

peterjhill · 2021-06-14T17:34:51Z

src/main/java/net/juniper/netconf/NetconfSession.java

        lastRpcReply = reply;
+        try {
+            serverHello = Hello.from(reply);
+        } catch (final Exception e) {


is there are more specific exception that can be caught here?

Yes, apologies, that was just me being lazy.

peterjhill · 2021-06-14T17:35:59Z

src/main/java/net/juniper/netconf/NetconfSession.java

-        if (idSplit.length != 2)
-            return null;
-        return idSplit[0];
+        return serverHello.getSessionId();


just curious, could this be a static method in Hello.java?

This couldn't be, no. An instance of the Hello object represents a single <hello> NETCONF element. So the session-id is an instance variable.

…ssue with XML namespace prefixes

GregDThomas · 2021-06-14T19:11:46Z

could you please squash all the commits into a single commit.

OK, I've squashed that down to one commit. I think I've made changes for all your comments except two, which I've responded to above.

Thanks for the feedback!

peterjhill

This is great. Really all my remaining comments are nits. You can take them or leave them. Thanks for rebasing and for the explanations.

peterjhill · 2021-06-14T19:12:37Z

src/main/java/net/juniper/netconf/element/Hello.java

+        final Document document = documentBuilderFactory.newDocumentBuilder()
+                .parse(new InputSource(new StringReader(xml)));
+        final XPath xPath = XPathFactory.newInstance().newXPath();
+        final String sessionId = xPath.evaluate("/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='hello']/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='session-id']", document);


that is a super long string. Would it make sense to add this to the NetconfConstants.java?

There’s certainly some refactoring that could be done here. However, I plan to do it with the implementation. My current thinking is that both Hello and RpcReply will inherit from AbstractNetconfElement which will contain various utility methods that can be shared between them. So I’d like to put it off for now, if that’s OK.

peterjhill · 2021-06-14T19:12:50Z

src/main/java/net/juniper/netconf/element/Hello.java

+        final HelloBuilder builder = Hello.builder()
+                .originalDocument(document)
+                .sessionId(sessionId);
+        final NodeList capabilities = (NodeList) xPath.evaluate("/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='hello']/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='capabilities']/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='capability']", document, XPathConstants.NODESET);


that is a super long string. Would it make sense to add this to the NetconfConstants.java?

As above …

peterjhill · 2021-06-14T19:13:44Z

src/main/java/net/juniper/netconf/element/Hello.java

+                .originalDocument(document)
+                .sessionId(sessionId);
+        final NodeList capabilities = (NodeList) xPath.evaluate("/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='hello']/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='capabilities']/*[namespace-uri()='urn:ietf:params:xml:ns:netconf:base:1.0' and local-name()='capability']", document, XPathConstants.NODESET);
+        for (int i = 0; i < capabilities.getLength(); i++) {


nit, this is fine.. I would have been tempted to do something like

capabilites.foreach(capability -> {

Unfortunately the NodeList doesn’t implement iterable so it has to be an old-style ‘for’ statement. Though it does highlight that there is a lot of reliance on old-style, pretty much deprecated javax packages. But I think changing the public API of the library is beyond the scope of this PR!

peterjhill · 2021-06-14T19:17:53Z

src/main/java/net/juniper/netconf/element/Hello.java

+        }
+
+        final Element helloElement
+                = createdDocument.createElementNS(NetconfConstants.URN_XML_NS_NETCONF_BASE_1_0, "hello");


When I see four lines that are very similar, I wonder if it could be turned into a private function. it looks like the difference is the name space name? So if you could pass in a Document and name space name and it would return an Element?

I'd also be tempted to make the name space name an enum and use that instead of the string.

peterjhill · 2021-06-14T19:18:51Z

src/main/java/net/juniper/netconf/NetconfSession.java

-        if (idSplit.length != 2)
-            return null;
-        return idSplit[0];
+        return serverHello.getSessionId();


peterjhill · 2021-06-14T19:19:20Z

src/main/java/net/juniper/netconf/Device.java

     * @param capabilities A list of netconf capabilities
     * @return the hello RPC that represents those capabilities.
     */
    private String createHelloRPC(List<String> capabilities) {


peterjhill · 2021-06-14T19:21:39Z

src/test/java/net/juniper/netconf/element/HelloTest.java

+                        "urn:ietf:params:netconf:base:1.0#confirmed-commit",
+                        "urn:ietf:params:netconf:base:1.0#validate",
+                        "urn:ietf:params:netconf:base:1.0#url?protocol=http,ftp,file");
+        assertThat(hello.hasCapability("urn:ietf:params:netconf:base:1.0#candidate"))


I love assertThat!!!

i like how much easier it is to read..

peterjhill · 2021-06-14T19:25:18Z

Let me know if you want to make any more changes based on my comments. I'll ping Juniper once we are all set so they can do any final checks and merge.

Thanks for your contributions!

peterjhill · 2021-06-14T19:48:40Z

Cool, I will ping Juniper.

GregDThomas · 2021-06-14T19:52:57Z

Nope, no more plans to make changes unless someone strongly objects to the current code!

Vladislav-Kisliy · 2021-06-20T14:41:41Z

.editorconfig

+ij_java_imports_layout = *,|,javax.**,java.**,|,$*
+ij_java_layout_static_imports_separately = true
+ij_java_names_count_to_use_import_on_demand = 999
+ij_java_use_single_class_imports = true


are you sure that this file should be in the remote repo?

That was a deliberate choice by me; an earlier comment from @peterjhill highlighted that I had inadvertently shuffled around the order of import statements. This file should stop that happening for in the future for users of IntelliJ at least by configuring it to use the defaults used in the project.

The file also defines standard editorconfig settings for things like line-endings, tab width etc., also in line with the project. These should work with most, if not all IDEs, and should make it easier for contributors to maintain the project style.

I'm happy to remove it if the consensus is it is unnecessary.

Vladislav-Kisliy · 2021-06-20T15:10:42Z

src/main/java/net/juniper/netconf/NetconfSession.java

+        try {
+            serverHello = Hello.from(reply);
+        } catch (final ParserConfigurationException | SAXException | XPathExpressionException e) {
+            throw new NetconfException("Invalid <hello> message from server: " + reply, e);
+        }


May I ask about a pack of final words in classes? I can explain the usage of this word for input parameters and in global fields but why do we need to have it for every single variable? Especially in multi-catch sections because these variables are implicitly declared final. Moreover, there is an entity like effective final probably it'd be better to make java compiler works instead of developers, what do you think?

Effectively final was introduced in Java 8. I've been developing in Java on and off since about 1.3, so it's become a fairly reflexive habit for me (*). I'm happy to remove it from where it's unnecessary if you think that's the right thing to do in this case.

(*) Kind of scary now I've written that to realise how long I've been at this! Probably spent more time developing with Java 1.6 than anything else, but that's probably a reflection of how long Java 1.6 was "current"!

GregDThomas · 2021-06-28T10:27:05Z

Hi folks; where are are with this PR? Do you want me to remove the final and/or .editorconfig file? Or is there something else you'd like done? I hope to get to the next stage of this in the coming week, weather permitting.

Vladislav-Kisliy · 2021-06-28T10:38:03Z

I have no objections probably somebody else has 🤷‍♂️

peterjhill · 2021-06-28T20:11:48Z

Let me ping Juniper

GregDThomas · 2021-06-29T15:35:07Z

Thank you for merging.

GregDThomas force-pushed the add-namespace-aware-parsing branch 2 times, most recently from 1462345 to dc45155 Compare June 13, 2021 18:35

peterjhill reviewed Jun 14, 2021

View reviewed changes

Create a Hello.java to parse and create <hello> messages to resolve i…

3446922

…ssue with XML namespace prefixes

GregDThomas force-pushed the add-namespace-aware-parsing branch from dc45155 to 3446922 Compare June 14, 2021 19:09

peterjhill approved these changes Jun 14, 2021

View reviewed changes

Vladislav-Kisliy reviewed Jun 20, 2021

View reviewed changes

ydnath merged commit 80e6eca into Juniper:master Jun 29, 2021

GregDThomas deleted the add-namespace-aware-parsing branch June 29, 2021 15:34

GregDThomas mentioned this pull request Jul 23, 2021

Library does not cope with nc: namespace prefix #49

Closed

Add namespace aware parsing for <hello> #50

Add namespace aware parsing for <hello> #50

Uh oh!

Conversation

GregDThomas commented Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydnath commented Jun 14, 2021

Uh oh!

peterjhill commented Jun 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GregDThomas commented Jun 14, 2021

Uh oh!

peterjhill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterjhill commented Jun 14, 2021

Uh oh!

peterjhill commented Jun 14, 2021

Uh oh!

GregDThomas commented Jun 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GregDThomas commented Jun 28, 2021

Uh oh!

GregDThomas commented Jun 12, 2021 •

edited

Loading