Re: Working Group Last Call: Structured Headers for HTTP

Hi Julian,

Thanks for the feedback; responses below.

> On 23 Feb 2020, at 5:06 am, Julian Reschke <julian.reschke@gmx.de> wrote:
> 
> On 29.01.2020 18:11, Tommy Pauly wrote:
>> ...
> 
> Below is my (slightly late feedback).
> 
> I was going to ask that the spec to be aligned with the recent
> terminology clarification we did in the core specs (and which are not
> yet published).
> 
> In fact, this is already addressed in the editor's draft, see current
> diffs at
> <https://greenbytes.de/tech/webdav/draft-ietf-httpbis-header-structure-latest-from-previous.diff.html>.
> In encourage all participants to review these changes...
> 
> That said, the title probably should also change from "Structured
> Headers for HTTP" to something like "Structured Field Values for HTTP
> Headers and Trailers".

I'm not against that, although it's quite wordy. Anyone else have thoughts?

> I did not really review the algorithms; this essentially requires
> writing code. I'm not really happy with the statement that in case of
> conflicts, the algorithms have higher precedence than the ABNF.
> 
> 
> Other than that; I found one major issue:
> 
> In 3.3.4:
> 
>  sh-token = ( ALPHA / "\*" ) *( tchar / ":" / "/" )
> 
> This should be
> 
>  sh-token = ( ALPHA / "*" ) *( tchar / ":" / "/" )
> 
> ...right?

Yes; the glories of markdown escaping rules. Fixed.

> Minor issues:
> 
> In Section 1:
> 
> At the end of the section (top level), there should be a paragraph break
> between the two sentences (so that each Section 2, 3, and 4 are treated
> consistently)

Added.

> In Section 1.1:
> 
> "In doing so, uses the VCHAR, SP, DIGIT, ALPHA and DQUOTE rules from
> [RFC5234]. It also includes the tchar rule from [RFC7230]."
> 
> ...should say..: "In doing so, *it* uses..."

Added.

> In Section 2:
> 
> "Specify the type of the header field itself; either Dictionary (Section
> 3.2), List (Section 3.1), or Item (Section 3.3)."
> 
> Maybe sort by section number (swap Dictionary and List)?

Done.

> Further down below it talks about the letter "Q" and the letter 'q'.
> Please make the quoting consistent.

I've done that, and also changed the latter letter to avoid confusion that they are related.

> In the example spec text:
> 
>> "fooUrl" contains a URI-reference (Section 4.1 of
>> [RFC3986], Section 4.1). If its value is not a valid URI-reference,
>> that URL MUST be ignored. If its value is a relative reference
>> (Section 4.2 of [RFC3986]), it MUST be resolved (Section 5 of
>> [RFC3986]) before being used.
> 
> 1. remove ", Section 4.1"

Done.

> 2. use of "URL" comes as as surprise here, maybe just say "field value"

The intent is to ignore the individual URL, not the entire field value.

> In Section 3.1.1:
> 
> "In HTTP headers, inner lists..." - I was going to say that this seems
> to be unnecessary verbose (here and in other places), but this has
> already been fixed in the latest editor's draft.

Ack.

> In Section 3.1.2:
> 
> ABNF:
> 
> parameter     = ";" *SP param-name [ "=" param-value ]
> 
> Prose:
> 
> "...parameters are separated from their item or inner-list and each
> other by semicolons."
> 
> So there's an unfortunate mismatch between the ABNF production
> "parameter" (which already includes the ";") and the use of "parameter"
> in the prose. Maybe worth addressing in the ABNF.

Please take a look at
  https://github.com/httpwg/http-extensions/commit/b4b8f4a73

> In Section 3.2:
> 
>> Members whose value is Boolean true MUST omit that value when serialised, unless it has parameters. For example, here both “b” and “c” are true, but “c”’s value is serialised because it has parameters:
>> 
>> Example-DictHeader: a=?0, b, c=?1; foo=bar
> 
> I guess this is needed in order to avoid ambiguities, but this outcome
> is really a bit strange. Is there a summary of how we got to that
> special case somewhere (github issue?)

https://github.com/httpwg/http-extensions/issues/992

> In Section 3.3:
> 
>> An item is can be a integer (Section 3.3.1), decimal (Section 3.3.2), string (Section 3.3.3), token (Section 3.3.4), byte sequence (Section 3.3.5), or Boolean (Section 3.3.6).
> 
> Please be consistent in upper/lowercasing...

Boolean refers to a person's name; lowercasing it would be equivalent to saying that your review is reschkean, not Reschkean. Or are you suggesting we uppercase all of them (which might be reasonable, given the next comment)?

> In Section 3.3.2:
> 
>> Decimals are numbers with an integer and a fractional component. The Integer component has at most 12 digits; the fractional component has at most three digits.
> 
> Same here.

Uppercase is used to refer to the SH-type; lowercase is referring to mathematical concepts.

> In Section 4.1:
> 
> Item 2 and 3 should be swapped (to reflect the order of the definitions
> in the spec).

OK, done.

> Item 5 seems to be unreachable by definition.

If for some reasons serialise is handed a value that doesn't match one of the above (or can't be identified as such), it needs to fail.

> In Section 4.1.3:
> 
> Section references in the list should be wrapped in "(" and ")"

Done (one case)

> In Section 5:
> 
> s/draft/specification/ or "document"

Done.

> Appendix A:
> 
> Acknowledgements should be in a unnumbered appendix at the end.
> 
> That said, is mentioning exactly one WG member the right thing here?

Good point. I've added a number of folks who contributed according to Github; if others feel that they were involved to a significant degree, please ping me (privately if you wish).

I've continued to single Matthew out because his contribution was considerably greater than anyone else's.

> Appendix C:
> 
>> A generic implementation of this specification should expose the top-level parse (Section 4.2) and serialize (Section 4.1) functions
> 
> Maybe swap the two parts (Sections mentioned in reverse order really
> trip me up)

Swapped.

Thanks again,

--
Mark Nottingham   https://www.mnot.net/

Received on Sunday, 23 February 2020 23:55:43 UTC