fix: Cleanup JsonWriter bytes conversion code and add some test cover…

…age (#984)

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
- [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/java-bigquerystorage/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [ ] Ensure the tests and linter pass
- [ ] Code coverage does not decrease (if any source code was changed)
- [ ] Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1alpha2/BQTableSchemaToProtoDescriptor.java

@@ -54,7 +54,7 @@ public class BQTableSchemaToProtoDescriptor {
               .put(Table.TableFieldSchema.Type.DOUBLE, FieldDescriptorProto.Type.TYPE_DOUBLE)
               .put(Table.TableFieldSchema.Type.GEOGRAPHY, FieldDescriptorProto.Type.TYPE_STRING)
               .put(Table.TableFieldSchema.Type.INT64, FieldDescriptorProto.Type.TYPE_INT64)
-              .put(Table.TableFieldSchema.Type.NUMERIC, FieldDescriptorProto.Type.TYPE_STRING)
+              .put(Table.TableFieldSchema.Type.NUMERIC, FieldDescriptorProto.Type.TYPE_BYTES)
               .put(Table.TableFieldSchema.Type.STRING, FieldDescriptorProto.Type.TYPE_STRING)
               .put(Table.TableFieldSchema.Type.STRUCT, FieldDescriptorProto.Type.TYPE_MESSAGE)
               .put(Table.TableFieldSchema.Type.TIME, FieldDescriptorProto.Type.TYPE_STRING)

google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1beta2/JsonToProtoMessage.java

@@ -23,6 +23,7 @@
 import com.google.protobuf.DynamicMessage;
 import com.google.protobuf.Message;
 import com.google.protobuf.UninitializedMessageException;
+import java.util.logging.Logger;
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
@@ -32,10 +33,11 @@
  * descriptor must have all fields lowercased.
  */
 public class JsonToProtoMessage {
+  private static final Logger LOG = Logger.getLogger(JsonToProtoMessage.class.getName());
   private static ImmutableMap<FieldDescriptor.Type, String> FieldTypeToDebugMessage =
       new ImmutableMap.Builder<FieldDescriptor.Type, String>()
           .put(FieldDescriptor.Type.BOOL, "boolean")
-          .put(FieldDescriptor.Type.BYTES, "string")
+          .put(FieldDescriptor.Type.BYTES, "bytes")
           .put(FieldDescriptor.Type.INT32, "int32")
           .put(FieldDescriptor.Type.DOUBLE, "double")
           .put(FieldDescriptor.Type.INT64, "int64")
@@ -142,9 +144,6 @@ private static void fillField(
         if (val instanceof ByteString) {
           protoMsg.setField(fieldDescriptor, ((ByteString) val).toByteArray());
           return;
-        } else if (val instanceof String) {
-          protoMsg.setField(fieldDescriptor, ((String) val).getBytes());
-          return;
         }
         break;
       case INT64:
@@ -237,18 +236,23 @@ private static void fillRepeatedField(
           }
           break;
         case BYTES:
-          if (val instanceof String) {
-            // TODO(jstocklass): If string, decode it and pass in the byte array. Will need to
-            // update tests to ensure that strings passed in are properly encoded as well.
-            protoMsg.addRepeatedField(fieldDescriptor, ((String) val).getBytes());
-          } else if (val instanceof JSONArray) {
+          if (val instanceof JSONArray) {
             try {
               byte[] bytes = new byte[((JSONArray) val).length()];
               for (int j = 0; j < ((JSONArray) val).length(); j++) {
-                bytes[j] = (byte) ((byte) (((JSONArray) val).get(j)) & 0xFF);
+                bytes[j] = (byte) ((JSONArray) val).getInt(j);
+                if (bytes[j] != ((JSONArray) val).getInt(j)) {
+                  throw new IllegalArgumentException(
+                      String.format(
+                          "Error: "
+                              + currentScope
+                              + "["
+                              + index
+                              + "] could not be converted to byte[]."));
+                }
               }
               protoMsg.addRepeatedField(fieldDescriptor, bytes);
-            } catch (ClassCastException e) {
+            } catch (JSONException e) {
               throw new IllegalArgumentException(
                   String.format(
                       "Error: "

google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1alpha2/BQTableSchemaToProtoDescriptorTest.java

@@ -44,7 +44,7 @@ public class BQTableSchemaToProtoDescriptorTest {
               .put(Table.TableFieldSchema.Type.DOUBLE, DoubleType.getDescriptor())
               .put(Table.TableFieldSchema.Type.GEOGRAPHY, StringType.getDescriptor())
               .put(Table.TableFieldSchema.Type.INT64, Int64Type.getDescriptor())
-              .put(Table.TableFieldSchema.Type.NUMERIC, StringType.getDescriptor())
+              .put(Table.TableFieldSchema.Type.NUMERIC, BytesType.getDescriptor())
               .put(Table.TableFieldSchema.Type.STRING, StringType.getDescriptor())
               .put(Table.TableFieldSchema.Type.TIME, StringType.getDescriptor())
               .put(Table.TableFieldSchema.Type.TIMESTAMP, Int64Type.getDescriptor())
@@ -206,6 +206,12 @@ public void testStructComplex() throws Exception {
             .setMode(Table.TableFieldSchema.Mode.NULLABLE)
             .setName("test_time")
             .build();
+    final Table.TableFieldSchema TEST_NUMERIC_REPEATED =
+        Table.TableFieldSchema.newBuilder()
+            .setType(TableFieldSchema.Type.NUMERIC)
+            .setMode(Table.TableFieldSchema.Mode.REPEATED)
+            .setName("test_numeric_repeated")
+            .build();
     final Table.TableSchema tableSchema =
         Table.TableSchema.newBuilder()
             .addFields(0, test_int)
@@ -220,6 +226,7 @@ public void testStructComplex() throws Exception {
             .addFields(9, TEST_GEO)
             .addFields(10, TEST_TIMESTAMP)
             .addFields(11, TEST_TIME)
+            .addFields(12, TEST_NUMERIC_REPEATED)
             .build();
     final Descriptor descriptor =
         BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);

google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1beta2/BQTableSchemaToProtoDescriptorTest.java

@@ -203,6 +203,12 @@ public void testStructComplex() throws Exception {
             .setMode(TableFieldSchema.Mode.NULLABLE)
             .setName("test_time")
             .build();
+    final TableFieldSchema TEST_NUMERIC_REPEATED =
+        TableFieldSchema.newBuilder()
+            .setType(TableFieldSchema.Type.NUMERIC)
+            .setMode(TableFieldSchema.Mode.REPEATED)
+            .setName("test_numeric_repeated")
+            .build();
     final TableSchema tableSchema =
         TableSchema.newBuilder()
             .addFields(0, test_int)
@@ -217,6 +223,7 @@ public void testStructComplex() throws Exception {
             .addFields(9, TEST_GEO)
             .addFields(10, TEST_TIMESTAMP)
             .addFields(11, TEST_TIME)
+            .addFields(12, TEST_NUMERIC_REPEATED)
             .build();
     final Descriptor descriptor =
         BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);

google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1beta2/JsonStreamWriterTest.java

@@ -37,7 +37,11 @@
 import com.google.protobuf.Timestamp;
 import java.io.IOException;
 import java.math.BigDecimal;
-import java.util.*;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.UUID;
 import java.util.concurrent.ExecutionException;
 import java.util.logging.Logger;
 import org.json.JSONArray;
@@ -145,6 +149,12 @@ public class JsonStreamWriterTest {
           .setMode(TableFieldSchema.Mode.NULLABLE)
           .setName("test_numeric")
           .build();
+  private final TableFieldSchema TEST_NUMERIC_REPEATED =
+      TableFieldSchema.newBuilder()
+          .setType(TableFieldSchema.Type.NUMERIC)
+          .setMode(TableFieldSchema.Mode.REPEATED)
+          .setName("test_numeric_repeated")
+          .build();
   private final TableFieldSchema TEST_GEO =
       TableFieldSchema.newBuilder()
           .setType(TableFieldSchema.Type.GEOGRAPHY)
@@ -177,6 +187,7 @@ public class JsonStreamWriterTest {
           .addFields(9, TEST_GEO)
           .addFields(10, TEST_TIMESTAMP)
           .addFields(11, TEST_TIME)
+          .addFields(12, TEST_NUMERIC_REPEATED)
           .build();
 
   @Before
@@ -414,6 +425,14 @@ public void testSingleAppendComplexJson() throws Exception {
             .setTestGeo("POINT(1,1)")
             .setTestTimestamp(12345678)
             .setTestTime(CivilTimeEncoder.encodePacked64TimeMicros(LocalTime.of(1, 0, 1)))
+            .addTestNumericRepeated(
+                BigDecimalByteStringEncoder.encodeToNumericByteString(new BigDecimal("0")))
+            .addTestNumericRepeated(
+                BigDecimalByteStringEncoder.encodeToNumericByteString(
+                    new BigDecimal("99999999999999999999999999999.999999999")))
+            .addTestNumericRepeated(
+                BigDecimalByteStringEncoder.encodeToNumericByteString(
+                    new BigDecimal("-99999999999999999999999999999.999999999")))
             .build();
     JSONObject complex_lvl2 = new JSONObject();
     complex_lvl2.put("test_int", 3);
@@ -425,7 +444,7 @@ public void testSingleAppendComplexJson() throws Exception {
     JSONObject json = new JSONObject();
     json.put("test_int", 1);
     json.put("test_string", new JSONArray(new String[] {"a", "b", "c"}));
-    json.put("test_bytes", "hello");
+    json.put("test_bytes", ByteString.copyFrom("hello".getBytes()));
     json.put("test_bool", true);
     json.put("test_DOUBLe", new JSONArray(new Double[] {1.1, 2.2, 3.3, 4.4}));
     json.put("test_date", 1);
@@ -434,6 +453,19 @@ public void testSingleAppendComplexJson() throws Exception {
     json.put(
         "test_numeric",
         BigDecimalByteStringEncoder.encodeToNumericByteString(new BigDecimal("1.23456")));
+    json.put(
+        "test_numeric_repeated",
+        new JSONArray(
+            new byte[][] {
+              BigDecimalByteStringEncoder.encodeToNumericByteString(new BigDecimal("0"))
+                  .toByteArray(),
+              BigDecimalByteStringEncoder.encodeToNumericByteString(
+                      new BigDecimal("99999999999999999999999999999.999999999"))
+                  .toByteArray(),
+              BigDecimalByteStringEncoder.encodeToNumericByteString(
+                      new BigDecimal("-99999999999999999999999999999.999999999"))
+                  .toByteArray(),
+            }));
     json.put("test_geo", "POINT(1,1)");
     json.put("test_timestamp", 12345678);
     json.put("test_time", CivilTimeEncoder.encodePacked64TimeMicros(LocalTime.of(1, 0, 1)));

google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1beta2/JsonToProtoMessageTest.java

@@ -42,7 +42,7 @@ public class JsonToProtoMessageTest {
   private static ImmutableMap<Descriptor, String> AllTypesToDebugMessageTest =
       new ImmutableMap.Builder<Descriptor, String>()
           .put(BoolType.getDescriptor(), "boolean")
-          .put(BytesType.getDescriptor(), "string")
+          .put(BytesType.getDescriptor(), "bytes")
           .put(Int64Type.getDescriptor(), "int64")
           .put(Int32Type.getDescriptor(), "int32")
           .put(DoubleType.getDescriptor(), "double")
@@ -59,9 +59,7 @@ public class JsonToProtoMessageTest {
           .put(
               BytesType.getDescriptor(),
               new Message[] {
-                BytesType.newBuilder()
-                    .setTestFieldType(ByteString.copyFrom("test".getBytes()))
-                    .build()
+                BytesType.newBuilder().setTestFieldType(ByteString.copyFromUtf8("test")).build()
               })
           .put(
               Int64Type.getDescriptor(),
@@ -101,15 +99,15 @@ public class JsonToProtoMessageTest {
               })
           .build();
 
-  private static ImmutableMap<Descriptor, String[]> AllRepeatedTypesToDebugMessageTest =
-      new ImmutableMap.Builder<Descriptor, String[]>()
-          .put(RepeatedBool.getDescriptor(), new String[] {"boolean"})
-          .put(RepeatedBytes.getDescriptor(), new String[] {"string", "bytes"})
-          .put(RepeatedInt64.getDescriptor(), new String[] {"int64"})
-          .put(RepeatedInt32.getDescriptor(), new String[] {"int32"})
-          .put(RepeatedDouble.getDescriptor(), new String[] {"double"})
-          .put(RepeatedString.getDescriptor(), new String[] {"string"})
-          .put(RepeatedObject.getDescriptor(), new String[] {"object"})
+  private static ImmutableMap<Descriptor, String> AllRepeatedTypesToDebugMessageTest =
+      new ImmutableMap.Builder<Descriptor, String>()
+          .put(RepeatedBool.getDescriptor(), "boolean")
+          .put(RepeatedBytes.getDescriptor(), "bytes")
+          .put(RepeatedInt64.getDescriptor(), "int64")
+          .put(RepeatedInt32.getDescriptor(), "int32")
+          .put(RepeatedDouble.getDescriptor(), "double")
+          .put(RepeatedString.getDescriptor(), "string")
+          .put(RepeatedObject.getDescriptor(), "object")
           .build();
 
   private static ImmutableMap<Descriptor, Message[]> AllRepeatedTypesToCorrectProto =
@@ -123,8 +121,8 @@ public class JsonToProtoMessageTest {
               RepeatedBytes.getDescriptor(),
               new Message[] {
                 RepeatedBytes.newBuilder()
-                    .addTestRepeated(ByteString.copyFrom("hello".getBytes()))
-                    .addTestRepeated(ByteString.copyFrom("test".getBytes()))
+                    .addTestRepeated(ByteString.copyFrom(new byte[] {0}))
+                    .addTestRepeated(ByteString.copyFrom(new byte[] {0, -116, -122, 71}))
                     .build(),
                 RepeatedBytes.newBuilder()
                     .addTestRepeated(
@@ -206,9 +204,10 @@ public class JsonToProtoMessageTest {
     new JSONObject().put("test_field_type", Integer.MAX_VALUE),
     new JSONObject().put("test_field_type", 1.23),
     new JSONObject().put("test_field_type", true),
-    new JSONObject().put("test_field_type", "test"),
+    new JSONObject().put("test_field_type", ByteString.copyFromUtf8("test")),
     new JSONObject().put("test_field_type", new JSONArray("[1, 2, 3]")),
-    new JSONObject().put("test_field_type", new JSONObject().put("test_int", 1))
+    new JSONObject().put("test_field_type", new JSONObject().put("test_int", 1)),
+    new JSONObject().put("test_field_type", "test")
   };
 
   private static JSONObject[] simpleJSONArrays = {
@@ -264,6 +263,7 @@ public class JsonToProtoMessageTest {
                   BigDecimalByteStringEncoder.encodeToNumericByteString(new BigDecimal("1.2"))
                       .toByteArray()
                 })),
+    new JSONObject().put("test_repeated", new JSONArray(new int[][] {{11111, 22222}})),
     new JSONObject().put("test_repeated", new JSONArray(new char[][] {{'a', 'b'}, {'c'}})),
     new JSONObject().put("test_repeated", new JSONArray(new String[][] {{"hello"}, {"test"}})),
     new JSONObject()
@@ -350,8 +350,10 @@ public void testAllTypes() throws Exception {
       int success = 0;
       for (JSONObject json : simpleJSONObjects) {
         try {
+          LOG.info("Testing " + json + " over " + entry.getKey().getFullName());
           DynamicMessage protoMsg =
               JsonToProtoMessage.convertJsonToProtoMessage(entry.getKey(), json);
+          LOG.info("Convert Success!");
           assertEquals(protoMsg, AllTypesToCorrectProto.get(entry.getKey())[success]);
           success += 1;
         } catch (IllegalArgumentException e) {
@@ -361,40 +363,45 @@ public void testAllTypes() throws Exception {
         }
       }
       if (entry.getKey() == Int64Type.getDescriptor()) {
-        assertEquals(2, success);
+        assertEquals(entry.getKey().getFullName(), 2, success);
       } else {
-        assertEquals(1, success);
+        assertEquals(entry.getKey().getFullName(), 1, success);
       }
     }
   }
 
   @Test
   public void testAllRepeatedTypesWithLimits() throws Exception {
-    for (Map.Entry<Descriptor, String[]> entry : AllRepeatedTypesToDebugMessageTest.entrySet()) {
+    for (Map.Entry<Descriptor, String> entry : AllRepeatedTypesToDebugMessageTest.entrySet()) {
       int success = 0;
       for (JSONObject json : simpleJSONArrays) {
         try {
+          LOG.info("Testing " + json + " over " + entry.getKey().getFullName());
           DynamicMessage protoMsg =
               JsonToProtoMessage.convertJsonToProtoMessage(entry.getKey(), json);
-          assertEquals(protoMsg, AllRepeatedTypesToCorrectProto.get(entry.getKey())[success]);
+          LOG.info("Convert Success!");
+          assertEquals(
+              protoMsg.toString(),
+              protoMsg,
+              AllRepeatedTypesToCorrectProto.get(entry.getKey())[success]);
           success += 1;
         } catch (IllegalArgumentException e) {
+          LOG.info(e.getMessage());
           assertTrue(
               e.getMessage()
                       .equals(
                           "JSONObject does not have a "
-                              + entry.getValue()[0]
+                              + entry.getValue()
                               + " field at root.test_repeated[0].")
                   || e.getMessage()
                       .equals("Error: root.test_repeated[0] could not be converted to byte[]."));
         }
       }
       if (entry.getKey() == RepeatedInt64.getDescriptor()
-          || entry.getKey() == RepeatedDouble.getDescriptor()
-          || entry.getKey() == RepeatedBytes.getDescriptor()) {
-        assertEquals(2, success);
+          || entry.getKey() == RepeatedDouble.getDescriptor()) {
+        assertEquals(entry.getKey().getFullName(), 2, success);
       } else {
-        assertEquals(1, success);
+        assertEquals(entry.getKey().getFullName(), 1, success);
       }
     }
   }
@@ -501,7 +508,7 @@ public void testStructComplex() throws Exception {
     JSONObject json = new JSONObject();
     json.put("test_int", 1);
     json.put("test_string", new JSONArray(new String[] {"a", "b", "c"}));
-    json.put("test_bytes", "hello");
+    json.put("test_bytes", ByteString.copyFromUtf8("hello"));
     json.put("test_bool", true);
     json.put("test_DOUBLe", new JSONArray(new Double[] {1.1, 2.2, 3.3, 4.4}));
     json.put("test_date", 1);
@@ -525,7 +532,7 @@ public void testStructComplexFail() throws Exception {
     JSONObject json = new JSONObject();
     json.put("test_int", 1);
     json.put("test_string", new JSONArray(new String[] {"a", "b", "c"}));
-    json.put("test_bytes", "hello");
+    json.put("test_bytes", ByteString.copyFromUtf8("hello"));
     json.put("test_bool", true);
     json.put("test_double", new JSONArray(new Double[] {1.1, 2.2, 3.3, 4.4}));
     json.put("test_date", 1);

google-cloud-bigquerystorage/src/test/proto/jsonTest.proto

@@ -15,6 +15,7 @@ message ComplexRoot {
   optional string test_geo = 10;
   optional int64 test_timestamp = 11;
   optional int64 test_time = 12;
+  repeated bytes test_numeric_repeated = 13;
 }
 
 message CasingComplex {