jackson-dataformats-binary: CBOR parser fails for `nextTextValue()` case for some reason (`float` decoding)
After seemingly unrelated change to jackson-core
(see https://github.com/FasterXML/jackson-core/pull/983 ), one CBOR test case started failing in ParserNextXxxTest
. It looks like END_ARRAY
token comes from CBORParser.nextTextValue()
as if maybe some initialization is missing, or … something.
But I think this is related to #347; I think it’s not a new bug but just being exposed by some change in sequence of actions.
@here-abarany I was wondering if you could help here?
About this issue
- Original URL
- State: closed
- Created a year ago
- Comments: 22 (22 by maintainers)
@here-abarany I really appreciate your feedback here. And one thing I may have forgotten to emphasize is that the goal of changes was NOT to change copying from parser to generator but to actually change copying to
TokenBuffer
(injackson-databind
) used for buffering content – but it happens to useJsonGenerator
as interface. So it is possible we may figure out bit different route. I hadn’t really considered format conversion use case, although was aware of it being an obvious thing (although whether users usecopyCurrentXxx()
methods or roll their own I don’t know – probably both methods are used).And like I said, had I realized scope of changes here, causing problems you point out (which are completely valid), I wouldn’t have proceeded. And may revert changes from 2.15 even: but I first want to think things through, considering various aspects.
We definitely won’t be immediately impacted by this given that it isn’t integrated yet, and our immediate concern will be configuring which
JsonParser
andJsonGenerator
types will handle reading and writing of data with the internal object model. I’ve envisioned use cases such as storing data as CBOR at rest while dynamically converting to/from JSON if text is explicitly requested. These are mostly theoretical at this point, but does at least demonstrate a potential pitfall.My biggest motivation for suggesting the more conservative approach is with such a widely used library, any change in behavior has the potential to cause regressions for clients. This is probably not an issue for text formats, though for the binary formats it could be more dangerous as it changes the fundamental representation of data. While I could see situations such as preferring a more compact representation for Smile being useful, if the primary concern is portability for CBOR files then having a
copyCurrentFP()
overridable function being a viable workaround, whether it simply restores the previous behavior for the immediate situation or has an option for more control. (though I suppose adding an option is the more dangerous option as it limits your flexibility for changing the interface later)Another option is to provide an overload to
JsonGenerator.copyCurrentEvent()
that allows you to pass it options to control the behavior, allowing you to make that decision at the callsite without having to modify the options of the generator itself.@cowtowncoder especially since this is so close to a release, maybe a more conservative approach is warranted to avoid minimal change in behavior by default? Based on the changes that were submitted, that would mean an opt-in feature for
JsonGenerator
to use the new code path for numbers. If you feel that preserving precision would be the more common desired use case, this could potentially be changed to be an opt-out feature for the 3.0 release, so those who would want to keep double precision numbers would have to explicitly disable it.Currently I don’t believe we have any real-world test cases with Jackson and CBOR at the moment, we have some initial work underway to prepare for integration but nothing concrete right now. I’m definitely keeping my eye on it and trying to push through adoption on our end.