XpathExpression doesn't accept Concatenated String

[huyuwen]

In recent tests, we found that XpathExpression doesn't accept those string that are concatenated with two segament and connection with '+'. There are 3 statements are impacted, When, Must, and Path. All xpath expression which used in the 3 statement which has '+' couldn't be loaded correctly. For instance: path "/qosipos:qos/qosipos:congestion-avoidance-"

• "map/qosipos:map-name" must "number(.) <= number(../../rate-num/rate-"
  • "num)"

All these examples mostly have the same feature, breaking one word into two parts. "congestion-avoidance-map" -> "congestion-avoidance-" + "map" "rate-num" -> "rate-" + "num"

It is not mandatory to have '-' in these strings. any break word will cause problem.

[svenefftinge]

Yes, that is a known limitation. You need to break xpath expressions on token-boundaries. E.g. the following should work:

path "/qosipos:qos/qosipos:congestion-avoidance-map"
+ "/qosipos:map-name"
must "number(.) <= number(../../rate-num/"
+ "rate-num)"

[huyuwen]

Well, I know that breaking xpath expressions on token-boundaries works. This issue will make our tools non backward compatible. Previously, we accepted the xpath expression which in my example. But now, user have to re-format quite a lot of their xpath expression, otherwise the tool couldn't continue to work. Is it too costly to fix this limitation?

[svenefftinge]

Yes, it would be very complicated, as with the current limitation we can just ignore " + ".

[svenefftinge]

Maybe there is another way we can improve the situation for users? E.g. having a special validation and quickfix?

[huyuwen]

From our point of view, we hope that tools could be backward compatible as much as possible. So is it possible to construct/merge these kind of concatenated strings into one string before parsing as XpathExpression?

As to special validation, could you give us an example? Regarding to quickfix, of cause, it is good to have from IDE. But the real situation is, we couldn't force all our users to use Yang IDE (whatever yangster, theia, eclipse, etc.). Some of our user even don't use IDE at all. They generate yang file from CLI tools. In this case, there is no chance to run this kind of quickfix.

BTW, It seems SchemaNodeIdentifier has the same limitation, like path in Augment.

[svenefftinge]

They generate yang file from CLI tools.

If the yang models are generated, couldn't you fix the generator?

We could also create a CLI tool, that will fix any of these issues.

[huyuwen]

Right, I have suggested them to do small improvements regarding to the line break behaviors. But I haven't get their response. What kind of CLI tool could you provide to us?

[svenefftinge]

Something that applies the quick fixes mentioned above. But it would be really much better if that was fixed where it is caused.

[huyuwen]

When could you provide such a CLI tool in case we can't get it fixed where it is caused.

[JanKoehnlein]

I could start working on it now.

[JanKoehnlein]

Here's an example CMD-line tool fixing these concatenations using the same lexer as the LSP:

package io.typefox.yang.parser.antlr.lexer.jflex

import java.io.FileReader
import org.antlr.runtime.Token
import io.typefox.yang.parser.antlr.internal.InternalYangParser
import java.util.regex.Pattern

class YangFix {

    static val CONCAT_PATTERN = Pattern.compile('(\"\\s*\\+\\s*\"|\'\\s*\\+\\s*\')')

    def static void main(String[] args) {
        val reader = new FileReader(args.get(0))
        val yangFlexer = new YangFlexer(reader)
        var stop = false
        var lastHidden = ''
        do {
            val token  = yangFlexer.nextToken
            if(token.type === Token.EOF) 
                stop = true
            if (token.type == InternalYangParser.RULE_HIDDEN) {
                lastHidden += token.text
            } else {
                if(!lastHidden.empty && !CONCAT_PATTERN.matcher(lastHidden).matches) 
                    print(lastHidden)
                lastHidden = ''
                if(token.text !== null)
                    print(token.text)
            }               
        } while (!stop)
    }
}

You could also do it with sed, which may be a bit less secure:

cat my.yang | sed -e 's/"[[:space:]]*+[[:space:]]*"//g'

[JanKoehnlein]

Is that enough or should I wrap that into an executable jar?

