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.

Sign up for GitHub

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

Proposal for clarifications when calling gNMI Set on a list node or element. #140

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Proposal for clarifications when calling gNMI Set on a list node or element. #140

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00c3ad4
Proposal for clarifications when calling gNMI Set on a list node or e…
wenovus May 10, 2021
393efa1
Remove openconfig-specific info, fix example, and improve wording.
wenovus Oct 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 40 additions & 6 deletions rpc/gnmi/gnmi-specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,35 @@ update: <
>
```

For `/a/b[name=b1]`:

```
update: <
path: <
elem: <
name: "a"
>
elem: <
name: "b"
key: <
key: "name"
value: "b1"
>
>
>
val: <
json_ietf_val: `{ "b": {
"name": "b1",
"c": {
"d": "AStringValue",
"e": 10042
}
}
}`
wenovus marked this conversation as resolved.
Show resolved Hide resolved
>
>
```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth explicitly stating here that the contents of the update is the JSON object that would be expected at that list entry?

For `/a` :

```
Expand All @@ -467,10 +496,10 @@ update: <
{
"name": "b1",
"c": {
"d": "AStringValue",
"d": "AStringValue",
"e": 10042
}
}
}
]
}`
>
Expand Down Expand Up @@ -1109,18 +1138,23 @@ array), the following considerations apply:

* In the case that multiple attribute values are required to uniquely address
an element - e.g., `/a/f[k1=10][k2=20] `- and a replace or update
operation's path specifies a subset of the attributes (e.g., `/a/f[k1=10]`)
then this MUST be considered an error by the target system - and an status
code of` InvalidArgument (3)` specified.
operation's path specifies a strict subset of the attributes (e.g.,
`/a/f[k1=10]`, `/a/f`), then this MUST be considered an error by the target
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means that one cannot send /path/to/list with a body that is an array of objects in JSON (assuming JSON_IETF) right? Is there a reason to disallow this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change anything here -- a strict/proper subset is likely what the original sentence meant to say.

I think there is no clear reason to disallow this, since the keys for each element in the array should be provided in the update message as well. However, that would now mean this spec needs to be changed, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an ambiguity here, I think that this was intended to say a non-null subset - i.e., keys were partially specified.

I think there are a few cases here:

  1. No keys are specified - e.g., /interfaces/interface - in this case, I think I expect that the serialised content of the list would be specified. e.g., the array of objects for JSON_IETF.
  2. A partial set of keys are specified - this is an error with InvalidArgument in my view.

I think it'd be worth checking implementations to see whether the no-keys variant is supported. This was the original motivation for having surrounding containers in OpenConfig such that a get + set could be done on the surrounding container to get all entries.

system - and an status code of` InvalidArgument (3)` specified.
* Where the path specified refers to a node which itself represents the
collection of objects (list, map, or array) a replace operation MUST remove
collection of objects (list, map, or array), a replace operation MUST remove
all collection entries that are not supplied in the value provided in the
`SetRequest`. An update operation MUST be considered to add new entries to
the collection if they do not exist.
* In the case that key values are specified both as attributes of a node, and
as their own elements within the data tree, update or replace operations
that modify instances of the key in conflicting ways MUST be considered an
error. The target MUST return a status code of `InvalidArgument (3)`.
* In OpenConfig, a path which refers to a keyed YANG list node for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this should live here, but rather in a specific document that covers OpenConfig -- but I'm not quite sure that this is something that we want to enforce here, based on the question above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above question refers to modifying a specific list element, whereas this question refers to modifying a whole list, so I think they're unrelated from one another.

modification (as opposed to an element of the list node) MUST terminate at
the [enclosing
container](https://github.com/openconfig/public/blob/master/doc/openconfig_style_guide.md#list)
element.

For example, consider a tree corresponding to the examples above, as illustrated
below.
Expand Down