Discussion:
[core] Adam Roach's Discuss on draft-ietf-core-links-json-07: (with DISCUSS and COMMENT)
Adam Roach
2017-04-21 23:11:46 UTC
Permalink
Adam Roach has entered the following ballot position for
draft-ietf-core-links-json-07: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-core-links-json/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

- The protocol has technical flaws that will prevent it from working
properly, or the description is unclear in such a way that the reader
cannot understand it without ambiguity.

Ambiguity 1: Section 1.1 describes "round-tripping" between 6690 format
and JSON/CBOR; however, the document describes the transform in one
direction, leaving the other implicit. The non-normative code in Appendix
A shows a conversion into link relation format, but it makes certain
assumptions about the syntax of link relation parameters that, while
possibly true today, are not guaranteed to hold true in the future. As
defined in this document, the transform requires parameter-specific
syntax knowledge of each link relation parameter that it will transform.
The problem this causes is probably best demonstrated with an example.

Let's say some future document defined a link relation ("smellslike")
that included a parameter with ABNF syntax like:

"smellsys" "=" quoted-string

Now, if we were to apply the transform in this document to the following
link relation:

</smellr2>;smellsys="h9341";rel="smellslike"

We get:

[{"href":"/smellr2", "smellsys":"h9341","rel":"smellslike"}]

Applying the logic that is implicit in the document and explicit (if
non-normative) in Appendix A, converting this back into a link relation
yields:

</smellr2>;smellsys=h9341;rel=smellslike

Note the stripping of double quotes from the parameter values. This is
fine for "rel", whose ABNF syntax allows for the presence or absence of
quotes -- but the emitted syntax for "smellsys" is actually in violation
of its defined ABNF.

This needs to be addressed by either indicating that conversion into link
relations requires that the conversion routine has explicit knowledge of
the syntax for each parameter it converts (and must fail to convert
parameters it does not understand), *or* the JSON and CBOR
representations need some additional indication that preserves knowledge
of whether the indicated value is required to be quoted when converted to
a link relation. If the first path is taken, the associated Ruby code
needs to be updated accordingly.

Ambiguity 2: The document requires that the thirteen defined values MUST
be encoded as integers. The document does not define what implementations
are to do if they receive a CBOR object that does not conform to this
encoding: is the parameter ignored? Is the entire link relation ignored?
Do you reject the entire collection of link relations? Or do you just go
ahead and parse it anyway, since the intended meaning is ambiguous (even
if out of spec)?

- The draft omits a normative reference necessary for its
implementation, or cites such a reference merely informatively rather
than normatively.

This document appears to use CDDL to define the formal schema for both
the JSON and CBOR representations of its data format, although the CDDL
document itself is cited only informatively. Additionally , figure 1
shows an application of CDDL to define schema for JSON. It's not clear
from a skim through the CDDL document that it can be used for JSON; it
would appear that using it in this fashion would require additional text
in this document to talk about how to apply CDDL to JSON, or waiting for
some other document to do so.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

In section 2.2, the phrase "outer collection" is a bit difficult to
understand at first (on first reading, the sense of the word "outer" is
unclear); recommend a more descriptive phrase like "The collection of
link relations is represented as a JSON or CBOR array of links"

The first paragraph on page 5 is using normal English prose to describe
normative behavior, which is fine; however it plainly says that the
encoding is normal JSON name/value pairs, and that "the value is a
string," full stop, no exceptions. Further down, this section says the
value can be a boolean, which effectively contradicts the earlier
statement. Even further down, it says the value can be an array of
strings. It would be much easier to understand if the initial statement
said "the value can be a string, a boolean, or an array of strings, as
described below," and then have each of the three situations described
independently.

Both of the CDDL schemata seem to omit support for array values (i.e., if
a name appears twice in a link relation, as shown in Figure 4).

Section 2.3 opens with "The above specification could be used as is..."
-- there is a risk that implementors might read this as permission rather
than hypothetical musing. Suggest rephrasing like "...might have been
able to be used as-is..."

Table 1 reads is introduced with the phrase "The substitution is
summarized below," which had me looking for the actual definition
elsewhere in the document. Not finding one, I conclude this table
defines, rather than summarizes, the substitution. (Replace "summarized"
with "defined").

One would presume the paragraph below table 1 should be removed.

The example in Figure 6 would benefit greatly by showing both the array
encoding and "foo" encoding used in Figure 4 (including, in particular,
the string -- rather than integer -- encoding of the "foo=3" parameter).
Carsten Bormann
2017-04-22 08:03:47 UTC
Permalink
Hi Adam,

