CZ-NIC/yangson

New `expand_keys=True` doesn't account for keyless opstate lists

Closed this issue · 12 comments

kwatsen commented

Here's an example: (Note: error=/ should be something like error[0]/)

self = <yangson.schemanode.LeafNode object at 0x1140f5400>, inst = <yangson.instance.ObjectMember object at 0x114141610>, scope = <ValidationScope.all: 3>
ctype = <ContentType.all: 3>                                                      
                                                                                  
    def _validate(self: "TerminalNode", inst: InstanceNode, scope: ValidationScope,
                  ctype: ContentType) -> None:
        """Extend the superclass method."""
        if (scope.value & ValidationScope.syntax.value and
                inst.value not in self.type):                                     
>           raise YangTypeError(inst.json_pointer(expand_keys=True), self.type.error_tag,
                                self.type.error_message)
E           yangson.exceptions.YangTypeError: [/ietf-restconf:errors/error=/error-type] invalid-type: expected enumeration

.tox/py38/lib/python3.8/site-packages/yangson/schemanode.py:955: YangTypeError
llhotka commented

Hmm, according to the documentation, json_pointer method returns JSON Pointer as per RFC 6901. With expand_keys=True this is no more the case.

I know we have issue #86, but I wanted to fix it by adding a method like follow_json_pointer tak would return the instance pointed to by a JSON pointer.

kwatsen commented

Hi Lada,

I agree that the expand_keys param generates invalid json-pointers, which caused Issues #86 and #95. I can back out that change and introduce a separate mechanism for more readable exception messages, but there are issues with resource-identifiers:

  1. resource identifiers cannot encode keyless list entries (this is a blocker)
  2. resource identifiers cannot uniquely identify duplicate leaf-list values (this is a nice-to-have)

We could still define an API to produce a Resource-ID, but it would have to throw an exception for keyless list entries. That said, relying on Resource-IDs seems unacceptably spotty and perhaps should be avoided, even if the code is already written?

Perhaps Yangson should use instance-identifiers instead, for exceptions? Instance-IDs are more verbose than Resource-IDs, but still more readable than json-pointers, as most array indexes are converted to key values, though array indexes are still used for keyless lists. This resolves the blocker (1). As for the nice-to-have (2), Instance-IDs are still unable to uniquely identify duplicate leaf-list values (AFAICT, as 7950 doesn't say explicitly, but it follows as no alternate leaf-list encoding is offered). But this seems okay to me, as from the exception message POV, as 1) it's only an issue when there's a duplicate and 2) seeing the value is more important than knowing which array entry the value appeared in.

To apply the change, perhaps the following, leaving it to the caller to pass a jptr or an instance-ID on a case-by-case basis?

OLD: def __init__(self: "InstanceException", path: JSONPointer, message: str):
NEW: `def init(self: "InstanceException", path: string, message: str):

Thoughts?

llhotka commented

I created PR #97 that redefines InstanceException and ValidationError (their sublasses are also affected) so that they now have the offending InstanceNode as their member, instead of JSONPointer. This should presumably alleviate the need for the expand_keys argument and we could then remove it.

@kwatsen @HRogge please check the PR. The change could potentially break exception handlers, but it should be easy to update them.

kwatsen commented

I commented on the PR. Yes, it is on the path towards removing the expand_keys param (which I have a PR ready to submit to do), but it remains the case that InstanceException.__str__() is returning a JSONPointer.

I've written code for InstanceException.instance_id() and would like to modify the code as follows:

OLD:

    def __str__(self: "InstanceException"):
        return f"[{self.instance.json_pointer()}] {self.message}"

NEW:

    def __str__(self: "InstanceException"):
        return f"[{self.instance.instance_id()}] {self.message}"

I understand that my exception handlers could call instance_id() themselves, but this seems like a better default as it is 1) more readable and 2) not JSON-specific.

Thoughts?

llhotka commented

Fixed by PR #97 (merged in 1.4.0)

llhotka commented

In version 1.4.2 all instance-related error messages use instance identifiers instead of JSON pointers. I also had to use curly braces {...} for delimiting the I-Is as they may contain [...] brackets. I hope it's OK.

kwatsen commented

Regarding curly-braces, is it legal to do that?

Also, after fetching/merging upstream, pytest tests/test_model.py throws:

E       AttributeError: 'RootNode' object has no attribute 'instance_route'

Perhaps I should try a fresh clone?

llhotka commented

Regarding curly-braces, is it legal to do that?

What do you mean by (not) being legal? It is no code, just error message.

llhotka commented

Also, after fetching/merging upstream, pytest tests/test_model.py throws:

E       AttributeError: 'RootNode' object has no attribute 'instance_route'

Perhaps I should try a fresh clone?

This is weird, I don't see this problem. The instance_route method is defined in InstanceNode class, and RootNode is its subclass. a Fresh clone might help.

kwatsen commented
kwatsen commented
llhotka commented

Okay. I was trying to run the pytest to see for myself what was going on, but the intent is that instance_route() returns a valid RFC 7951 instance identifier, correct?

Yes, braces are just used to separate the instance identifier from the rest of the error message.

Also, could you explain why curly braces were needed for the “error message”?

Because instance identifiers may contain XPath predicates in square brackets, such as [number="8"]. I thought it was better not to use brackets as external delimiters. See the doctest snippets in this section.