Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Priority of error messages during validation #2147

Open
jktjkt opened this issue Dec 21, 2023 · 6 comments
Open

Priority of error messages during validation #2147

jktjkt opened this issue Dec 21, 2023 · 6 comments
Labels
is:question Issue is actually a question.

Comments

@jktjkt
Copy link
Contributor

jktjkt commented Dec 21, 2023

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
Copy link
Contributor Author

jktjkt commented Dec 21, 2023

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 michalvasko added the is:question Issue is actually a question. label Dec 21, 2023
@michalvasko
Copy link
Member

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
Copy link
Contributor Author

jktjkt commented Dec 21, 2023

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

@michalvasko
Copy link
Member

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
Copy link
Contributor Author

jktjkt commented Dec 21, 2023

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

@michalvasko
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:question Issue is actually a question.
Projects
None yet
Development

No branches or pull requests

2 participants