-
Notifications
You must be signed in to change notification settings - Fork 89
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
Adjust gNMI SetRequest Update handling for list keys + value payloads #176
base: master
Are you sure you want to change the base?
Conversation
@gcsl - PTAL - this just tightens up the protocol around disallowing list keys to be included in the value payload since they are immutable in this context |
@dplore - looking for review here. This has been a long standing PR - it looks like some protocol needs to be put in place across various repos to ensure distribution of review - Thx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a behavioral change here regarding support for idempotent behavior that I don't think we want to make. See comments.
@@ -1283,8 +1291,8 @@ root + | |||
In this case, nodes `k1` and `k2` are standalone nodes within the schema, but | |||
also correspond to attribute values for the node "`f`". In this case, an update | |||
or replace message specifying a path of `/a/f[k1=10][k2=20]` setting the value | |||
of `k1` to 100 MUST be considered erroneous, and a status code of | |||
`InvalidArgument (3)` specified. | |||
of `k1` to either the same value (10) or an alternate value (100) MUST be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support idempotent behavior, I believe setting a node which is also a list key to the same value it already has should be permitted. Only changing the node's value must be an error.
path, and as their own elements within the value, then this MUST be considered | ||
an error by the target system - and a status code of `InvalidArgument (3)` | ||
specified. Targeting a list member with update or replace operations that | ||
modify it's own keys in either conflicting or identical fashion is not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support idempotent behavior, identical update/replace should be permitted. Agreed that conflicting node / list key values must return an error. I think that is already covered by the last bullet point in this stanza,
Clarify that an Update to a list member targeted in the Path cannot include
those list keys in the value payload either as a direct descendant or ultimate
leafref'd node.
Since you cannot modify the list keys on a targeted Path, disallow the contents
altogether in the payload. At best, the same values could be specified but
since the Path is unprefixed while
JSON_IETF
encoding currently remainsprefixed, an association needs to be drawn from a schema set perspective.
In addition, this reduces pointless and redundant bytes on the wire.