Amartus / yang2swagger

Yang to swagger generator
Eclipse Public License 1.0
32 stars 21 forks source link

Use namespaces ONLY when the parent node is in different module (or is the "first" node) - from RFC 8040 3.5.3 #32

Closed pedrofsamaral closed 4 years ago

pedrofsamaral commented 5 years ago

From RFC 8040 "3.5.3 Encoding Data Resource Identifiers in the Request URI" it is defined to include module name on the URI as follows

... If a node in the path is defined in a module other than its parent node or its parent is the datastore, then the module name followed by a colon character (":") MUST be prepended to the node name in the resource identifier. ...

Currently the "-use-namespace" implements an extension of this requirement since it prepends the module-name to ALL nodes in the path.

Is there a way or can it be considered to add the possibility to generate exactly what is described in RFC 8040? I.e. prepend the module name ONLY when the parent node is defined in other module or in the "first" node case?

Thank you in advance, Pedro Amaral

bartoszm commented 5 years ago

Hi Pedro, I have left -use-namespace disable by default because of https://tools.ietf.org/html/rfc7951#section-4 Actually this requirement has to be handled at the code binding level (e.g. for Java customized ObjectMapper from jackson should be defined) but I've never had an usecase to think what data should be added as metadata to swagger to allow codegen folks generate code aligned with section 4. Changing behavior of URL generation to only generate module name for segments where parent is different should be straightforward. Yet, technically we are not against the RFC 8040 - module name is optional for ANY segment of URL ;)

arthurMll commented 5 years ago

Hi Bartoszm,

I do agree with Pedro that it is desirable that URI encoding could be generated as namespace qualified according to RFC8040, i.e., using namespaces only when the parent node is in different module or if it is a root node.

In fact I do not understand very well your reference to RFC7951, according to https://tools.ietf.org/html/rfc7951#section-4:

A namespace-qualified member name MUST be used for all members of a top-level JSON object and then also whenever the namespaces of the data node and its parent node are different. In all other cases, the simple form of the member name MUST be used.

Thus, if this JSON encoding rules applies, if it is quite clear that both top-level objects and also those objects which its parent node is defined in a different module, MUST be namespace-qualified both at the RESTCONF API URL and also in the JSON body objects exchanged in POST/PUT Request operations body and GET Response body messages.

Moreover, I do not find a reference in RFC8040 that states:

Yet, technically we are not against the RFC 8040 - module name is optional for ANY segment of URL ;)

pedrofsamaral commented 5 years ago

Hi Bartosz,

regarding your final statement:

Yet, technically we are not against the RFC 8040 - module name is optional for ANY segment of URL ;)

although I am getting the point in your answer from a server perspective (i.e. the RFC 8040 does not explicitly prohibit - nor make it OPTIONAL- the presence of namespace in other "segments" of URL), an implementation of a client following the RFC 8040 (i.e. namespaces only when the parent node is in different module or if it is a root node) will not work with a server following the "optional"/"omitted in RFC" approach of namespaces for all segments.

Based on this I think we cannot state we are not being against the RFC 8040 meaning (even if not explicitly stated in text).

bartoszm commented 5 years ago

Hi Pedro, Indeed my bad - there is no freedom in interpretation of the segments namespaces.

arthurMll commented 4 years ago

Is this issue solved already?

arthurMll commented 4 years ago

Hi all,

I have review the latest version patches and it seems the segments namespaces in URL is done.

I wonder if it can be also posible to have the JSON qualification according to the same rules into the operations body json definitions.

Right now for a given model which has been augmented, the JSON object does qualify the root element but not the child augmented nodes.

Hope it is clear the issue. There are some examples at: https://tools.ietf.org/html/rfc7951#section-4

Thanks in advance.

bartoszm commented 4 years ago

Arthur, I was thinking about support for JSON qualifier in the body but this might be tricky for two reason:

arthurMll commented 4 years ago

I noticed that right now the tool introduces the "x-augmentation" custom tag in the appropiate definitions. I do agree that a swagger serializer/deserializer based on that custom tag (or any other defined in the OpenAPI spec) may be the right solution to solve this.

In the meanwhile, maybe it could be cleaner to remove the root namespace qualification from the body json encoding, to do not have mixed approaches. What do you think?

Moreover, I detected another thing regarding the x-path tag values, right now the namespace qualification for the referenced URLs is complete and not consistent with the URL namespace qualification implemented for the specification's path model entries.

Example:

        x-path: >-
          /tapi-common:context/tapi-connectivity:connectivity-context/tapi-connectivity:connectivity-service/tapi-connectivity:uuid
bartoszm commented 4 years ago

fixed in 1.1.14 release

arthurMll commented 4 years ago

There is still an issue with the prefix qualification of object augmentations.

Right now it seems the tool when detecs an augment yang statement, it generates an "Augmentation" schema definition which includes the following specification extension:

  tapi.topology.ContextAugmentation2:
    type: "object"
    properties:
      topology-context:
        $ref: "#/definitions/tapi.topology.TopologyContext"
    x-augmentation:
      prefix: "tapi-topology"
      namespace: "urn:onf:otcc:yang:tapi-topology"

I assume the intention is that the any software consuming this schemas will take into account this extension in order to correctly implement the namespace prefixing.

However, I also found that the current version of the tool is able to generate well the namespace prefixing of the root objects. In these cases, the tool generates a Wrapper schema definition which does prepend the model prefix to the root property, e.g.,:

  tapi.topology.TopologyContextWrapper:
    properties:
      tapi-topology:topology-context:
        $ref: "#/definitions/tapi.topology.TopologyContext"

Then, taking the above example, I wonder why it couldn't the tool directly reference the "Wrapper" schema definitions when the augmentation is referenced by a parent object. For the above example, the parent object:

  tapi.common.Context:
    allOf:
    - $ref: "#/definitions/tapi.common.TapiContext"
    - $ref: "#/definitions/tapi.notification.ContextAugmentation1"
    - $ref: "#/definitions/tapi.topology.ContextAugmentation2"

,references the "Augmentation" object (in this case "ContextAugmentation2"), however if it would reference the "TopologyContextWrapper" the problem would be solved.

Evaluating other cases, I found that in case a retrieved object is not either a root model object, or an augmentation object, the namespace qualification is not done right neither. For instance, in case of:

/tapi-common:context/tapi-topology:topology-context/tapi-topology:topology/tapi-topology:node/tapi-topology:owned-node-edge-point/tapi-connectivity:cep-list/connection-end-point

The OAS API for that specific object looks like:

  ? /data/tapi-common:context/tapi-topology:topology-context/topology={uuid}/node={node-uuid}/owned-node-edge-point={owned-node-edge-point-uuid}/tapi-connectivity:cep-list/connection-end-point={connection-end-point-uuid}
  : get:
      ...
      responses:
        200:
          description: "tapi.connectivity.ceplist.ConnectionEndPoint"
          schema:
            $ref: "#/definitions/tapi.connectivity.ceplist.ConnectionEndPointWrapper"

And the referenced schema is:

  tapi.connectivity.ceplist.ConnectionEndPointWrapper:
    properties:
      connection-end-point:
        $ref: "#/definitions/tapi.connectivity.ceplist.ConnectionEndPoint"

it can be seen that "connection-end-point" property is not namespace-qualified as "tapi-connectivity:connection-end-point" as it would be expected according to RESTCONF.