Deviation changing `leafref` to another type still triggers leafref validation.

[robshakir]

Considering this module:

module a {
    prefix "a";
    namespace "urn:a";

    container a {
        leaf b {
            type leafref {
                path "/d"; // does not exist
            }
        }
    }

    deviation "/a/b" {
        deviate replace {
            type string;
        }
    }
}

the deviate replace changes the type for /a/b to string, but pyang will still fail reporting that path /d doesn't exist. The replace is successful, but since i_leafref remains set, then the validation continues to check the leafref characteristics.

Opening this issue to get an issue number to create the test demonstrating the fix here.

Note, this fix appears to need a new validation phase to be added, since reference_1 runs over statements with children, unless I've misunderstood something. Happy to take alternate suggestions as to the fix. I'll open a PR.

[mbj4668]

Hmm, I don't think is an accurate description on what's going on. pyang validates the leafref before applying deviations.

Consider this version of the module:

module a {
  namespace "urn:a";
  prefix a;

  container a {
    leaf b {
      type leafref {
        path "/d"; // does not exist
      }
    }
  }
  leaf d {
    type int8;
  }
  deviation "/d" {
    deviate not-supported;
  }
  deviation "/a/b" {
    deviate replace {
      type string;
    }
  }
}

If the i_leafref property was not deleted, this module would fail to validate.

I don't think that this is in fact a bug. RFC 7950 says about deviate:

   The "deviate" statement defines how the server's implementation of
   the target node deviates from its original definition.

So this means that you have an original model, which then an implementation can choose to deviate from. Hence, the original model must be valid in itself. This is why pyang first validates the references, and then applies the deviations.

Do you have a use case for this in mind?

[robshakir]

That is indeed an interesting case. But I think it is valid using the same logic. The module is valid once all its statements have been considered. 'd' is removed and then never referenced. I'm not clear we should expect a half parsed module to be valid in and of itself. Of course this - the module deviating itself - is not the common case. We expect that the server publishes deviations authored the implementor of the server to a module that is not written by the same author.

The scenario that this is used for is more nuanced than the test module. We have a structure that is used within a wider context - namely "openconfig-aft" that is used within "openconfig-network-instance". The AFT module has an absolute leafref to '/network-instances/network-instance/config/name' within a grouping - let's call it 'aft-top'.

We want to create a module which is just the subtree defined in 'aft-top' for code generation purposes. To do this we have a 'uses aft-top' and then a number of 'deviate replace' statements to replace the absolute leafrefs with the type that they target.

In this case, I argue that the original module is valid - since the original module is the sum of the 'uses' and 'deviate' statements.

[robshakir]

Sorry. I hit send too early here - the gap in the specification here is really around existence of 'deviate' statements that modify the contents of data nodes defined within the same module. I can't find anything in 7950 that says this construct is invalid.

How do we solve this otherwise - one needs to apply a patch to the module. You can see this in https://github.com/openconfig/gribi where we apply a patch before generating protobuf - this solution seems significantly cleaner since it keeps the changes within the source YANG itself.

[robshakir]

Friendly ping here -- this issue is causing us some upstream issues. I'd like to discuss the above further if there's disagreement here. :-) Thanks & Happy New Year!

[mbj4668]

It was never the intention that deviations would be used in this way. But setting that aside for a moment, the proposed fix is not correct, since it rejects the module in my example above. Also, it rejects the following variant of your original module:

module a {
  namespace "urn:a";
  prefix a;

  typedef dref {
    type leafref {
      path "/d"; // does not exist
    }
  }   

  container a {
    leaf b {
      type dref;
    }
  }
  deviation "/a/b" {
    deviate replace {
      type string;
    }
  }
}