{"action":"closed","issue":{"url":"https://api.github.com/repos/netconf-wg/restconf/issues/63","repository_url":"https://api.github.com/repos/netconf-wg/restconf","labels_url":"https://api.github.com/repos/netconf-wg/restconf/issues/63/labels{/name}","comments_url":"https://api.github.com/repos/netconf-wg/restconf/issues/63/comments","events_url":"https://api.github.com/repos/netconf-wg/restconf/issues/63/events","html_url":"https://github.com/netconf-wg/restconf/issues/63","id":158043784,"number":63,"title":"Review of draft-ietf-netconf-restconf","user":{"login":"mnot","id":74384,"avatar_url":"https://avatars.githubusercontent.com/u/74384?v=3","gravatar_id":"","url":"https://api.github.com/users/mnot","html_url":"https://github.com/mnot","followers_url":"https://api.github.com/users/mnot/followers","following_url":"https://api.github.com/users/mnot/following{/other_user}","gists_url":"https://api.github.com/users/mnot/gists{/gist_id}","starred_url":"https://api.github.com/users/mnot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mnot/subscriptions","organizations_url":"https://api.github.com/users/mnot/orgs","repos_url":"https://api.github.com/users/mnot/repos","events_url":"https://api.github.com/users/mnot/events{/privacy}","received_events_url":"https://api.github.com/users/mnot/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/netconf-wg/restconf/labels/REVIEW","name":"REVIEW","color":"0052cc"}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2016-06-02T01:53:11Z","updated_at":"2016-07-27T04:06:42Z","closed_at":"2016-07-27T04:06:42Z","body":"_I'm not sure this is the most appropriate way to give feedback, but it's the easiest :)_\r\n\r\nSome things I noticed in a reading -13:\r\n\r\n* In `Terms`, there are a few \"resources\" that are defined as \"a resource with the media type...\". It would be more accurate to say \"a resource that has a representation with the media type...\" (because resources don't have media types in HTTP).\r\n\r\n* \"plain patch\" is defined as \"a specific PATCH request type that can be used for simple merge operations.\" What media type does it have, if it's specific? A link to the defining section would help.\r\n\r\n* > This strategy obviates the need for responses provided by the server to contain Hypermedia as the Engine of Application State (HATES) links, originally described in Roy Fielding's doctoral dissertation [rest-dissertation].\r\n\r\n  I'd say this differently; AIUI the client is able to use the data presented by NETCONF to determine the links it needs.\r\n\r\n* > HTTP/1.1 pipelining MUST be supported by the server.  Responses MUST be sent in the same order that requests are received.\r\n\r\n  It's not appropriate to re-state requirements from HTTP; if it's a HTTP/1.1 server, it will support pipelining. Also, see below RE: versions.\r\n\r\n*  > No other versions of HTTP are supported for use with RESTCONF.\r\n\r\n  As noted elsewhere, this seems needlessly restrictive, and probably impossible to enforce. HTTP versioning is hop-by-hop, so a client behind a proxy can't control the version of HTTP presented to the server, and vice versa.\r\n\r\n* > If the RESTCONF client is not authorised to access a resource, the server MUST send an HTTP response with \"401 Unauthorised\" status-line\r\n\r\n  401 is used for authentication challenges, not authorisation. It might equally be appropriate to present 403 Forbidden (especially if client cert authentication is in use, since it doesn't (currently) use 401). Also, there are situations where this MUST can be reasonably violated, e.g., if a 400, 500, 501, etc. status code is more appropriate; it needs to be a SHOULD.\r\n\r\n* > A resource has a media type identifier, represented by the \"Content-Type\" header in the HTTP response message.\r\n\r\n  As elsewhere, resources don't have media types. \r\n\r\n* In a few examples, `Server example.com` -- missing colon\r\n\r\n* `ETag: a74eefc993a2b` -- missing quotes around the etag-value\r\n\r\n* Same example - `Pragma` is a request header field, it is not valid in responses.\r\n\r\n* > Note that the way that access control is applied to data resources is completely incompatible with HTTP caching.  The Last-Modified and ETag headers maintained for a data resource are not affected by changes to the access control rules for that data resource.  It is possible for the representation of a data resource that is visible to a particular client to be changed without detection via the Last- Modified or ETag values. \r\n\r\n  It's not \"completely incompatible\"; as described below that passage, responses can be cached, but need to be validated before being used to satisfy a request (i.e., `Cache-Control: no-cache` semantics). I also strongly suspect you could assign a freshness lifetime if appropriate `Vary` or `Key` headers are sent.\r\n\r\n* > The DELETE method is used to delete the target resource.  If the DELETE request succeeds, a \"204 No Content\" status-line is returned, and there is no response message-body.\r\n\r\n  Other users of these resources might find a response body helpful (e.g., negotiated by `Prefer`); it shouldn't be prohibited (while there's no 2119 language here, some might interpret it as such). Also, \"there is no response message-body\" is true for 204, it's best to leave the definition of what does and does not have a message body to RFC7230 (as this is an area rife with bugs and interop problems).\r\n\r\n* > query: the set of parameters associated with the RESTCONF message. These have the familiar form of \"name=value\" pairs.\r\n\r\n  This is very informal; the syntax you're referring to is defined by HTML5, not HTTP or URIs, so you need to refer to that spec.\r\n\r\n* > fragment: This field is not used by the RESTCONF protocol.\r\n\r\n  It would be good if the media types clearly specify their fragment identifier semantics where possible, so that future uses can leverage them, even if this protocol doesn't use them.\r\n\r\n* > Since the datastore contents change at unpredictable times, responses from a RESTCONF server generally SHOULD NOT be cached.\r\n\r\n  See above re: caching.\r\n\r\n* > A \"Pragma\" header specifying \"no-cache\" MAY also be sent in case the \"Cache-Control\" header is not supported.\r\n\r\n  See above re: appropriate use of `Pragma`.\r\n\r\n* > Instead of relying on HTTP caching, the client SHOULD track the \"ETag\" and/or \"Last-Modified\" headers returned by the server for the datastore resource (or data resource if the server supports it).\r\n\r\n* > HTTP status-lines are used to report success or failure for RESTCONF operations.\r\n\r\n  Status codes, not status lines. The status line includes the reason phrase, which is not guaranteed to persist end-to-end.\r\n\r\n* > The following table summarizes the return status codes used specifically by RESTCONF operations\r\n\r\n  This table makes me really uncomfortable; it will be read by many as redefining / specialising the semantics of HTTP status codes. \r\n\r\n* The parent MIME media type for RESTCONF resources is application/yang, which is defined in [RFC6020].  Has this use of the \".\" syntax been discussed with the media types community? To date, that's been used only to indicate a registration tree, which this is not.\r\n\r\n* The structure of the document makes it hard to find the definitions of the individual link relation types' semantics (reflected by the resources they identify), and the syntax and semantics of the media types defined. These are first-class artefacts in HTTP-based protocols.\r\n\r\n* It's a bit of a pity that host-meta is just used to discover the entry point; it could have been used to discover more of the API's resources (or templates for them), allow more flexibility in deployment, and the ability to layer in new resources without versioning the whole protocol.\r\n\r\n* Finally, \"RESTCONF\" is a horrible name; is it too late to change it to \"NETCONF-HTTP\"?"}}
{"actor":{"display_login":"abierman"}}