search

FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
314 stars 136 forks source link

fix #155: Only flush outputs when FLUSH_PASSED_TO_STREAM is allowed #156

Closed carterkozak closed 5 years ago

carterkozak commented 5 years ago

Previously most serializers would attempt a flush when AUTO_CLOSE_TARGET was disabled, which is not expected if FLUSH_PASSED_TO_STREAM is also disabled.

This change results in binary serializers behaving the same as the json serializer.

cowtowncoder commented 5 years ago

Sounds legit, thank you for submitting this PR! I wonder if there'd be an easy way to test these via unit tests, to guard against regression?

Also: I think I'll need to backport this in 2.10 (since master is now for 3.0.0-SNAPSHOT), but changes are small enough that should be easy.

cowtowncoder commented 5 years ago

Looks like jackson-core has trivial test for the feature under src/test/java/com/fasterxml/jackson/core/main/TestGeneratorClosing.java, so could cut'n paste. Unfortunately there's no way (at this point) to easily share test classes.

I also added this on Jackson WIP page:

https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress

carterkozak commented 5 years ago

It should be relatively straightforward to write a test -- I can take a look later tonight. I'd really appreciate a 2.x backport, happy to help out with that.

cowtowncoder commented 5 years ago

@cakofony excellent: thank you again for reporting this & providing patch!

carterkozak commented 5 years ago

Thanks for the quick replies! I've added tests for cbor and smile, but I'm less familiar with the other formats and the same style of template didn't seem to work.

cowtowncoder commented 5 years ago

Thank you! I ended up manually merging bits and pieces as I realized that backporting will be challenging otherwise, but change is the same.