search

netconf-wg / restconf

9 stars 4 forks source link

RW mar-22 review of draft-10 #59

Closed abierman closed 8 years ago

abierman commented 8 years ago

I think that you have addresses some of my recent comments on 09, but not all of them.

I've reproduced what I think are my outstanding review comments here which I think also apply to -10;

The first 6 questions related to section 4.8: Query Parameters:

  1. Are query parameters and paths case sensitive? I would guess so. The draft doesn't make any explicit mention of this, does it need to?
  2. Should an unexpected query parameter cause an error or should it just be ignored? For some (but not all) of the query parameters in sections 4.8.1 through 4.8.9 it indicates that an explicit error should be returned if the query parameter is used for an resource type on which it isn't supported, but section 4.8 allows vendor specific query parameters to also be defined. I think that interop might be easier if unknown query parameters are just ignored.
  3. Both the depth and fields query parameters (4.8.2 & 4.8.3) indicate that they apply to the API resource (3.3), but I wasn't sure that they served any purpose since my reading of the draft is that the api resource is just the top level and effectively fixed. I.e. it doesn't return any data from the datastore or the operations, and they need to be explicitly queried separately if required. Proposed fix:

4.8.2 & 4.8.3 Old: This parameter is only allowed for GET methods on API, datastore, and data resources. A "400 Bad Request" status-line is returned if it used for other methods or resource types.

4.8.2 & 4.8.3 New: This parameter is only allowed for GET methods on datastore, and data resources. A "400 Bad Request" status-line is returned if it used for other methods or resource types.

  1. Minor nit. It might be helpful if the options were listed in the same order as they are listed in the table in section 4.8. E.g. this would mean that the section on "filter" would need to move up above the section on "insert".
  2. Section 4.8.2: The "depth" Query Parameter The last but one paragraph reads: "By default, the server will include all sub-resources within a retrieved resource, which have the same resource type as the requested resource. Only one level of sub-resources with a different media type than the target resource will be returned."

I'm not convinced that this paragraph is correct, e.g. for a GET request on /restconf/data. By default I would expect it to return all levels of child nodes (which have a different resource type) rather than just the first layer of children.

If the depth parameter is changed to not apply to the API resource, then could this paragraph just be changed to:

New: "By default, the server will include all sub-resources within a retrieved resource."

  1. Section 8: "error-info". Should the type be anydata rather than the deprecated anyxml?

Old: anyxml error-info { description "This anyxml value MUST represent a container with zero or more data nodes representing additional error information."; } New: anydata error-info { description "This anydata value MUST represent a container with zero or more data nodes representing additional error information."; }

These remaining questions are those that I raised previously (eliding those which have been addressed in -10):

  1. Section 3.4.1 Edit Collision Detection: In -10, this has been updated to make it clear that it only applies to configuration resources which is now internally consistent.

However, it feels like the timestamp is potentially be a bit inaccurate for a REST like interface.

It is probably too late to raise this now, but I was wondering whether the server shouldn't also track operational resources using parallel timestamps/etags? Whether the config or operational timestamp/etag is returned would depend on which node had been queried and the "content" GET query parameter:

I don't understand the purpose of this last paragraph means, or what its relevance is. I suggest that either "resource definition version" is defined in 1.1.4 Terms, or this paragraph is deleted.

  1. Section 4.1. OPTIONS: The OPTIONS method is sent by the client to discover which methods are supported by the server for a specific resource (e.g., GET, POST, DELETE, etc.).

    The server SHOULD implement this method, however the same information could be extracted from the YANG modules and the RESTCONF protocol specification.

I suggest adding the sentence: The server SHOULD NOT include any body text for the OPTIONS method.

  1. Section 7.1 Error Response message: When an error occurs for a request message on a data resource or an operation resource, and a "4xx" class of status codes will be returned (except for status code "403 Forbidden"), then the server SHOULD send a response message-body containing the information described by the "errors" container definition within the YANG module Section 8. The Content-Type of this response message MUST be application/yang.errors (see example below).

    The client MAY specify the desired encoding for error messages by specifying the appropriate media-type in the Accept header. If no error media is specified, then the media type of the request message SHOULD be used, or the server MAY choose any supported message encoding format. If there is no request message the server MUST select "application/yang.errors+xml" or "application/ yang.errors+json", depending on server preference. All of the examples in this document, except for the one below, assume that XML encoding will be returned if there is an error.

i) The beginning of the error paragraph makes the description conditional on the request message being on a data resource. Should this cover other resource types as well, or otherwise what is the error handling strategy for other resource types? ii) Should the last sentence of the first paragraph be "The Content-Type of this response message MUST be a subtype of application/yang.errors (see example below). "? iii) Would it be wise to indicate that the client SHOULD specify desired encoding for error messages? iv) The second paragraph states "If no error media is specified, then the media type of the request message is used. " but this directly conflicts with "The Content- Type of this response message MUST be application/yang.errors (see example below). " v) "If there is no request message the server ..." - I would think that there must always be a request message.

In summary, what about this text instead: When an error occurs for a request message on an api, data, datastore, or operation resource, and a "4xx" class of status codes will be returned (except for status code "403 Forbidden"), then the server SHOULD send a response message-body containing the information described by the "errors" container definition within the YANG module Section 8. The Content-Type of this response message MUST be a subtype of application/yang.errors (see example below).

The client SHOULD specify the desired encoding for error messages by specifying the appropriate media-type in the Accept header. If no error media is specified, then the server SHOULD choose the error media type with the clients preferred encoding, or otherwise the server MAY choose any supported message encoding format. If there is no request message the server MUST select "application/yang.errors+xml" or "application/ yang.errors+json", depending on server preference. All of the examples in this document, except for the one below, assume that XML encoding will be returned if there is an error.

Thanks, Rob

abierman commented 8 years ago

1A -- added text clarifying query parameter requirements 1B -- did not remove API from fields and depth. fields useful and depth can be used as a quick test to see if the /restconf node exists; clarified that default for datastore is to return all child data resources

anyxml changed to anydata

C: However, it feels like the timestamp is potentially be a bit inaccurate for a REST like interface. A: Not removing it -- useful in network management and many browsers support it Changed from MUST to MAY for datastore, SHOULD to MAY for data

No tracking of operational data added; wait for opstate solutions and add later

"resource definition version" : paragraph deleted

not changing OPTIONS section; defined in HTTP not RESTCONF

changed errors to any resource type

pick XML or JSON as preferred; WG decided not to do that

updated error response handing text

abierman commented 8 years ago

issues addressed in restconf-12