Add support for CBOR stringref extension (`CBORGenerator.Feature.STRINGREF`) #347

here-abarany · 2022-12-10T02:46:52Z

See http://cbor.schmorp.de/stringref for stringref definition. Repeated binary and text strings are assigned an index on write and a tagged integer is written when repeated. Parser respects the stringref IDs when reading data back in.

Nested tags are now supported. The previous getCurrentTag() function is still supported for backwards compatibility, and returns the first tag. Otherwise a new TagList type is used to efficiently handle multiple tags.

cowtowncoder · 2022-12-11T05:16:54Z

Ok, first of all: thank you for contributing this feature! It sounds a useful thing, CBOR implementing support something Smile has had for a while.

One thing I was wondering about: since this is a sizable thing, I was wondering if it was possible to split this in two PRs: one dealing with parsing, and second one (building on first) for generation? I realize it may be tricky wrt tests, but it would be easier to code review that way if at all possible.

Second thing: regardless of whether it's 1 or 2 PRs, one (and only) process thing we need is CLA.
It can be found from here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and is only needed once before the first contribution is merged (one is good for all future PRs for all Jackson projects).
The usual way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.

Thank you once again, looking forward to reviewing this PR and getting it merged for 2.15

here-abarany · 2022-12-12T20:48:52Z

One thing I was wondering about: since this is a sizable thing, I was wondering if it was possible to split this in two PRs: one dealing with parsing, and second one (building on first) for generation?

From what I understand this would require separate branches, which would be very unwieldy to manage and would require the first PR to be submitted before the second could actually be looked at. (since the second one would contain the code from the first) What I can do is split it into two commits, which would allow you to review them separately while being part of the same branch/PR.

Second thing: regardless of whether it's 1 or 2 PRs, one (and only) process thing we need is CLA.

Since I'm contributing this on company time I've forwarded this to the OSS department to make sure I get the OK to sign it. Hopefully it won't take too long to get approval.

cowtowncoder · 2022-12-12T22:24:53Z

One thing I was wondering about: since this is a sizable thing, I was wondering if it was possible to split this in two PRs: one dealing with parsing, and second one (building on first) for generation?

From what I understand this would require separate branches, which would be very unwieldy to manage and would require the first PR to be submitted before the second could actually be looked at. (since the second one would contain the code from the first) What I can do is split it into two commits, which would allow you to review them separately while being part of the same branch/PR.

Second thing: regardless of whether it's 1 or 2 PRs, one (and only) process thing we need is CLA.

Since I'm contributing this on company time I've forwarded this to the OSS department to make sure I get the OK to sign it. Hopefully it won't take too long to get approval.

Yes, would require 2 branches and I guess that won't play nicely with PRs. Too bad but I guess it makes sense to keep things bundled.

And yes, CLA should be fine; there's CCLA available if your company prefers that.

here-abarany · 2022-12-24T02:38:57Z

Just to give a heads up, we have a tentative agreement internally to sign the CCLA, but are trying to find the right people to sign it. I'm hoping we'll get it submitted by mid January. In the meantime I'll be out the next two weeks myself for the holidays.

here-abarany · 2023-02-01T19:55:26Z

@cowtowncoder I finally managed to get a signature and forwarded the CCLA last Friday. I just wanted to make sure that it was received and we can continue with the PR.

cowtowncoder · 2023-02-02T23:47:29Z

@here-abarany I can't seem to find CCLA. Was it sent to info at fasterxml dot com?

here-abarany · 2023-02-03T00:11:05Z

@cowtowncoder it was sent to clas rather than info, which was the address in the document. I can forward it to info as well.

cowtowncoder · 2023-02-03T00:12:57Z

@here-abarany Ok odd. I think clas should get forward to info... but yes, please try re-sending it as I don't see it.

here-abarany · 2023-02-03T00:14:01Z

@cowtowncoder I just forwarded it now.

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORGenerator.java

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORGenerator.java

cowtowncoder · 2023-02-03T00:26:44Z

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java

