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

[Joe Hildebrand <[e-mail]>]

> On Aug 17, 2024, at 5:40 AM, Carsten Bormann <[e-mail]> wrote:
> 
> 
> ## 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

Nod.  After thinking about it some more, I don't think it's important to add.  Let me try a couple of things I thought of overnight before we add this complexity.

> 
>> - 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.)

Can we at least say that the output type of the app-strings need to be compatible enough to concatenate?

Take your 'f' approach from above; what would f'2' + f'3' mean?

>> - 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.

OK.  It would be nice to have just a few words in this spec about the concatenation, then.  Particularly because the commas are now optional unlike in 8610.

> 
>> 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.

Nod.  Again, I don't think there's a use case for this that matters, but it doesn't hurt anything, so there's no need to change it.

> ## 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

+1.

> ## 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).

I really don't want to crank up an entire other parser instance for each app-string.  There's not a *lot* of overhead, but there's enough that it will become noticeable, I think.  Therefore, I'm going to interpolate the app-string grammars into my larger grammar.  I hope I do it in a way that is similar enough to everyone else that it will interoperate and still allow extensibility correctly.  Maybe I'll get it right.

>> - (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.

It's not worth the change if others had tools available to them to process 0x 0o and 0b strings (including hex floats) all in one place.  Maybe I could have gotten all of them except hexfloats if I'd been willing to use eval().  If we are going to make a change for this, I'd suggest hexfloats be separated from hexints as well, since they are more likely to need custom code.

> ### 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.

I *didn't* read 8610 as I sat down to implement this, and didn't understand that it was important to do so from the text.  I started work on a CDDL implementation months ago, but never got very far.  Nevertheless, I expected CDDL to be completely unrelated to implementing diagnostic notation, and assumed I didn't need that doc.  In particular, section 1 says:

"This document sets forth a further step of evolution of EDN, and it is intended to serve as a single reference target in specifications that use EDN."


>> - 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.

Let's agree on the previous point before tackling this.

>> - 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.

Got it!

> (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.)

If the comment was a little more than /head/ maybe it would have been more clear?  Perhaps /string, length 3/?

>> - 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.

This probably needs to be a separate thread.  What I'm most worried about is that we haven't thought of the ways that you could mangle input to a parser constructed from the ABNF to get surprising results.  JSON is small enough that we think we've worked through those boundaries (although we did find the issue with U+2028 and U+2029 relatively late).

Maybe we should at least do some fuzzing of the ABNF.

>> 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…)

Thanks, I'll take a look.  I have a bunch of strings that are supposed to cause failures.  Those probably should be in a separate file.  I'll send PRs.

— 
Joe Hildebrand