added support for leaf-list default substatement #136 #139
Conversation
|
Hi @robshakir |
Currently, `deviate add` and `deviate delete` on defaults statements are implemented incorrectly, this change makes the necessary fixes.
|
leaf-list's default rules: https://datatracker.ietf.org/doc/html/rfc7950#section-7.7.2 @robshakir when reviewing please see summary in OP. |
| case deviatedNode.IsLeafList(): | ||
| // It is unclear from RFC7950 on how deviate delete works | ||
| // when there are duplicate leaf-list values. | ||
| appendErr(fmt.Errorf("%s: deviate delete on default statements unsupported for leaf-lists, please use replace instead", Source(e.Node))) |
There was a problem hiding this comment.
I'm not sure that I understand the error discussed above. Entries in a leaf-list must be unique (ref), and delete must match a specific keyword, so if you have:
leaf-list foo {
type string;
default "one";
default "two";
}then:
deviation "/foo" {
deviate delete {
default "one";
}
}then the default of foo would become ["two"] rather than ["one", "two"] in the before case. If there is no matching default for one that is removed then this is an error.
There was a problem hiding this comment.
https://datatracker.ietf.org/doc/html/rfc7950#section-7.7
In configuration data, the values in a leaf-list MUST be unique.
What if it's state data? Then which one do we delete?
Unfortunately I couldn't find it in https://datatracker.ietf.org/doc/html/rfc7950#section-7.7.2, and when looking at pyang's behaviour, I hit this: mbj4668/pyang#756.
There was a problem hiding this comment.
I agree that the behaviour can be implemented for config-false leaf-lists, so I've added a TODO to work on it after this PR. This PR is in someone else's repository so I prefer to open a PR for that after this is merged. We need to take care of erroring out when there are duplicate defaults.
There was a problem hiding this comment.
That sounds fine with me.
This whole idea that config true and config false leaf-lists have different approaches seems a problem to me -- it just creates more complexity in the language. I'd be OK with an interpretation that just enforces uniqueness for config true and config false leaflists the same way.
pkg/yang/entry.go
Outdated
| return len(e.DefaultValues()) > 0 | ||
| } | ||
|
|
||
| // SingleDefaultValue returns the schema default value for e, if any. This is |
There was a problem hiding this comment.
I wonder whether rather than requiring a user to do two calls here - i.e.,:
var default string
if a.HasDefault() {
default = a.SingledefaultValue()
}it's better to have these combined, such that you can do:
if def, ok := a.SingleDefaultValue(); a.IsLeaf() && ok {
default = def
}
Since DefaultValues will return nil, then this is detectable in a single call for the leaf-list case so this approach would create some symmetry between the leaf and leaf-list types.
There was a problem hiding this comment.
Doing it this way seems better for usability as well since it makes error handling more natural. I'll make the change.
There was a problem hiding this comment.
Note that Entry.SingleDefaultValue() (string, bool) only returns true when there is one and only one default value. This includes the case when a leaf-list has a single default value.
| } | ||
| }`, | ||
| }, | ||
| wantProcessErrSubstring: "deviate delete on default statements unsupported for leaf-lists", |
There was a problem hiding this comment.
Potentially we can remove this TODO and address the issue here if you agree with my assessment of 7950 above.
There was a problem hiding this comment.
Can you see my reply above to see if you agree with what I intend to do for this here?
There was a problem hiding this comment.
Yep, that sounds good to me.
`SingleDefaultValue` now has an `ok` return value to indicate whether there is one and only one default value for `Entry`. Previously, this function returned the first default value if there are more than one; now, it returns the empty string with `ok=false` since this helps users avoid misuse.
Hi @robshakir,
I've approached the issue described in #136
Let me plz know if that PR satisfies the requirements of the feature and contribution guidelines.
closes #136
Cumulative summary after edits by @wenovus:
LeafListnow has aDefault []*Valuefield.Entry.Defaultchanged to []string .Entry.DefaultValue() string->Entry.SingleDefaultValue() (string, bool)&Entry.DefaultValues() []stringEntry.SingleDefaultValue() (string, bool)only returns true when there is one and only one default value. This includes the case when a leaf-list has a single default value.YangType.HasDefaultto determine whether the empty string is a default value.NOTE: This is a backwards-incompatible change (see summary)