code review 178930043: x/net/webdav: handle the absence of the If header.

x/net/webdav: minor fixes to ensure RFC/litmus compliance.

- Handle the absence of the If header.
- Reject Mkcol requests with a body.
- Delete on non-existant item should return 404.
- Support OPTIONS request
- Ensure logger is invoked for all requests.

[nmvc, 2014-11-18 04:35:24]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.net

[nmvc, 2014-11-18 04:37:53]

Hello dave@cheney.net, dfc, nigeltao, nigeltao@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[nigeltao, 2014-12-05 03:56:24]

The CL description mentions logger twice.

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode28
webdav/webdav.go:28: // Logger is an optional error logger. If non-nil, it will
be called
This doc comment needs updating.

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode105
webdav/webdav.go:105: fi, err := h.FileSystem.Stat(r.URL.Path)
You could roll this into the next line:

if fi, err := h.FileSystem.Stat(r.URL.Path); err == nil {

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode109
webdav/webdav.go:109: allowed += ", PUT, PROPFIND"
A small thing, but using a constant string would avoid an allocation every time
we handled an OPTIONS request for a directory:

if fi.IsDir() {
  allowed = "OPTIONS, GET, HEAD, POST, DELETE, TRACE, PROPPATCH, COPY, MOVE,
LOCK, UNLOCK, PUT, PROPFIND"
} else {
  allowed = "OPTIONS, GET, HEAD, POST, DELETE, TRACE, PROPPATCH, COPY, MOVE,
LOCK, UNLOCK"
}

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode117
webdav/webdav.go:117: w.Header().Set("Allow", allowed)
s/allowed/allow/ would be consistent with the HTTP header.

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode148
webdav/webdav.go:148: return http.StatusMethodNotAllowed, err
Should this just be
return http.StatusNotFound, err
and then we don't have to bother for the os.IsNotExist check??

[nmvc, 2014-12-11 00:51:10]

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode28
webdav/webdav.go:28: // Logger is an optional error logger. If non-nil, it will
be called
On 2014/12/05 03:56:24, nigeltao wrote:
> This doc comment needs updating.

Done.

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode105
webdav/webdav.go:105: fi, err := h.FileSystem.Stat(r.URL.Path)
On 2014/12/05 03:56:24, nigeltao wrote:
> You could roll this into the next line:
> 
> if fi, err := h.FileSystem.Stat(r.URL.Path); err == nil {

Done.

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode109
webdav/webdav.go:109: allowed += ", PUT, PROPFIND"
On 2014/12/05 03:56:24, nigeltao wrote:
> A small thing, but using a constant string would avoid an allocation every
time
> we handled an OPTIONS request for a directory:
> 
> if fi.IsDir() {
>   allowed = "OPTIONS, GET, HEAD, POST, DELETE, TRACE, PROPPATCH, COPY, MOVE,
> LOCK, UNLOCK, PUT, PROPFIND"
> } else {
>   allowed = "OPTIONS, GET, HEAD, POST, DELETE, TRACE, PROPPATCH, COPY, MOVE,
> LOCK, UNLOCK"
> }

Done.

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode117
webdav/webdav.go:117: w.Header().Set("Allow", allowed)
On 2014/12/05 03:56:24, nigeltao wrote:
> s/allowed/allow/ would be consistent with the HTTP header.

Done.

https://codereview.appspot.com/178930043/diff/140001/webdav/webdav.go#newcode148
webdav/webdav.go:148: return http.StatusMethodNotAllowed, err
On 2014/12/05 03:56:24, nigeltao wrote:
> Should this just be
> return http.StatusNotFound, err
> and then we don't have to bother for the os.IsNotExist check??


Concievably the error isn't a 404 though; for example the underlying FS could
fail due to unavailability. The multistatus response should encompass this.

[gobot, 2014-12-19 05:16:36]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/178930043 is best) in the description in your
new CL.

Thanks very much.