Work on fix for #201 based on #200

cowtowncoder · cowtowncoder · commit 6cedcc4d1574 · 2020-04-08T09:06:33.000-07:00
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
@@ -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
@@ -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
@@ -71,21 +71,48 @@ 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),
                 (byte) 0x7F, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF);
 
         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
@@ -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
@@ -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)