thank you very much for this deep review.
This will allow us to improve the document a lot.

Let me pick one issue here that I think goes beyond the details of links-json.
Post by Adam Roach
Let's say some future document defined a link relation ("smellslike")
"smellsys" "=" quoted-string
My knee-jerk reaction is “then don’t do that”.

But the more fundamental problem is that RFC 5988 actually defines a data model and its serialization without ever saying so. Any reasonably defined link parameter will either have a syntax that is limited to ptoken or allow both ptoken and quoted-string serialization.
The fact that we then use ABNF to define the link parameter syntax obscures that property and allows the above deviation. We already made the mistake for [anchor, title, rt, if] (see MUSTBEQUOTED in the reference implementation). There is no need to make it again, but the use of ABNF for what should be a data model definition does make it too easy to shoot ourselves in the foot.

Note that the above issue hits any implementation that wants to generate RFC 5988 syntax, not just one that converts links-json into RFC 5988 syntax.

So my takeaway summary is that links-json only works for reasonably defined link parameters (after grandfathering in anchor, title, rt, if) and probably should say so much more explicitly. But I don’t know how to fix RFC 5988 about this issue.

Grüße, Carsten
Carsten Bormann
2017-04-22 08:32:47 UTC
Permalink
Interesting.
Julian Reschke reminds me that this is repaired in draft-nottingham-rfc5988bis-05.txt:

3. Link Serialisation in HTTP Headers

The Link header field provides a means for serialising one or more
links into HTTP headers.

The ABNF for the field value is:

Link = #link-value
link-value = "<" URI-Reference ">" *( OWS ";" OWS link-param )
link-param = token BWS "=" BWS ( token / quoted-string )

Note that any "link-param" can be generated with values using either
the "token" or the "quoted-string" syntax, and therefore recipients
MUST be able to parse both forms. Individual "link-param"s specify
their syntax in terms of the value after any necessary unquoting (as
per [RFC7230], Section 3.2.6).

Unfortunately, this doesn’t retroactively fix RFC 6690, which still has restrictive ABNF for rt, if, title, anchor. Hmm.

Grüße, Carsten
Post by Carsten Bormann
Hi Adam,
thank you very much for this deep review.
This will allow us to improve the document a lot.
Let me pick one issue here that I think goes beyond the details of links-json.
Post by Adam Roach
Let's say some future document defined a link relation ("smellslike")
"smellsys" "=" quoted-string
My knee-jerk reaction is “then don’t do that”.
But the more fundamental problem is that RFC 5988 actually defines a data model and its serialization without ever saying so. Any reasonably defined link parameter will either have a syntax that is limited to ptoken or allow both ptoken and quoted-string serialization.
The fact that we then use ABNF to define the link parameter syntax obscures that property and allows the above deviation. We already made the mistake for [anchor, title, rt, if] (see MUSTBEQUOTED in the reference implementation). There is no need to make it again, but the use of ABNF for what should be a data model definition does make it too easy to shoot ourselves in the foot.
Note that the above issue hits any implementation that wants to generate RFC 5988 syntax, not just one that converts links-json into RFC 5988 syntax.
So my takeaway summary is that links-json only works for reasonably defined link parameters (after grandfathering in anchor, title, rt, if) and probably should say so much more explicitly. But I don’t know how to fix RFC 5988 about this issue.
Grüße, Carsten
_______________________________________________
core mailing list
https://www.ietf.org/mailman/listinfo/core
Alexey Melnikov
2017-04-24 09:00:12 UTC
Permalink
Carsten and Adam,
Post by Carsten Bormann
Interesting.
3. Link Serialisation in HTTP Headers
The Link header field provides a means for serialising one or more
links into HTTP headers.
Link = #link-value
link-value = "<" URI-Reference ">" *( OWS ";" OWS link-param )
link-param = token BWS "=" BWS ( token / quoted-string )
Note that any "link-param" can be generated with values using either
the "token" or the "quoted-string" syntax, and therefore recipients
MUST be able to parse both forms. Individual "link-param"s specify
their syntax in terms of the value after any necessary unquoting (as
per [RFC7230], Section 3.2.6).
I think this document should recognize this change and just reference draft-nottingham-rfc5988bis normatively. (I will be AD sponsoring it shortly anyway.) Adam, will this address this concern
Post by Carsten Bormann
Unfortunately, this doesn’t retroactively fix RFC 6690, which still has restrictive ABNF for rt, if, title, anchor. Hmm.
Well, we just have to special case them. The list of special attributes is fixed.
Post by Carsten Bormann
Grüße, Carsten
Post by Carsten Bormann
Hi Adam,
thank you very much for this deep review.
This will allow us to improve the document a lot.
Let me pick one issue here that I think goes beyond the details of links-json.
Post by Adam Roach
Let's say some future document defined a link relation ("smellslike")
"smellsys" "=" quoted-string
My knee-jerk reaction is “then don’t do that”.
But the more fundamental problem is that RFC 5988 actually defines a data model and its serialization without ever saying so. Any reasonably defined link parameter will either have a syntax that is limited to ptoken or allow both ptoken and quoted-string serialization.
The fact that we then use ABNF to define the link parameter syntax obscures that property and allows the above deviation. We already made the mistake for [anchor, title, rt, if] (see MUSTBEQUOTED in the reference implementation). There is no need to make it again, but the use of ABNF for what should be a data model definition does make it too easy to shoot ourselves in the foot.
Note that the above issue hits any implementation that wants to generate RFC 5988 syntax, not just one that converts links-json into RFC 5988 syntax.
So my takeaway summary is that links-json only works for reasonably defined link parameters (after grandfathering in anchor, title, rt, if) and probably should say so much more explicitly. But I don’t know how to fix RFC 5988 about this issue.
Grüße, Carsten
_______________________________________________
core mailing list
https://www.ietf.org/mailman/listinfo/core
Adam Roach
2017-04-24 16:01:48 UTC
Permalink
Post by Alexey Melnikov
I think this document should recognize this change and just reference draft-nottingham-rfc5988bis normatively. (I will be AD sponsoring it shortly anyway.) Adam, will this address this concern
I think citing 5988bis is definitely called for, but not sufficient to
solve this issue. The thing that actually prevents the situation I
describe from occurring is that 5988bis will prevent new link relation
parameters of type quoted-string (only) from being defined, and my read
of 6690 is that the syntax of CoAP link relations will mirror those of
5988bis link relations, so I'm far less concerned about the
future-proofness of this protocol.