+    /**
+     * Stack of text and binary string references.
+     */
+    protected Stack<StringRefList> _stringRefs = new Stack<>();


Could probably create sub-class that has helper methods, if that simplifies calling code (will add a note where this'd help)

cowtowncoder · 2023-02-03T00:27:14Z

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java

                    _streamReadContext = _streamReadContext.getParent();
                    return (_currToken = JsonToken.END_OBJECT);
                }
                return (_currToken = _decodePropertyName());
            }
        } else {
            if (!_streamReadContext.expectMoreValues()) {
-                _tagValue = -1;
+                --_nestedDepth;
+                if (!_stringRefs.empty() && _stringRefs.peek().depth == _nestedDepth) {


Using a simple sub-class of Stack would allow hiding this logic into single method.

I created a helper class for this. I chose to use containment rather than subclass for this case as the interface is slightly different.

@here-abarany Ok yes, that is cleaner approach from OO design perspective. Although being only used internally not a big thing either way.

cowtowncoder · 2023-02-03T00:28:50Z

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java

@@ -837,6 +978,9 @@ protected String _numberToName(int ch, boolean neg) throws IOException
                // [dataformats-binary#269] (and earlier [dataformats-binary#30]),
                // got some edge case to consider
                if (i < 0) {
+                    if (isStringref) {
+                        _reportError("String reference index too large.");


Please remove trailing .

I think I got all the added errors, let me know if I missed any.

cowtowncoder · 2023-02-03T00:40:09Z

Ok @here-abarany -- impressive changes! I had some questions etc, although nothing major.

My main concern is actually getting this to merge against 3.0 (master). I know it's not practical to split this into smaller chunks, but one thing that might help is if white-space changes (removal of white-space from end-of-lines) could be done as a separate PR first?
I could merge that in, then into master; that'd probably remove noise slightly.

I suspect I will need help with master merge anyway but maybe little bit less.

here-abarany · 2023-02-03T00:51:11Z

@cowtowncoder thanks for the review, I hope to push up an update later today based on your comments. I'll have two separate pushes: one to rebase onto the latest 2.15 changes and one to push up the updates.

With regards to the whitespace changes, my editor is configured to clear trailing whitespace on save. I might be able to check out to the 2.15 head, save those files, and cherry-pick my changes on top of that to reduce some of the noise. If I'm successful I'll include that with my pushes later today.

I can try merging into the master branch locally and see what happens. Hopefully everything merges cleanly, if not I'll note what issues I encountered.

cowtowncoder · 2023-02-03T00:53:08Z

@cowtowncoder thanks for the review, I hope to push up an update later today based on your comments. I'll have two separate pushes: one to rebase onto the latest 2.15 changes and one to push up the updates.

With regards to the whitespace changes, my editor is configured to clear trailing whitespace on save. I might be able to check out to the 2.15 head, save those files, and cherry-pick my changes on top of that to reduce some of the noise. If I'm successful I'll include that with my pushes later today.

I can try merging into the master branch locally and see what happens. Hopefully everything merges cleanly, if not I'll note what issues I encountered.

It's unlikely to merge cleanly, partly due to package renaming, partly due to big API changes.
But exactly how tricky it is can vary. :)

As to whitespace, I'll try to figure out how to do that on my end for 2.15; that way you can just rebase on 2.15 head. No need for you to have to do that now I think.

cowtowncoder · 2023-02-03T01:35:23Z

@here-abarany Ok I did white-space trimming, merged it to master (re-linking some sources that hopefully reduces conflicts -- changing Java package causes lots of pain). So you may want to merge/rebase from 2.15 now.

here-abarany · 2023-02-03T02:19:54Z

@cowtowncoder I have pushed up my changes, I believe I addressed all the comment threads from the review. The first push is just a rebase, so you should be able to view just my latest changes by comparing the latest push just before this comment.

cowtowncoder · 2023-02-03T02:33:31Z

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORGenerator.java

@@ -236,6 +253,11 @@ public int getMask() {
     */
    protected boolean _bufferRecyclable;

+    /**
+     * Table of previously referenced text and binary strings.
+     */


... Used if StringRef functionality is enabled. // or something like that

@since 2.15

I wasn't sure if protected fields should be marked, I'll go through and try to add it for the fields I've added. (as well as the comment clarification noted here)

cowtowncoder · 2023-02-03T02:36:21Z

cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java

+
+    /**
+     * Shared string that should be used in place of _textBuffer when a string reference is used.
+     */


@SInCE 2.15

cowtowncoder · 2023-02-03T02:39:31Z

cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/ParserWithJsonOrgSampleTest.java

@@ -47,6 +47,16 @@ public void testJsonSampleDoc() throws IOException
        verifyJsonSpecSampleDoc(cborParser(data), false, true);
    }

+    public void testJsonSampleDocStringref() throws IOException
+    {
+        byte[] data = cborDoc(new CBORFactory().enable(CBORGenerator.Feature.STRINGREF),


Could you change this to use Builder? This merges cleanly with 3.0 where factories are immutable.

I've switched this and the one below to use a builder.

See http://cbor.schmorp.de/stringref for stringref definition. When a stringref namespace is encountered, indices will be generated for each valid binary and text string and used when referenced. Nested tags are now supported. The previous getCurrentTag() function is still supported for backwards compatibility, and returns the first tag. Otherwise a new TagList type is used to efficiently handle multiple tags.

cowtowncoder · 2023-02-03T02:40:05Z

cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/mapper/BiggerDataTest.java

+		Citm citm0 = MAPPER.readValue(getClass().getResourceAsStream("/data/citm_catalog.json"),
+				Citm.class);
+		ObjectMapper mapper = new CBORMapper(
+		    new CBORFactory().enable(CBORGenerator.Feature.STRINGREF));


Same here, builder over factory configuration (I know, many existing tests use this, trying to move over with new code).

cowtowncoder · 2023-02-03T02:41:18Z

@here-abarany Ok, sorry, missed couple of smaller things; now done with reviewing I think. Minor test changes requested to make it more likely things merge less uncleanly.

When enabled with the STRINGREF feature, a namespace is created for the root element and indices are produced for valid binary and text strings.

Copying tags can be dangerous in two main cases: 1. Jackson creates its own tags, such as for BigNum or the self describe tag. 2. The semantic meaning of an element changes. For example, converting between stringrefs and no stringrefs. In some cases it would result in duplicate tags, while in others it may cause completely incorrect tags, such as a stringref tag applying to a string rather than an integer. Before nested tag support was added this could generate files that Jackson could not parse, though even with nested tag support it may cause undefined behavior.

here-abarany · 2023-02-03T02:47:58Z

@cowtowncoder I have pushed up a new revision to fix your latest comments.

cowtowncoder · 2023-02-04T01:57:02Z

Thank you @here-abarany! I will pick this up again today or tomorrow, get it merged.

cowtowncoder · 2023-02-04T03:01:59Z

Ok, phew. Merge to master went ok, almost: there is just one new failure and that's for String-Refs; BiggerDataTest.testRoundTripStringref().

I suspect it is since CBORParser.nextFieldName() methods (renamed as just nextName() in 3.0) do not all handle String-refs.
Or rather, new nextNameMatch() method doesn't -- it probably needs to, and I think jackson-databind uses that for Bean properties in 3.0.

Could you have a look if you can see how to fix this, @here-abarany ?

here-abarany · 2023-02-04T03:35:35Z

@cowtowncoder thanks for getting it merged in, I'll see if I can take a look on Monday for the master issue.

cowtowncoder · 2023-02-05T01:45:01Z

@cowtowncoder thanks for getting it merged in, I'll see if I can take a look on Monday for the master issue.

Thank you in advance! Hopefully it is relatively straight-forward.

here-abarany force-pushed the 2.15 branch from 99494af to fe4283c Compare December 12, 2022 21:38