Next message: Tim Ellison/OTT/OTI: "Section 5"
To: Juergen Reuter <reuterj@ira.uka.de>
Cc: ietf-dav-versioning@w3.org
From: "Tim Ellison/OTT/OTI" <Tim_Ellison@oti.com>
Message-ID: <OF5B60ADB4.5AC59530-ON852568E3.0055B26C@ott.oti.com>
Date: Thu, 18 May 2000 11:38:14 -0400
Subject: Re: draft-ietf-deltav04.5 now available
Juergen,
I agree with most of your comments. I have sent separate messages to the
list for areas that may require further discussion.
Tim
Juergen Reuter <reuterj@ira.uka.de>
Sent by: ietf-dav-versioning-request@w3.org
17/05/00 11:20 AM
To: "Clemm, Geoff" <gclemm@rational.com>
cc: "DeltaV (E-mail)" <ietf-dav-versioning@w3.org>, jjh@ira.uka.de,
reuterj@ira.uka.de
Subject: Re: draft-ietf-deltav04.5 now available
Hi, all!
This is a partial review of draft-ietf-deltav-versioning-04.5.htm. It
only covers those sections that are relevant for basic versioning. I
apologize for this late review and for currently having not enough time
for a full review; I also had not enough time to sort out those issues
that others may already have pointed out. Nevertheless, I hope that
this review helps to improve the protocol.
Table of Contents
-----------------
Many headlines end with a period where it probably should not occur
(at least, the "." do not occur consistently).
Section 1
---------
For the reader, it would be quite helpful to see a commented listing
or table/figure of the protocols and their features that this protocol
directly builds upon. For example, is the Bindings protocol required
if a server just wants to support basic versioning? Is it required
for advanced versioning? Is WebDAV locking required? If locking is
required, why? Version control systems usually have some kind of
built-in locking mechanism, depending on their versioning model. Is a
DeltaV server supposed to map such internal locks to externally
visible WebDAV locks (which might be difficult)? What about the
redirection resources protocol which was mentioned in early versions
of this protocol? It seems to have disappeared.
Section 1.2
-----------
> A "versioned resource" ... , it must be checked out ...
> A "working resource" ... by applying a CHECKOUT request ...
> A "revision" ... by applying a CHECKIN request ...
The intention of this section is to introduce terms. I think it is
dangerous to refer here to methods like CHECKOUT or CHECKIN which are
specified later, because this might lead to circular definitions. (In
fact, the term "versionable resource" is defined circularly; cp. my
comments to section 6.1). Therefore, I propose to prefer wording like
"checking out" rather than "applying a CHECKOUT request". None the
less, I do notice that wording like "checking out" is unprecise and
therefore may lead the reader to some misconception, depending on the
reader's a priori conception of versioning models; but I think this is
still better than refering to definitions that appear later in the
document and thereby pretending precision which is not really there.
> Predecessor, Successor
> ...
Ancestor and descendant are defined in terms of predecessor and
successor. Bearing object inheritence in mind, it would be nice to
notice that a predecessor can be viewed as an immediate ancestor and a
successor as an immediate descendent.
> Foo.htm
<sarcasm>CONGRTLS.W95?</sarcasm> :-)
> Target
DeltaV uses the concept of a single target which may be a a versioned
resource, a revision, or a working resource; this is reflected in the
Target-Selector header. Alternately, one could introduce a concept
that uses a revision-target, a versioned-resource-target and a
working-resource-target, reflected by three distinct headers. Just
wondering: what was the design rationale for the concept of a single
target?
Section 1.3
-----------
> Because this augmented BNF uses the basic production rules provided in
> Section 2.2 of [RFC2068], those rules apply to this document as well.
Except for the LWS production, the augmented BNF in section 2.1 of
[RFC2068] does *not* use any of the basic production rules provided in
section 2.2. of [RFC2068]; hence, the argumentation is weak. I
propose to change the wording e.g. into the following: "The basic
production rules provided in Section 2.2 of [RFC2068] apply to this
document as well."
> The term "protected" is placed in parentheses following the definition
> of a property that cannot up updated with a PROPPATCH request.
Typo: replace "cannot up updated" with "cannot be updated".
Btw, what does "cannot" mean? Return a 403 (Forbidden) or 409
(Conflict) as suggested in section 8.2.1 of [WebDAV] for read-only
properties?
What is the difference between a "protected" property and a
"read-only" property? Yes, I know, a "protected" property may change
e.g. as a side effect of a CHECKIN, but not via a PROPPATCH. Section
4.1 of [WebDAV] says:
> Live properties include cases where a) the value of a property is
> read-only, maintained by the server, and b) the value of the
> property is maintained by the client, but the server performs syntax
> checking on submitted values.
So, a read-only property may also change. Hence I do not see any
difference between "protected" and "read-only".
Section 2.1
-----------
> An unversioned resource that can be put under version control is
> called a versionable resource.
What does "can be put under version control" mean? For a suggestion,
see my comments to section 6.1.
Section 2.2
-----------
> The default target is also selected for any versioned resource other
> than the one identified by the request-URL.
For example?
Section 2.2.1
-------------
> A working resource id does not distinguish working resources from
> different versioned resources ... since the same working resource id
> can be assigned ...
This probably should read: "A working resource id does not need to
distinguish working resources from different versioned resources
... since the same working resource id may be assigned ..."
Section 2.3
-----------
Could you give an example? Suppose you perform a LOCK request on a
versioned resource, and the Target-Selector header of that request
contains a label for some revision. Is this equivalent with LOCKing
the stable URL of that revision?
Section 3
---------
Cp. section 1.3.
Section 3.1.2
-------------
> When an id is marshaled in the header of an HTTP request, the
> characters are encoded using the UTF-8 encoding scheme.
It may be worth noting that, according to the syntax of [HTTP/1.1],
the encoded message header field may not contain any CTL character
except LWS.
Section 3.1.4
-------------
The href element is frequently used throughout WebDAV and should be
known to the reader of this protocol, which is an extension to WebDAV;
hence I think this section can be dropped.
Section 3.2
-----------
WebDAV introduces a format to specify properties: it presents a table
with name, namespace, purpose, value, and description of each
property. It would be nice to continue this format throughout
sections 3.2 ... 3.5, maybe extended by an additional item "type" that
distinguishes between "protected", "read-only", and "read/write".
Section 3.2.1
-------------
> This property is used to track the author of a resource. Each revision
> of a versioned resource can have a different DAV:author values,
> indicating who authored that revision.
This sounds as if DAV:author is a revision property, but not a
versioned resource property or working resource property. Then this
section should be moved to section 3.4. Or is the property DAV:author
available on revisions, versioned resources and working resources?
Then, this should be clearly stated, e.g. by providing a definition of
"resource property" in the introduction of section 3.2, for example:
"For the purpose of this protocol, a resource property is defined to
be a property as specified in [RFC2518] that is available on versioned
resources, revisions and working resources."
Section 3.2.2
-------------
See section 3.2.1. The same applies here.
Section 3.3
-----------
> The DAV:resourcetype of a versioned resource ...
This probably should read: "The value of the DAV:resourcetype property
of a versioned resource ..."
Section 3.3.1
-------------
> <!ELEMENT revision-set (href*)>
If a versioned resource may be empty (i.e. contain no revision), then
"(href*)" seems appropriate. Otherwise, I would like to vote for
"(href+)". Section 2.1 of this protocol should clarify if a versioned
resource may be empty.
> <!ELEMENT working resource id-set (working-resource-id*)>
Typo: replace this with:
"<!ELEMENT working-resource-id-set (working-resource-id*)>"
> <!ELEMENT working-resource-id (#PCDATA)
Typo: missing ">": "<!ELEMENT working-resource-id (#PCDATA)>"
Section 3.4
-----------
> ... all revision properties except ... are immutable.
"immutable" should be defined somewhere and compared with "read-only"
and "protected". "immutable" implies "protected", but not vice-versa,
right? "immutable" implies "read-only", but not vice-versa, right?
Section 3.4.2
-------------
> Since a revision id can be specified in a target selector header, the
> length of a revision id should be compatible with common header length
> constraints.
Could you give a reference for "common header length constraints"?
Were you thinking of section 3.4.8 of RFC 822?
Section 3.4.3
-------------
> The DAV:predecessor-set of a revision contains a stable URL for the
> revision that was checked out to produce this revision. The
> DAV:predecessor-set property can contain multiple revisions if the
> advanced versioning MERGE method is supported.
>
> <!ELEMENT predecessor-set (href*)>
As far as I understand, the DAV:predecessor-set for the initial
revision contains no href element, which should be mentioned.
Otherwise, the predecessor-set element should be defined as "<!ELEMENT
predecessor-set (href+)>". How about the following wording: "The
DAV:predecessor-set of a revision contains a stable URL for each
revision that is a predecessor as specified in section 1.2. Except
for the initial revision which does not have any predecessor, each
revision either has a single predecessor that was checked out to
create this revision, or it has multiple predecessors which it was
merged from."
Section 3.4.4
-------------
> This property contains a stable URL for each of the revisions whose
> DAV:predecessor-set identifies this revision.
This probably should read: "This property contains a stable URL for
each of the revisions whose DAV:predecessor-set contains a URL that
identifies this revision."
Section 3.4.5
-------------
> Value: id
Yes, I know, I am nitpicking, but the "value" item usually refers to
the property's value, while here it refers to the label element, which
itself is the property's value. Hence, correctly, it should be read:
<!ELEMENT label-set (label*)>
Value: <!ELEMENT label (#PCDATA)>, with the child content of this
element representing an id.
> ... common header length constraints ...
See section 3.4.2.
> For efficiency ... may report only a subset of the labels ...
Oops! Then there is, just because of efficiency reasons, no reliable
way to recover all labels, even if I have full access rights? I
think, this s a flaw.
Section 3.4.7
-------------
> Value: id
Cp. section 3.4.5.
Section 3.5.2
-------------
Cp. section 3.4.3.
Section 4
---------
It should be mentioned that all versioning headers obey the message
header format as specified in section 4.2 of [HTTP/1.1]. Otherwise,
the field-name, for example, does not need to be a token, but could be
regarded as a literal string. But that would mean that there is no
implied LWS between the field-name and the ":" (just as, for example,
there is no implied LWS in the HTTP-Version production in section 3.1
of [HTTP/1.1]). Section 4.2 of [HTTP/1.1] also explicitly allows LWS
between the ":" and the field-value.
I would like to propose the following introduction in section 4: "This
section defines some header fields additional to those defined in
[WebDAV] and [HTTP/1.1]. They follow the format and conventions
defined in section 4.2 of [HTTP/1.1]."
Section 4.1
-----------
> ... there is no target for the versioned resource (primarily used with
> the SET-TARGET method).
For the reader's convenience, it would be nice to add a reference
like: "... with the SET-TARGET method; see section 6.2)."
Section 5
---------
> Note that if a method modifies only a binding to a resource and not
> the resource itself (e.g. DELETE, MOVE, BIND, UNBIND), the method is
> being applied to the collection containing that binding, so the method
> is not affected by versioning unless that collection is under version
> control.
Ok, but what happens, for example, when you try to DELETE a stable
URL? Does that mean that the associated revision is to be deleted?
Trying to MOVE a stable URL should fail (e.g. 405 (Method not
allowed)), right? The same questions arise when you apply DELETE or
MOVE on a versioned resource whose target is a revision.
Section 5.2
-----------
> When PROPFIND is applied to a versioned resource...
This probably should read: "When PROPFIND is applied to a binding that
binds a versioned resource..."
> When PROPFIND is applied to a stable URL for a revision, the DAV:urn
> for the revision will be returned.
This probably should read: "When PROPFIND is applied to a binding that
binds a stable URL for a revision, the DAV:urn for the revision will
be returned."
Section 5.4
-----------
> When a resource supports core versioning, the DAV response header for
> an OPTIONS request MUST contain "VERSION".
In the DAV response header, WebDAV uses a production called "extend"
which is actually undefined. This has been put on the WebDAV issues
list. On the w3c-dist-auth mailing list, there was a proposal to
define:
extend = "<" absoluteURI ">"
However, the literal "VERSION" would not satisfy this production. I
strongly recommend to achive agreement about the extend production (at
least on the w3c-dist-auth mailing list) before using it.
Section 5.5
-----------
> 4xx (No Such Target)
Just to remind to: Do not forget to finally assign a number for this
error. Btw, in section 6, you may want to list this error in the
response status code sections, where appropriate.
Section 6.1
-----------
> Preconditions:
> The request-URL MUST identify a versionable resource.
This is a circular dependency, because in section 1.2 "versionable
resource" is defined as "resource that can be placed under version
control with a VERSION request".
To break this circularity, I would like to propose the following
wording for the definition of "versionable resource" in section 1.2:
"any resource that evolves over time, that can be tracked in a
history, and that has not yet been put under version control (or, in
other words, has no versioned resource associated)".
> Return Status Codes:
Why do you put the return status codes between the preconditions and
postconditions? I would expect them behind the postconditions,
because the postconditions are only relevant for the 200 (Ok) status,
and hence the return status codes altogether present a more global
view on what may happen and thus should be outside the scope of the
pre-/postconditions block.
Section 6.2
-----------
> SET-TARGET
Although "SET-TARGET" is a valid token as required by the
extension-method production in section 5.1.1 of [HTTP], I do not know
of any method name that uses a hyphen to divide words (for example,
cp. "MKCOL"). Is there a specific reason to break this silent
convention?
> A SET-TARGET request can be applied to a versioned resource whose
> target is a revision to specify that revision to be the target when
> the specified label is used in a Target-Selector header.
Ugly wording -- hard to understand! What about the following: "A
SET-TARGET request can be applied to a versioned resource to specify a
revision to be the target. The label of the target must be specified
in a Target-Selector header." (Is this what you intended to say?)
> Preconditions:
Maybe you want to add: "The request must contain a valid
Target-Selector header."
> <!ELEMENT label (#PCDATA))
Typo: replace "))" with ")>".
> 404 (Not Found): The request-URL did not identify a resource.
A null resource is a resource, but I suppose 404 should be returned
also if the request-URL identifies a null resource. Hence, the
wording shoud be slightly modified.
It may be considerable to mention that 423 (Locked) might be returned.
Section 6.2.2
-------------
> <? xml version="1.0" encoding="utf-8" ?>
The white space between the "?" and "xml" is invalid; see rule [23] of
[XML1.0].
Section 6.3
-----------
> A CHECKOUT request does not require a lock token, since it does not
> modify an existing target.
Yes, but even if it did modify an existing target, it would not
require a lock token, as long as the target is not locked, right?
> 404 (Not Found): The request-URL did not identify a resource.
Problem: null resources (cp. section 6.2)
> 405 (Method Not Allowed): The request-URL did not identify a versioned
> resource whose target is a revision.
"whose" is misleading: the target is determined not only from the
versioned resource, but also from the target selector. Hence, I would
like to suggest the following wording: "The request-URL MUST identify
a versioned resource. The target of this resource as determined from
the Target-Selector header MUST be a working resource".
Section 6.4
-----------
> If the request-URL is write-locked for the specified target selector,
> the CHECKIN request MUST include the appropriate lock token.
This is (at least for me) misunderstanding. How about the following
wording: "The request-URL together with the target selector determine
a stable URL for the new revision. If this stable URL is
write-locked, the CHECKIN request MUST include the appropriate lock
token."
> If the request body of the CHECKIN request contains a
> DAV:keep-checked-out element, then instead of deleting the working
> resource, CHECKIN will modify the DAV:predecessor-set of the working
> resource to contain a stable URL for the newly created revision.
Does that imply that you can do a series of CHECKIN requests without
any intermediate CHECKOUT?
> The request-URL MUST identify a versioned resource whose target is a
> working resource.
Problem: "whose" (cp. section 6.3)
> <!ELEMENT checkin-policy (keep-checked-out|overwrite|ANY)* >
This is an invalid element type declaration: mixed content can not be
mixed with "ANY"; see rule [46] of [XML1.0].
> 201 (Created): The revision was successfully created.
Why not use 200 (Ok)? Creating a new revision occurs in conjunction
with deleting the working resource (unless keep-checked-out is set);
hence, isn't it a transport of data rather than a creation?
> 405 (Method Not Allowed): The request-URL does not identify a
> versioned resource whose target is a working resource, or the
> specified DAV:checkin-policy elements are invalid.
... or DAV:overwrite was applied on a server that does not support
mutable revisions.
Problem: "whose" (cp. section 6.3)
Section 6.5
-----------
> The request-URL MUST identify a versioned resource whose target is a
> working resource.
Problem: "whose" (cp. section 6.3)
> 404 (Not Found): The request-URL did not identify a resource.
Problem: null resources (cp. section 6.2)
> 405 (Method Not Allowed): The URL did not identify a versioned
> resource whose target is a working resource.
Problem: "whose" (cp. section 6.3)
Section 6.6
-----------
> Unlike a resource property, a report depends on more than the state of
> the resource identified by the request-URL.
Could you give an example?
Section 6.6.1
-------------
> >>RESPONSE
>
> HTTP/1.1 200 OK
>
> Host: www.webdav.org
Host is a request header only, not a response header.
Section 6.6.2
-------------
> Many properties consist of a set of one or more href elements.
Probably this should read: "Many of the properties specified in
sections 3.2 through 3.5 consist ... ". Or is property-report
designed to handle user-defined properties as well?
> The elements of a DAV:property-report identify which properties of the
> resource are to be reported.
"the resource" should be replaced with "the resources" or "each
resource".
> >>REQUEST
> ...
The example still contains identifiers from earlier versions of the
draft:
replace "<D:revisions>" with "<D:revision-set>" (2 times)
replace "<D:revisions/>" with "<D:revision-set/>" (2 times)
replace "<D:revision>" with "<D:revision-id>" (2 times)
replace "<D:revision/>" with "<D:revision-id/>" (2 times)
Section 18
----------
> All other IANA considerations mentioned in [RFC2518] also applicable
Typo: replace "applicable" with "apply".
##########################################################################
Bye,
Juergen