mbj4668 / pyang

An extensible YANG validator and converter in python
ISC License
530 stars 342 forks source link

YIN parser doesn't handle import by revision (and it doesn't even matter) #850

Open predi opened 1 year ago

predi commented 1 year ago

Hi,

pyang's YIN parser improperly handles imports by revision for my carefully crafted test case. It dismisses the second import as part of "import cycle detection code" in yin_parser.py Line 293.

                    if modname in mymodules:
                        # circular import; ignore here and detect in validation
                        pass
<?xml version="1.0" encoding="utf-8"?>
<module xmlns="urn:ietf:params:xml:ns:yang:yin:1" 
        xmlns:t="test:uri" 
        xmlns:t1="test-dep:uri" 
        xmlns:t2="test-dep:uri" 
        name="test">
  <yang-version value="1.1"/>
  <namespace uri="test:uri"/>
  <prefix value="t"/>
  <import module="test-dep">
    <revision-date date="2023-03-01"/>
    <prefix value="t1"/>
  </import>
  <import module="test-dep">
    <revision-date date="2023-03-30"/>
    <prefix value="t2"/>
  </import>
  <revision date="2023-03-30"/>
  <container name="top">
    <t2:extension/>
    <leaf name="foo">
      <type name="t1:type"/>
    </leaf>
  </container>
</module>
<?xml version="1.0" encoding="utf-8"?>
<module xmlns="urn:ietf:params:xml:ns:yang:yin:1"
        xmlns:td="test-dep:uri" 
        name="test-dep">
  <yang-version value="1.1"/>
  <namespace uri="test-dep:uri"/>
  <prefix value="td"/>
  <revision date="2023-03-01"/>
  <typedef name="type">
    <type name="uint8">
      <range value="1..5"/>
    </type>
  </typedef>
</module>
<?xml version="1.0" encoding="utf-8"?>
<module xmlns="urn:ietf:params:xml:ns:yang:yin:1"
        xmlns:td="test-dep:uri"
        name="test-dep">
  <yang-version value="1.1"/>
  <namespace uri="test-dep:uri"/>
  <prefix value="td"/>
  <revision date="2023-03-30"/>
  <revision date="2023-03-01"/>
  <typedef name="type">
    <type name="uint8">
      <range value="1..10"/>
    </type>
  </typedef>
  <extension name="extension"/>
</module>

Run with:

pyang_tool.py test@2023-03-30.yin

Result:

test@2023-03-30.yin:9: warning: imported module "test-dep" not used
test@2023-03-30.yin:15: error: extension "extension" is not defined in module test-dep

Expected: no error or warning

The test case is actually meant to demonstrate a bug in RFC 7950, Section 13, YIN format: the YANG <-> YIN mapping cannot be considered bidirectional (RFC claims this to be true) due to the fact that XML namespace bindings (needed by extensions) cannot replace the prefix:module mappings in module header statements. IMO, it was a mistake to "encode" YANG extension usages in YIN the way they currently are, as the "encoding" is inherently ambiguous for import by revision. Not to mention it requires needlessly complex YIN parser implementations (pyang's own implementation speaks for itself in this regard) that need to know the extension's definition to properly parse what should essentially be keyword:argument pairs expressed as XML.

mbj4668 commented 1 year ago

This is an unforeseen effect of allowing multiple imports of the same module in YANG 1.1.

In YANG 1.0 this wasn't allowed, and there the YIN mapping works.

predi commented 1 year ago

The YIN parser implementation in pyang could be revised to choose the import with the newest revision in such a case (making it even more complex).

mbj4668 commented 1 year ago

Yes it could, but as you indicated, this behavior is not specified in the RFC, and from an XML-perspective it is questionable.

In

<foo xmlns:p1="urn:example:p" xmlns:p2="urn:example:p">
  <p1:bar/>
  <p2:baz/>
</foo>

The elements "bar" and "baz" both have the same namespace.

predi commented 1 year ago

Indeed. It doesn't help that p1 and p2 don't need to match the prefix mappings in a YANG module (or even be supplied by an XML parser implementation to the application).

Another way out of this would be to issue a warning for such cases. If you reverse the order of import statements in my test case, pyang will pretend that everything checks out, which is not ideal either.