CESNET / libyang

YANG data modeling language library
BSD 3-Clause "New" or "Revised" License
368 stars 292 forks source link

Priority of error messages during validation #2147

Open jktjkt opened 10 months ago

jktjkt commented 10 months ago

It seems that some must violations are reported before the mandatory violations. For example, in this model:

module wtf {
  yang-version 1.1;
  namespace "wtf";
  prefix "wtf";
  container channel-plan {
    list channel {
      key "name";
      leaf name {
        type string;
      }
      leaf lower-frequency {
        type uint64;
        mandatory true;
      }
      leaf upper-frequency {
        type uint64;
        mandatory true;
      }
      must "lower-frequency >= 191325000" {
        error-message "Cannot use frequency lower than 191.325 THz";
      }
      must "upper-frequency <= 196125000" {
        error-message "Cannot use frequency higher than 196.125 THz";
      }
    }
  }
}

...and the following data:

{
  "wtf:channel-plan": {
    "channel": [
      {
        "name": "x",
        "lower-frequency": "193000000"
      }
    ]
  }
}

...I get this message, and some SW which only reports the first violation (ahem ahem, yang-cli from netconf-cli) would only say that Cannot use frequency higher than 196.125 THz (Data location "/wtf:channel-plan/channel[name='x']".).

Of course yanglint is a bit smarter:

$ yanglint -f json ~/work/cesnet/gerrit/CzechLight/cla-sysrepo/sample.json ~/work/cesnet/gerrit/CzechLight/cla-sysrepo/yang/wtf.yang 
libyang err : Cannot use frequency higher than 196.125 THz (Data location "/wtf:channel-plan/channel[name='x']".)
libyang err : Mandatory node "upper-frequency" instance does not exist. (Data location "/wtf:channel-plan/channel[name='x']".)
YANGLINT[E]: Failed to parse input data file "/home/jkt/work/cesnet/gerrit/CzechLight/cla-sysrepo/sample.json".

I was wondering if there's a way (apart from adding a must "count(lower-frequency) = 1 and count(upper-frequency) = 1") so that the root cause, which is a missing node, is reported "more prominently". One could even argue that evaluating an XPath condition like this when there's no default value for that node makes little sense, but I don't know what the standard says about this.

As a programmer, I guess this happens because the validation works from the root of the tree, and the must is actually present at the outer enclosing node, which gets checked before its children, right?

jktjkt commented 10 months ago

The original, internal bugreport which triggered this one is actually the behavior of netopeer2-server. It appears to report only the first violation, which is about that must, and not the second one which is about the missing node (and the real cause), btw. Or maybe netconf-cli botches this, somehow?

michalvasko commented 10 months ago

I cannot help you in this case. The validation is performed first on parents and then on their children, nothing else makes sense. And because the must conditions belong to the parent channel node and the mandatory flag is set on the children lower-frequency and upper-frequency, one is checked before the other.

jktjkt commented 10 months ago

Does netopeer2-server report all validation errors, or just the first one?

michalvasko commented 10 months ago

Only the first one, currently. With several errors there is always risk of reporting some invalid/irrelevant ones (such as in this case) so I would prefer to keep it this way.

jktjkt commented 10 months ago

In this case, though, the relevant error is the second one, while the first one is a bit misleading.

michalvasko commented 10 months ago

Yes, but in most cases it is the other way around. There is no perfect solution and the current one is best from those available, I believe.