[Cbor] Re: draft-ietf-cbor-edn-literals-10 implementation notes

[Carsten Bormann <[e-mail]>]

Hi Joe,

thank you for this extensive feedback.

I have structured my response into several sections

> - Overall, this spec is in pretty good shape.  Aside from the app-strings, I was able to implement directly from the ABNF using my standard tooling.  Reading the actual text of the spec answered most of my questions.

Good.  Of course, I’d like to know the complement of the “most” — I assume there will be something below on some of these items.

## Proposals for technical changes

> - I wish that floating point encodings had a separate spec syntax from integers, rather than relying on getting a decimal point or "e" into the output somehow (e.g. JS doesn't have good float formatting built in).  For example, 2_f1 could mean 0xf94000, while 2_1 would mean 0x190002.

We have used diagnostic notation in a different way for a long time.
So I’d consider this a technical change.

The specific change suggested here violates an invariant of encoding indicators: You can leave them off and will still get an equivalent data item from a data model point of view.  

(It did take us some time to learn enough from implementations to evolve the CBOR data model from a muddy boundary between integer and float in RFC 7049 to a much more well-defined boundary that we now have in RFC 8949.)

I would probably solve the specific use case by two application-oriented extension literals:

f'2' is equivalent to 2.0, not 2
i'1e3' is equivalent to 1000, not 1000.0

> - s4.1 It should be an error to mix app-strings and strings, or app-strings of multiple types.  The text at the end of the section starting with "Some of the strings may be app-strings..." is not proscriptive enough.

This is a reasonable conclusion when you just look at the set of application-extensions that are in the document, but it breaks down when constructing strings from components that can be described by app-strings (something like a JWT that mixes base64-encoded strings with delimiters such as “.” and “~”).
(The example would need to build the base64-encoded parts using application-oriented literals, for which we don’t have an application-oriented literal — b64 goes the other way around.  I find it quite likely that we’ll have application-oriented literals for JWT and CWT over time.)

> - It's not clear to me what happens if there are multiple items in the sequence inside <<>>.  I assume they are concatenated together, even though that's a little odd unless you are generating cbor streams.  I would have expected the production to be:
> 
>    embedded = "<<" one-item ">>"

(Current ABNF is:

    embedded        = "<<" seq ">>"

where seq is a CBOR sequence.)

This syntax for Embedded CBOR sequences is quite explicitly part of Appendix G.3 of RFC 8610.
Embedded CBOR sequences are a natural way to employ the fact that the byte string encoding already provides an outer delineation, so an array head is superfluous.  I’ve seen them used in various contexts but don’t have an example handy off the top of my head.

> You would still get concatenation with << 1 >> + << 2 >> in the unlikely event you need it.

I would consider needing to write this (instead of << 1, 2 >>) noise.

## Bugs

> - s3.2, "Herewith I buy" /.../ "gned: Alice & Bob" doesn't match the grammar.  I think it needs a +.


Good point.
(Mental note: I need to run the examples through the newest edn-abnf…)

I propose to simply remove the “contract” member of the example, as it is no longer really a compelling way of using comments.

Now:
https://github.com/cbor-wg/edn-literal/pull/58

## Exposition

### ABNF

> - Like others, I don't like the two-level ABNF.  I understand that you're going for extensibility, but you can still leave an extension point in place while having a single grammar.

As an implementers’ note for an implementation that just implements the extensions and constructs that already are in the document, I can follow that view (even if it works out differently in not only my implementation).
Extensibility, however, is important, and I think I have made my point why a little more work for some initial implementations will be worth it for extensibility.
Maybe an implementers’ note always will take the perspective of an implementer, not one of caring about protocol evolution; but some implementers have found that the two-level approach is also easier to implement (and more robust, to boot).

> - (nit) I'd prefer basenumber to be split into 3 or 4 rules, one for each base, since each needs special processing.

I personally like the more compact way, but I’ll put up a change proposal in a PR.

### Document structure

> - The doc structure is quite odd in that it presents the extension points before the main format.  I understand that's an outgrowth of how the doc grew over time, but it needs a small refactor before publishing.  I'm willing to provide more suggestions or help, if the authors want.

One comment on another recent document essentially was that we should do a complete roll-up of the documents that constitute our ecosystem — we already did a roll-up for CBOR (RFC 8949) but could may revise that with a target publication date of 2027 (another seven years).  
I’d rather do the editorial work then.  
Right now the document relies on the reader having read Section 8 of 8949 and Appendix G of 8610; so focusing on the additions first sounds acceptable to me.

> - s3 could be moved after the ABNF section and the (possibly new) app-string section and make more sense.

I need to re-read that to have an opinion.
(Moving the ABNF from an appendix to a main section strikes back.)

It seems to me a forward reference from Section 3 to the rule “app-string” in Section 4.1 would also do it.

> - s4.2 could be outdented to s5, containing both ABNF and the descriptions of the app-string formats from s2.  Having to go back and forth made reading more difficult than it needed to be.

Makes sense on a napkin.  I’ll try to do a PR.

> - s4.2.1, h'/head/ 63 /contents/ 66 6f 6f' should become << "cfoo" >>, not << "foo" >> if I'm understanding correctly.

“foo” is a text string of length 3, so it begins with 0x63, followed by 66 6f 6f for the f o o characters.
“cfoo” is a text string of length 4, so it begins with 0x64, followed by 63 66 6f 6f.

$ echo "h'/head/ 63 /contents/ 66 6f 6f'" | edn-abnf -tdiag - | diag2pretty.rb
44          # bytes(4)
   63666f6f # "cfoo"
$ echo '<< "foo" >>' | edn-abnf -tdiag - | diag2pretty.rb
44          # bytes(4)
   63666f6f # "cfoo"
$ echo '<< "cfoo" >>' | edn-abnf -tdiag - | diag2pretty.rb
45            # bytes(5)
   6463666f6f # “dcfoo"
$ echo "h'/head/ 63 /contents/ 66 6f 6f'" | edn-abnf -tdiag - | diag2diag.rb -e
<< "foo" >>
$ echo '<< "foo" >>' | edn-abnf -tdiag - | diag2diag.rb -e
<< "foo" >>
$ echo '<< "cfoo" >>' | edn-abnf -tdiag - | diag2diag.rb -e
<< "cfoo" >>
(Embedded CBOR can be confusing because of the multiple layers, why it was a great addition to get syntax for that in Appendix G.3 of RFC 8610.)

> - s6 Security Considerations seems like it could use some more text about how this format isn't intended for interchange.

Some text (or just key words) would be appreciated.

> I'm building up a large-ish set of test vectors.  I'm willing to put those into a separate repo for sharing if anyone is interested in collaborating.

Absolutely!
I have a very lazy set of tests (specifically testing out recent changes) as a CSV (first column = for equivalent and - for not equivalent, second column EDN, third column also EDN but different features):

https://github.com/cabo/edn-abnf/blob/explicit-concatenation/tests/basic.csv

(This CSV contains carriage returns in some of the CSV-quoted strings; this abuse of CSV might not survive a Windows environment, but the tests will magically still pass :-).

(This could have been EDN plus hex encoded CBOR data items, but it is so much easier to write the latter in EDN, too…)

> I'm not quite done with the implementation, so there are likely to be a few more comments as I continue to dig.

Looking forward to them!

Grüße, Carsten