In terms of the issue at hand, I think this document really needs to
have English prose that captures the logic in the Ruby script (since the
script is non-normative).

My reading is that these rules are:

* Always include quotes for the known special cases (anchor, title,
rt, and if).
* For other parameters, omit quotes if the value is composed entirely
of token characters
* Include quotes otherwise

This necessarily requires some description of how to take the JSON/CBOR
format and convert it to HTTP/CoAP format, but that's something I think
the document really needs anyway.

/a
Alexey Melnikov
2017-04-24 16:17:57 UTC
Permalink
Hi Adam,
Post by Alexey Melnikov
I think this document should recognize this change and just reference
draft-nottingham-rfc5988bis normatively. (I will be AD sponsoring it
shortly anyway.) Adam, will this address this concern> I think citing 5988bis is definitely called for,
Let's at least do this.
but not sufficient to solve this issue. The thing that actually
prevents the situation I describe from occurring is that 5988bis will
prevent new link relation parameters of type quoted-string (only) from
being defined, and my read of 6690 is that the syntax of CoAP link
relations will mirror those of 5988bis link relations, so I'm far less
concerned about the future-proofness of this protocol.So other than the currently defined parameters (in rfc5988bis or RFC
6690), this situation will not occur, right?
In terms of the issue at hand, I think this document really needs to
have English prose that captures the logic in the Ruby script (since
the script is non-normative).Ok.
* Always include quotes for the known special cases (anchor, title,
rt, and if).
* For other parameters, omit quotes if the value is composed entirely
of token characters
* Include quotes otherwiseSeems sensible.
This necessarily requires some description of how to take the
JSON/CBOR format and convert it to HTTP/CoAP format, but that's
something I think the document really needs anyway.Agreed.
Adam Roach
2017-04-24 16:19:56 UTC
Permalink
Post by Carsten Bormann
Hi Adam,
Post by Adam Roach
Post by Alexey Melnikov
I think this document should recognize this change and just reference draft-nottingham-rfc5988bis normatively. (I will be AD sponsoring it shortly anyway.) Adam, will this address this concern
I think citing 5988bis is definitely called for,
Let's at least do this.
Post by Adam Roach
but not sufficient to solve this issue. The thing that actually
prevents the situation I describe from occurring is that 5988bis will
prevent new link relation parameters of type quoted-string (only)
from being defined, and my read of 6690 is that the syntax of CoAP
link relations will mirror those of 5988bis link relations, so I'm
far less concerned about the future-proofness of this protocol.
So other than the currently defined parameters (in rfc5988bis or RFC
6690), this situation will not occur, right?
As far as I can tell, that is true.

/a

Loading...