code review 169240043: go.net/webdav: new Handler, FileSystem, LockSystem and ...

go.net/webdav: new Handler, FileSystem, LockSystem and lockInfo types.

[nigeltao, 2014-11-07 07:13:30]

Hello nmvc@google.com (cc: bradfitz@golang.org, dave@cheney.net,
dr.volker.dobler@gmail.com, golang-codereviews@googlegroups.com),

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

[nmvc, 2014-11-09 21:15:43]

https://codereview.appspot.com/169240043/diff/80001/webdav/lock.go
File webdav/lock.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/lock.go#newcode28
webdav/lock.go:28: Confirm(path string, conditions ...Condition) (c io.Closer,
err error)
Some comment perhaps to indicate how multiple conditions are considered [i.e.
AND or OR]?

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode222
webdav/webdav.go:222: w.Header().Set("Lock-Token", "<"+token+">")
It is in the spec, see the definition of Coded-URL:
http://www.webdav.org/specs/rfc4918.html#HEADER_DAV

  Coded-URL        = "<" absolute-URI ">" 
                     ; No linear whitespace (LWS) allowed in Coded-URL
                     ; absolute-URI defined in RFC 3986, Section 4.3 

And then, based on that the definition of the Lock-Token header:

  Lock-Token = "Lock-Token" ":" Coded-URL

It happens that lock tokens are absolute URIs with no spaces.

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode233
webdav/webdav.go:233: // It's not explicit in the spec, but section 9.11.2 seems
to imply that
They should be stripped as in the lock-info the token XML encoding is without
the brackets; but the Lock-Token header is defined to always include them.

[dave_cheney.net, 2014-11-09 22:29:14]

Some random comments collected over the last few days.

I am not a fan of the use of named return values, but this is your package, I'm
not going to harp on about it.

https://codereview.appspot.com/169240043/diff/80001/webdav/file.go
File webdav/file.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/file.go#newcode14
webdav/file.go:14: Mkdir(path string, perm os.FileMode) error
What about calling this MkdirAll for symmetry with RemoveAll below.