[huyuwen]

Let me try with the two solutions, will come back to you soon.

[huyuwen]

Hi Jan,

I tried to run YangFix against a very simple yang file. Basicly it works fine. I will do some further test and consider how intergrate into our logic. Regarding to the sed command, it doesn't work.

[JanKoehnlein]

sed's regexps are a bit different on all machines. The one above works on my Mac. Here are some alternatives

cat my.yang | sed 's/"\s*\+\s*"//g'
cat my.yang | sed -e 's/"\s*\+\s*"//g'
cat my.yang | sed -E 's/"\s*\+\s*"//g'

[spoenemann]

Let's reopen this in case more tools / quick-fixes are needed.

[andreasjakobik]

Hello,

Can we please reopen this case? We face problems still with autogenerated yang files where must, when and path expressions have been chopped up at non token boundaries. For example:

      container src-port {
        when "(not(../../ssh-and-telnet-acl)) and (../acl-"
        + "protocol='tcp' or ../acl-protocol='udp' or ../acl-"
        + "protocol=6 or ../acl-protocol=17)" {
          description
            "";
        }
        //...

We have built for our own tools a fixer, but it does not help when viewing the modules in Yangster on VSCode or Theia environments.

Other open source or commercial tools such as pyang and Confd accept the concatenated strings where token boundaries have been broken, so yang-lsp unfortunately "looks bad" here.

Since the generated yang files must not be touched, yang-lsp should accept them as is.

Thanks, Andreas

[dhuebner]

@andreasjakobik Is that mainly about a possibility to split inside the ID?
"../acl-protocol" - > "../acl-" + "protocol" ~~... or do you have other cases in generated yang files like e.g. split before a colon? "/ecertm:certm/ecertm:user-label" -> "/ecertm:certm/ecertm:" + "user-label"~~

Just noticed that a failing case comes from deviation "/ecertm:certm/ecertm:" + "user-label" which is not the XpathExpression but SchemaNodeIdentifier and SchemaNodeIdentifier is not a Part of this issue

[andreasjakobik]

Yeah, believe so. Seems you find a solution then? Thanks!

[dhuebner]

@andreasjakobik

Seems you find a solution then?

Yes, after talking to @svenefftinge we found a good solution. All existing tests are passing and I added a bit more for Xpath too. Please let me know if there are any drawbacks in tooling or serialization on your side.

Overall I think this one can finally be closed

[dhuebner]

@andreasjakobik Did you had a chance to try it out with a nightly build?

[andreasjakobik]

@dhuebner Just tested and it works like a charm! The validator utility we have now reports 0 errors 0 warnings on a file that previously had a lot of errors and warnings.

We ususually wait for an official release to test, so that we can easily pull a new version from repo https://repo1.maven.org/maven2/io/typefox/yang/io.typefox.yang/ but it was not a problem really to clone the yang-lsp git repo, build and use the io.typefox.yang-0.4.0-SNAPSHOT.jar instead.

Thanks!

[dhuebner]

@andreasjakobik

Just tested and it works like a charm!

Glad to hear!

@spoenemann and @kaisalmen are currently working on setup for GitHub action to prepare a release. So I hope you and others can consume 0.4.0 release soon.

[andreasjakobik]

Sorry... :-)

Upon further testing, broken augment paths still pose a problem. Here is an example:

augment "/if:interfaces/if:interface/ethxrouter6000:ethernet/l"
  + "2servicer6k:service-instance" {

When putting all in one line it is fine.

[dhuebner]

@andreasjakobik augment uses SchemaNodeIdentifier and not the Xpath see my comment above https://github.com/theia-ide/yang-lsp/issues/113#issuecomment-824024678

SchemaNodeIdentifier is a bit different to Xpath expression and need to be handled separately. Should I reopen this one or create a new issue?

[andreasjakobik]

@dhuebner I can create a new one if you wish.

[dhuebner]

@andreasjakobik I would prefer a new Issue because the possible solution might be different to one for Xpath

[andreasjakobik]

@dhuebner Sure, #205 has been created.