Skip to content

Commit 186d213

Browse files
yirutangyirutang
authored
fix: Add missing field error to row error message (#1752)
* fix: Add missing field error to row error message * . * . * . * . * . * . * . * . * . * . * . * . * .
1 parent 626752b commit 186d213

File tree

5 files changed

+160
-26
lines changed

5 files changed

+160
-26
lines changed

‎google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/Exceptions.java

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ public static class AppendSerializtionError extends RuntimeException {
225225
private final String streamName;
226226

227227
public AppendSerializtionError(String streamName, Map<Integer, String> rowIndexToErrorMessage) {
228-
super(String.format("Append serializtion failed for writer: %s", streamName));
228+
super(String.format("Append serialization failed for writer: %s", streamName));
229229
this.rowIndexToErrorMessage = rowIndexToErrorMessage;
230230
this.streamName = streamName;
231231
}
@@ -239,6 +239,35 @@ public String getStreamName() {
239239
}
240240
}
241241

242+
/** This exception is used internally to handle field level parsing errors. */
243+
public static class FieldParseError extends IllegalArgumentException {
244+
private final String fieldName;
245+
private final String bqType;
246+
private final Throwable cause;
247+
248+
protected FieldParseError(String fieldName, String bqType, Throwable cause) {
249+
this.fieldName = fieldName;
250+
this.bqType = bqType;
251+
this.cause = cause;
252+
}
253+
254+
public String getFieldName() {
255+
return fieldName;
256+
}
257+
258+
public String getBqType() {
259+
return bqType;
260+
}
261+
262+
public Throwable getCause() {
263+
return cause;
264+
}
265+
266+
public String getMessage() {
267+
return cause.getMessage();
268+
}
269+
}
270+
242271
/**
243272
* This writer instance has either been closed by the user explicitly, or has encountered
244273
* non-retriable errors.

‎google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/JsonStreamWriter.java

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,19 @@ public ApiFuture<AppendRowsResponse> append(JSONArray jsonArr, long offset)
155155
rowsBuilder.addSerializedRows(protoMessage.toByteString());
156156
currentRequestSize += protoMessage.getSerializedSize();
157157
} catch (IllegalArgumentException exception) {
158-
rowIndexToErrorMessage.put(i, exception.getMessage());
158+
if (exception instanceof Exceptions.FieldParseError) {
159+
Exceptions.FieldParseError ex = (Exceptions.FieldParseError) exception;
160+
rowIndexToErrorMessage.put(
161+
i,
162+
"Field "
163+
+ ex.getFieldName()
164+
+ " failed to convert to "
165+
+ ex.getBqType()
166+
+ ". Error: "
167+
+ ex.getCause().getMessage());
168+
} else {
169+
rowIndexToErrorMessage.put(i, exception.getMessage());
170+
}
159171
}
160172
}
161173

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

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -221,11 +221,24 @@ private static DynamicMessage convertJsonToProtoMessageImpl(
221221
+ ")");
222222
}
223223
}
224-
if (!field.isRepeated()) {
225-
fillField(protoMsg, field, fieldSchema, json, jsonName, currentScope, ignoreUnknownFields);
226-
} else {
227-
fillRepeatedField(
228-
protoMsg, field, fieldSchema, json, jsonName, currentScope, ignoreUnknownFields);
224+
try {
225+
if (!field.isRepeated()) {
226+
fillField(
227+
protoMsg, field, fieldSchema, json, jsonName, currentScope, ignoreUnknownFields);
228+
} else {
229+
fillRepeatedField(
230+
protoMsg, field, fieldSchema, json, jsonName, currentScope, ignoreUnknownFields);
231+
}
232+
} catch (Exceptions.FieldParseError ex) {
233+
throw ex;
234+
} catch (Exception ex) {
235+
// This function is recursively called, so this throw will be caught and throw directly out
236+
// by the catch
237+
// above.
238+
throw new Exceptions.FieldParseError(
239+
currentScope,
240+
fieldSchema != null ? fieldSchema.getType().name() : field.getType().name(),
241+
ex);
229242
}
230243
}
231244

@@ -329,26 +342,17 @@ private static void fillField(
329342
protoMsg.setField(fieldDescriptor, ((ByteString) val).toByteArray());
330343
return;
331344
} else if (val instanceof JSONArray) {
332-
try {
333-
byte[] bytes = new byte[((JSONArray) val).length()];
334-
for (int j = 0; j < ((JSONArray) val).length(); j++) {
335-
bytes[j] = (byte) ((JSONArray) val).getInt(j);
336-
if (bytes[j] != ((JSONArray) val).getInt(j)) {
337-
throw new IllegalArgumentException(
338-
String.format(
339-
"Error: "
340-
+ currentScope
341-
+ "["
342-
+ j
343-
+ "] could not be converted to byte[]."));
344-
}
345+
byte[] bytes = new byte[((JSONArray) val).length()];
346+
for (int j = 0; j < ((JSONArray) val).length(); j++) {
347+
bytes[j] = (byte) ((JSONArray) val).getInt(j);
348+
if (bytes[j] != ((JSONArray) val).getInt(j)) {
349+
throw new IllegalArgumentException(
350+
String.format(
351+
"Error: " + currentScope + "[" + j + "] could not be converted to byte[]."));
345352
}
346-
protoMsg.setField(fieldDescriptor, bytes);
347-
return;
348-
} catch (JSONException e) {
349-
throw new IllegalArgumentException(
350-
String.format("Error: " + currentScope + "could not be converted to byte[]."));
351353
}
354+
protoMsg.setField(fieldDescriptor, bytes);
355+
return;
352356
}
353357
break;
354358
case INT64:

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

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.google.api.gax.grpc.testing.MockGrpcService;
3030
import com.google.api.gax.grpc.testing.MockServiceHelper;
3131
import com.google.cloud.bigquery.storage.test.JsonTest;
32+
import com.google.cloud.bigquery.storage.test.SchemaTest;
3233
import com.google.cloud.bigquery.storage.test.Test.FooType;
3334
import com.google.cloud.bigquery.storage.test.Test.UpdatedFooType;
3435
import com.google.cloud.bigquery.storage.v1.Exceptions.AppendSerializtionError;
@@ -639,7 +640,46 @@ public void testMultipleAppendSerializtionErrors()
639640
"JSONObject has fields unknown to BigQuery: root.not_foo.",
640641
rowIndexToErrorMessage.get(0));
641642
assertEquals(
642-
"JSONObject does not have a string field at root.foo.", rowIndexToErrorMessage.get(2));
643+
"Field root.foo failed to convert to STRING. Error: JSONObject does not have a string field at root.foo.",
644+
rowIndexToErrorMessage.get(2));
645+
}
646+
}
647+
}
648+
649+
@Test
650+
public void testBadStringToNumericRowError()
651+
throws DescriptorValidationException, IOException, InterruptedException {
652+
TableSchema TABLE_SCHEMA =
653+
TableSchema.newBuilder()
654+
.addFields(
655+
0,
656+
TableFieldSchema.newBuilder()
657+
.setName("test_field_type")
658+
.setType(TableFieldSchema.Type.NUMERIC)
659+
.setMode(TableFieldSchema.Mode.NULLABLE)
660+
.build())
661+
.build();
662+
SchemaTest.StringType expectedProto =
663+
SchemaTest.StringType.newBuilder().setTestFieldType("allen").build();
664+
JSONObject foo = new JSONObject();
665+
// put a field which is not part of the expected schema
666+
foo.put("test_field_type", "allen");
667+
JSONArray jsonArr = new JSONArray();
668+
jsonArr.put(foo);
669+
670+
try (JsonStreamWriter writer =
671+
getTestJsonStreamWriterBuilder(TEST_STREAM, TABLE_SCHEMA).build()) {
672+
try {
673+
ApiFuture<AppendRowsResponse> appendFuture = writer.append(jsonArr);
674+
Assert.fail("expected AppendSerializtionError");
675+
} catch (AppendSerializtionError appendSerializtionError) {
676+
Map<Integer, String> rowIndexToErrorMessage =
677+
appendSerializtionError.getRowIndexToErrorMessage();
678+
assertEquals(1, rowIndexToErrorMessage.size());
679+
assertTrue(
680+
rowIndexToErrorMessage
681+
.get(0)
682+
.startsWith("Field root.test_field_type failed to convert to NUMERIC. Error:"));
643683
}
644684
}
645685
}

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1226,4 +1226,53 @@ public void testJsonAllFieldsNullValue() throws Exception {
12261226
JsonToProtoMessage.convertJsonToProtoMessage(TestInt64.getDescriptor(), json);
12271227
assertEquals(expectedProto, protoMsg);
12281228
}
1229+
1230+
@Test
1231+
public void testBadJsonFieldRepeated() throws Exception {
1232+
TableSchema ts =
1233+
TableSchema.newBuilder()
1234+
.addFields(
1235+
0,
1236+
TableFieldSchema.newBuilder()
1237+
.setName("test_repeated")
1238+
.setType(TableFieldSchema.Type.NUMERIC)
1239+
.setMode(TableFieldSchema.Mode.REPEATED)
1240+
.build())
1241+
.build();
1242+
JSONObject json = new JSONObject();
1243+
json.put("test_repeated", new JSONArray(new String[] {"123", "blah"}));
1244+
1245+
try {
1246+
DynamicMessage protoMsg =
1247+
JsonToProtoMessage.convertJsonToProtoMessage(RepeatedBytes.getDescriptor(), ts, json);
1248+
Assert.fail("Should fail");
1249+
} catch (Exceptions.FieldParseError ex) {
1250+
assertEquals(ex.getBqType(), "NUMERIC");
1251+
assertEquals(ex.getFieldName(), "root.test_repeated");
1252+
}
1253+
}
1254+
1255+
@Test
1256+
public void testBadJsonFieldIntRepeated() throws Exception {
1257+
TableSchema ts =
1258+
TableSchema.newBuilder()
1259+
.addFields(
1260+
0,
1261+
TableFieldSchema.newBuilder()
1262+
.setName("test_repeated")
1263+
.setType(TableFieldSchema.Type.DATE)
1264+
.setMode(TableFieldSchema.Mode.REPEATED)
1265+
.build())
1266+
.build();
1267+
JSONObject json = new JSONObject();
1268+
json.put("test_repeated", new JSONArray(new String[] {"blah"}));
1269+
1270+
try {
1271+
DynamicMessage protoMsg =
1272+
JsonToProtoMessage.convertJsonToProtoMessage(RepeatedInt32.getDescriptor(), ts, json);
1273+
Assert.fail("Should fail");
1274+
} catch (IllegalArgumentException ex) {
1275+
assertEquals(ex.getMessage(), "Text 'blah' could not be parsed at index 0");
1276+
}
1277+
}
12291278
}

0 commit comments

Comments
 (0)