What we found in pkg/sftp is we needed to specify that when talking about paths,
they had to be paths created by the `path` package, not `path/filepath` to
eliminate the tedious debate about who's filepath separator won.

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode38
webdav/webdav.go:38: } else if h.LockSystem == nil {
There are some webdav operations that don't require locking, ie, all the Level 1
operations don't require locking.

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode166
webdav/webdav.go:166: if li == (lockInfo{}) {
What about making readLockInfo return a pointer, then you can check li == nil,
which feels more natural.

[nmvc, 2014-11-09 22:47:18]

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode38
webdav/webdav.go:38: } else if h.LockSystem == nil {
On 2014/11/09 22:29:13, dfc wrote:
> There are some webdav operations that don't require locking, ie, all the Level
1
> operations don't require locking.

you're correct- however trials with OSX/Win suggested to me that their inbuilt
clients are not resilient against lack of lock support. as-such I think it makes
sense to make this an error to avoid people running into it by accident.

that said, I guess an alternative is the appropriate spec response for lack of
method support.

[dave_cheney.net, 2014-11-09 23:02:41]

Sounds reasonable. In all the toy implementations I've written I've
never gone very fast past level 1 support, and just fudged the
locking. You've done the hard yards to parse all the lock conditions
so you may as well use it.


On Mon, Nov 10, 2014 at 9:47 AM,  <nmvc@google.com> wrote:
>
> https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go
> File webdav/webdav.go (right):
>
>
https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode38
> webdav/webdav.go:38: } else if h.LockSystem == nil {
> On 2014/11/09 22:29:13, dfc wrote:
>>
>> There are some webdav operations that don't require locking, ie, all
>
> the Level 1
>>
>> operations don't require locking.
>
>
> you're correct- however trials with OSX/Win suggested to me that their
> inbuilt clients are not resilient against lack of lock support. as-such
> I think it makes sense to make this an error to avoid people running
> into it by accident.
>
> that said, I guess an alternative is the appropriate spec response for
> lack of method support.
>
> https://codereview.appspot.com/169240043/

[nigeltao, 2014-11-10 00:32:46]

The CL is certainly under-commented, but I was hoping to get consensus on the
overall interface design first before taking the time to wordsmith.

https://codereview.appspot.com/169240043/diff/80001/webdav/file.go
File webdav/file.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/file.go#newcode14
webdav/file.go:14: Mkdir(path string, perm os.FileMode) error
On 2014/11/09 22:29:13, dfc wrote:
> What about calling this MkdirAll for symmetry with RemoveAll below.

On my reading, http://www.webdav.org/specs/rfc4918.html#METHOD_MKCOL says that
we want Mkdir and not MkdirAll.


> What we found in pkg/sftp is we needed to specify that when talking about
paths,
> they had to be paths created by the `path` package, not `path/filepath` to
> eliminate the tedious debate about who's filepath separator won. 

Yeah, plenty of comments to write. TODO added.

https://codereview.appspot.com/169240043/diff/80001/webdav/lock.go
File webdav/lock.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/lock.go#newcode28
webdav/lock.go:28: Confirm(path string, conditions ...Condition) (c io.Closer,
err error)
On 2014/11/09 21:15:43, nmvc wrote:
> Some comment perhaps to indicate how multiple conditions are considered [i.e.
> AND or OR]?

Done.

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode166
webdav/webdav.go:166: if li == (lockInfo{}) {
On 2014/11/09 22:29:13, dfc wrote:
> What about making readLockInfo return a pointer, then you can check li == nil,
> which feels more natural.

Eh, a zero struct seems fine to me.

https://codereview.appspot.com/169240043/diff/80001/webdav/webdav.go#newcode233
webdav/webdav.go:233: // It's not explicit in the spec, but section 9.11.2 seems
to imply that
On 2014/11/09 21:15:43, nmvc wrote:
> They should be stripped as in the lock-info the token XML encoding is without
> the brackets; but the Lock-Token header is defined to always include them.

Done.

[dave_cheney.net, 2014-11-10 05:49:26]

> https://codereview.appspot.com/169240043/diff/80001/webdav/file.go#newcode14
> webdav/file.go:14: Mkdir(path string, perm os.FileMode) error
> On 2014/11/09 22:29:13, dfc wrote:
>>
>> What about calling this MkdirAll for symmetry with RemoveAll below.

That is true, DELETE behaves like RemoveAll, but MKCOL doesn't create
intermediary collections. That is both bizarre, and something we just
have to live with

> Eh, a zero struct seems fine to me.

Fair enough.

[dave_cheney.net, 2014-11-10 06:06:49]

Some more comments, i'm really nit picking. I think what you have here is great.

https://codereview.appspot.com/169240043/diff/100001/webdav/file.go
File webdav/file.go (right):

https://codereview.appspot.com/169240043/diff/100001/webdav/file.go#newcode17
webdav/file.go:17: OpenFile(path string, flag int, perm os.FileMode) (File,
error)
Can this be stripped down a bit ? maybe read and/or write need to be retained,
but does filemode have an analogy over webdav ?

https://codereview.appspot.com/169240043/diff/100001/webdav/file.go#newcode24
webdav/file.go:24: io.Writer
I like how webdav.File extends the http.File interface to be writable. Nice.

https://codereview.appspot.com/169240043/diff/100001/webdav/lock.go
File webdav/lock.go (right):

https://codereview.appspot.com/169240043/diff/100001/webdav/lock.go#newcode29
webdav/lock.go:29: Confirm(path string, conditions ...Condition) (c io.Closer,
err error)
I'm interested in seeing what the implementation of this would look like.
Specifically if Conditions can be expressed as functions, rather than structs.

https://codereview.appspot.com/169240043/diff/100001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/169240043/diff/100001/webdav/webdav.go#newcode22
webdav/webdav.go:22: type Handler struct {
Several times I have stared to write, and then stopped writing, a request to add
a New style constructor.

But this is better, there is no need for a New function, the literal initialiser
syntax is perfectly good here, and as there is only one entry point that uses
this type, Handler.ServeHTTP there is only one place that you need to test the
preconditions.

I'm bookmarking this to remind myself that it is ok to use more public
structures.

https://codereview.appspot.com/169240043/diff/100001/webdav/webdav.go#newcode259
webdav/webdav.go:259: return 1 * time.Second, nil
Needs a comment about where this default comes from.

[nigeltao, 2014-11-10 23:17:15]

https://codereview.appspot.com/169240043/diff/100001/webdav/file.go
File webdav/file.go (right):

https://codereview.appspot.com/169240043/diff/100001/webdav/file.go#newcode17
webdav/file.go:17: OpenFile(path string, flag int, perm os.FileMode) (File,
error)
On 2014/11/10 06:06:48, dfc wrote:
> Can this be stripped down a bit ? maybe read and/or write need to be retained,
> but does filemode have an analogy over webdav ?

The intent is that each method maps exactly to the package os method of the same
name (and same arguments).

https://codereview.appspot.com/169240043/diff/100001/webdav/lock.go
File webdav/lock.go (right):

https://codereview.appspot.com/169240043/diff/100001/webdav/lock.go#newcode29
webdav/lock.go:29: Confirm(path string, conditions ...Condition) (c io.Closer,
err error)
On 2014/11/10 06:06:48, dfc wrote:
> I'm interested in seeing what the implementation of this would look like.
> Specifically if Conditions can be expressed as functions, rather than structs.

Maybe, but functions would probably need defining new (abstract?) types for
their arguments, and we already have an abstraction, called LockSystem.

In any case, we'll see what the implementation looks like in a subsequent code
review.

https://codereview.appspot.com/169240043/diff/100001/webdav/webdav.go
File webdav/webdav.go (right):

https://codereview.appspot.com/169240043/diff/100001/webdav/webdav.go#newcode259
webdav/webdav.go:259: return 1 * time.Second, nil
On 2014/11/10 06:06:49, dfc wrote:
> Needs a comment about where this default comes from.

It's just a placeholder. There's nothing special about this value.

[dave_cheney.net, 2014-11-10 23:24:19]

LGTM.

On Tue, Nov 11, 2014 at 10:17 AM,  <nigeltao@golang.org> wrote:
>
> https://codereview.appspot.com/169240043/diff/100001/webdav/file.go
> File webdav/file.go (right):
>
> https://codereview.appspot.com/169240043/diff/100001/webdav/file.go#newcode17
> webdav/file.go:17: OpenFile(path string, flag int, perm os.FileMode)
> (File, error)
> On 2014/11/10 06:06:48, dfc wrote:
>>
>> Can this be stripped down a bit ? maybe read and/or write need to be
>
> retained,
>>
>> but does filemode have an analogy over webdav ?
>
>
> The intent is that each method maps exactly to the package os method of
> the same name (and same arguments).
>
> https://codereview.appspot.com/169240043/diff/100001/webdav/lock.go
> File webdav/lock.go (right):
>
> https://codereview.appspot.com/169240043/diff/100001/webdav/lock.go#newcode29
> webdav/lock.go:29: Confirm(path string, conditions ...Condition) (c
> io.Closer, err error)
> On 2014/11/10 06:06:48, dfc wrote:
>>
>> I'm interested in seeing what the implementation of this would look
>
> like.
>>
>> Specifically if Conditions can be expressed as functions, rather than
>
> structs.
>
> Maybe, but functions would probably need defining new (abstract?) types
> for their arguments, and we already have an abstraction, called
> LockSystem.
>
> In any case, we'll see what the implementation looks like in a
> subsequent code review.
>
> https://codereview.appspot.com/169240043/diff/100001/webdav/webdav.go
> File webdav/webdav.go (right):
>
>
https://codereview.appspot.com/169240043/diff/100001/webdav/webdav.go#newcode259
> webdav/webdav.go:259: return 1 * time.Second, nil
> On 2014/11/10 06:06:49, dfc wrote:
>>
>> Needs a comment about where this default comes from.
>
>
> It's just a placeholder. There's nothing special about this value.
>
> https://codereview.appspot.com/169240043/

[nigeltao, 2014-11-11 06:47:09]

*** Submitted as
https://code.google.com/p/go/source/detail?r=4c0f0f26ee6f&repo=net ***

go.net/webdav: new Handler, FileSystem, LockSystem and lockInfo types.

LGTM=dave
R=nmvc, dave
CC=bradfitz, dr.volker.dobler, golang-codereviews
https://codereview.appspot.com/169240043