From 6cedcc4d1574b3d91b5ecf5b1efb1f3ea0189fa7 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Wed, 8 Apr 2020 09:06:33 -0700 Subject: [PATCH] Work on fix for #201 based on #200 --- .../dataformat/cbor/CBORGenerator.java | 59 +++++++++++++++---- .../cbor/gen/ArrayGenerationTest.java | 52 +++++++++++++--- .../cbor/gen/GeneratorSimpleTest.java | 38 ++++++++++-- release-notes/CREDITS-2.x | 6 ++ release-notes/VERSION-2.x | 3 + 5 files changed, 135 insertions(+), 23 deletions(-) diff --git a/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORGenerator.java b/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORGenerator.java index 0ab5cf308..0cbbda84e 100644 --- a/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORGenerator.java +++ b/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORGenerator.java @@ -114,9 +114,6 @@ public int getMask() { */ private final static int MIN_BUFFER_LENGTH = (3 * 256) + 2; - private final static long MIN_INT_AS_LONG = (long) Integer.MIN_VALUE; - private final static long MAX_INT_AS_LONG = (long) Integer.MAX_VALUE; - /** * Special value that is use to keep tracks of arrays and maps opened with infinite length */ @@ -671,10 +668,48 @@ private final void _writeIntNoCheck(int i) throws IOException { _outputBuffer[_outputTail++] = b0; } + private final void _writeIntMinimal(int markerBase, int i) throws IOException + { + _ensureRoomForOutput(5); + byte b0; + if (i >= 0) { + if (i < 24) { + _outputBuffer[_outputTail++] = (byte) (markerBase + i); + return; + } + if (i <= 0xFF) { + _outputBuffer[_outputTail++] = (byte) (markerBase + SUFFIX_UINT8_ELEMENTS); + _outputBuffer[_outputTail++] = (byte) i; + return; + } + b0 = (byte) i; + i >>= 8; + if (i <= 0xFF) { + _outputBuffer[_outputTail++] = (byte) (markerBase + SUFFIX_UINT16_ELEMENTS); + _outputBuffer[_outputTail++] = (byte) i; + _outputBuffer[_outputTail++] = b0; + return; + } + } else { + b0 = (byte) i; + i >>= 8; + } + _outputBuffer[_outputTail++] = (byte) (markerBase + SUFFIX_UINT32_ELEMENTS); + _outputBuffer[_outputTail++] = (byte) (i >> 16); + _outputBuffer[_outputTail++] = (byte) (i >> 8); + _outputBuffer[_outputTail++] = (byte) i; + _outputBuffer[_outputTail++] = b0; + } + private final void _writeLongNoCheck(long l) throws IOException { if (_cfgMinimalInts) { - if (l <= MAX_INT_AS_LONG && l >= MIN_INT_AS_LONG) { - _writeIntNoCheck((int) l); + if (l >= 0) { + if (l <= 0x100000000L) { + _writeIntMinimal(PREFIX_TYPE_INT_POS, (int) l); + return; + } + } else if (l >= -0x100000000L) { + _writeIntMinimal(PREFIX_TYPE_INT_NEG, (int) (-l - 1)); return; } } @@ -935,14 +970,18 @@ public void writeNumber(int i) throws IOException { @Override public void writeNumber(long l) throws IOException { - if (_cfgMinimalInts) { - // First: maybe 32 bits is enough? - if (l <= MAX_INT_AS_LONG && l >= MIN_INT_AS_LONG) { - writeNumber((int) l); + _verifyValueWrite("write number"); + if (_cfgMinimalInts) { // maybe 32 bits is enough? + if (l >= 0) { + if (l <= 0x100000000L) { + _writeIntMinimal(PREFIX_TYPE_INT_POS, (int) l); + return; + } + } else if (l >= -0x100000000L) { + _writeIntMinimal(PREFIX_TYPE_INT_NEG, (int) (-l - 1)); return; } } - _verifyValueWrite("write number"); _ensureRoomForOutput(9); if (l < 0L) { l += 1; diff --git a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/ArrayGenerationTest.java b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/ArrayGenerationTest.java index f4ebe73ef..40d86f2f6 100644 --- a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/ArrayGenerationTest.java +++ b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/ArrayGenerationTest.java @@ -3,7 +3,10 @@ import java.io.ByteArrayOutputStream; import com.fasterxml.jackson.core.*; +import com.fasterxml.jackson.core.JsonParser.NumberType; import com.fasterxml.jackson.dataformat.cbor.CBORFactory; +import com.fasterxml.jackson.dataformat.cbor.CBORGenerator; +import com.fasterxml.jackson.dataformat.cbor.CBORParser; import com.fasterxml.jackson.dataformat.cbor.CBORTestBase; /** @@ -15,23 +18,54 @@ public class ArrayGenerationTest extends CBORTestBase public void testIntArray() throws Exception { - _testIntArray(false); - _testIntArray(true); + _testIntArray(); } public void testLongArray() throws Exception { - _testLongArray(false); - _testLongArray(true); + _testLongArray(); } public void testDoubleArray() throws Exception { - _testDoubleArray(false); - _testDoubleArray(true); + _testDoubleArray(); } - private void _testIntArray(boolean useBytes) throws Exception { + public void testMinimalIntValues2() throws Exception + { + // Array with 2 values that can't be passed as `int`s but DO fit + // CBOR 5-byte int (sign + 32-bits) + final long[] input = new long[] { + 0xffffffffL, // max value that fits + -0xffffffffL - 1 // min value that fits + }; + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + CBORGenerator gen = FACTORY.createGenerator(bytes); + assertTrue(gen.isEnabled(CBORGenerator.Feature.WRITE_MINIMAL_INTS)); + gen.writeArray(input, 0, 2); + gen.close(); + + // With default settings, should get: + byte[] encoded = bytes.toByteArray(); + assertEquals(11, encoded.length); + + // then verify contents + + CBORParser p = FACTORY.createParser(encoded); + assertToken(JsonToken.START_ARRAY, p.nextToken()); + assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken()); + assertEquals(NumberType.LONG, p.getNumberType()); + assertEquals(input[0], p.getLongValue()); + assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken()); + assertEquals(NumberType.LONG, p.getNumberType()); + assertEquals(input[1], p.getLongValue()); + assertToken(JsonToken.END_ARRAY, p.nextToken()); + p.close(); + + // but then also check without minimization + } + + private void _testIntArray() throws Exception { // first special cases of 0, 1 values _testIntArray(0, 0, 0); _testIntArray(0, 1, 1); @@ -52,7 +86,7 @@ private void _testIntArray(boolean useBytes) throws Exception { _testIntArray(7777, 0, 1); } - private void _testLongArray(boolean useBytes) throws Exception { + private void _testLongArray() throws Exception { // first special cases of 0, 1 values _testLongArray(0, 0, 0); _testLongArray(0, 1, 1); @@ -73,7 +107,7 @@ private void _testLongArray(boolean useBytes) throws Exception { _testLongArray(6110, 0, 1); } - private void _testDoubleArray(boolean useBytes) throws Exception { + private void _testDoubleArray() throws Exception { // first special cases of 0, 1 values _testDoubleArray(0, 0, 0); _testDoubleArray(0, 1, 1); diff --git a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/GeneratorSimpleTest.java b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/GeneratorSimpleTest.java index c3e39c423..64d8aa777 100644 --- a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/GeneratorSimpleTest.java +++ b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/gen/GeneratorSimpleTest.java @@ -71,7 +71,7 @@ public void testMinimalIntValues() throws Exception out = new ByteArrayOutputStream(); gen = cborGenerator(out); - gen.writeNumber(Integer.MAX_VALUE); + gen.writeNumber((long) Integer.MAX_VALUE); gen.close(); _verifyBytes(out.toByteArray(), (byte) (CBORConstants.PREFIX_TYPE_INT_POS + 26), @@ -79,13 +79,40 @@ public void testMinimalIntValues() throws Exception out = new ByteArrayOutputStream(); gen = cborGenerator(out); - gen.writeNumber(Integer.MIN_VALUE); + gen.writeNumber((long) Integer.MIN_VALUE); gen.close(); _verifyBytes(out.toByteArray(), (byte) (CBORConstants.PREFIX_TYPE_INT_NEG + 26), (byte) 0x7F, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF); } + // [dataformats-binary#201] + public void testMinimalIntValues2() throws Exception + { + ByteArrayOutputStream out; + CBORGenerator gen; + + out = new ByteArrayOutputStream(); + gen = cborGenerator(out); + // -1 if coerced as int, BUT cbor encoding can fit in 32-bit integer since + // sign is indicated by prefix + gen.writeNumber(0xffffffffL); + gen.close(); + _verifyBytes(out.toByteArray(), + (byte) (CBORConstants.PREFIX_TYPE_INT_POS + 26), + (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF); + + out = new ByteArrayOutputStream(); + gen = cborGenerator(out); + // similarly for negative numbers, we have 32-bit value bits, not 31 + // as with Java int + gen.writeNumber(-0xffffffffL - 1); + gen.close(); + _verifyBytes(out.toByteArray(), + (byte) (CBORConstants.PREFIX_TYPE_INT_NEG + 26), + (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF); + } + public void testIntValues() throws Exception { // first, single-byte @@ -156,12 +183,15 @@ public void testLongValues() throws Exception { ByteArrayOutputStream out = new ByteArrayOutputStream(); CBORGenerator gen = cborGenerator(out); - long l = -1L + Integer.MIN_VALUE; + + // 07-Apr-2020, tatu: Since minimization enabled by default, + // need to use value that can not be minimized + long l = ((long) Integer.MIN_VALUE) << 2; gen.writeNumber(l); gen.close(); byte[] b = out.toByteArray(); - assertEquals((byte) (CBORConstants.PREFIX_TYPE_INT_NEG + 27), b[0]); assertEquals(9, b.length); + assertEquals((byte) (CBORConstants.PREFIX_TYPE_INT_NEG + 27), b[0]); // could test full contents, but for now this shall suffice assertEquals(0, b[1]); assertEquals(0, b[2]); diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x index 4abfe468b..776552d5e 100644 --- a/release-notes/CREDITS-2.x +++ b/release-notes/CREDITS-2.x @@ -126,3 +126,9 @@ Binh Tran (ankel@github) * Reported, contributed fix for #192: (ion) Allow `IonObjectMapper` with class name annotation introspector to deserialize generic subtypes (2.11.0) + +Jonas Konrad (yawkat@github) +* Reported, contributed fix for #201: `CBORGenerator.Feature.WRITE_MINIMAL_INTS` does not write + most compact form for all integers + (2.11.0) + diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index 892ec2fc4..91be220f2 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -19,6 +19,9 @@ Project: jackson-datatypes-binaryModules: (contributed by Bryan H) #198: `jackson-databind` should not be full dependency for (cbor, protobuf, smile) modules +#201: `CBORGenerator.Feature.WRITE_MINIMAL_INTS` does not write most compact form + for all integers + (reported by Jonas K) - `AvroGenerator` overrides `getOutputContext()` properly 2.10.3 (03-Mar-2020)