[
https://issues.apache.org/jira/browse/AVRO-3375?focusedWorklogId=727777&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-727777
]
ASF GitHub Bot logged work on AVRO-3375:
----------------------------------------
Author: ASF GitHub Bot
Created on: 15/Feb/22 20:05
Start Date: 15/Feb/22 20:05
Worklog Time Spent: 10m
Work Description: martin-g commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807230074
##########
File path:
lang/java/avro/src/main/java/org/apache/avro/TracingAvroTypeException.java
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.avro;
Review comment:
ditto
##########
File path:
lang/java/avro/src/main/java/org/apache/avro/generic/TracingClassCastException.java
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.generic;
Review comment:
Why in `o.a.avro.generic` ?
I'd put all path/tracing related classes in one package, but I admit you
better understand the problem this PR solves than me!
##########
File path:
lang/java/avro/src/main/java/org/apache/avro/TracingAvroTypeException.java
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.avro;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.avro.path.PathElement;
+import org.apache.avro.util.SchemaUtil;
+
+public class TracingAvroTypeException extends AvroTypeException implements
PathTracingException<AvroTypeException> {
+ private final List<PathElement> reversePath;
+
+ public TracingAvroTypeException(AvroTypeException cause) {
+ super(cause.getMessage(), cause);
+ this.reversePath = new ArrayList<>(1); // expected to be short
Review comment:
IMO the expected size should be a bit bigger. Better to waste
(temporarily) few bytes memory than to resize the array.
E.g. it could be `2` or `3`
##########
File path:
lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericDatumWriter.java
##########
@@ -323,14 +326,139 @@ public void writeDoesNotAllowJavaEnumForGenericEnum()
throws IOException {
public void writeFieldWithDefaultWithExplicitNullDefaultInSchema() throws
Exception {
Schema schema = schemaWithExplicitNullDefault();
GenericRecord record = createRecordWithDefaultField(schema);
- writeObject(schema, record);
+ writeObject(record);
}
@Test
public void writeFieldWithDefaultWithoutExplicitNullDefaultInSchema() throws
Exception {
Schema schema = schemaWithoutExplicitNullDefault();
GenericRecord record = createRecordWithDefaultField(schema);
- writeObject(schema, record);
+ writeObject(record);
+ }
+
+ @Test
+ public void testNestedNPEErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.get("a").put("f4", null);
+ try {
+ writeObject(topLevelRecord);
+ Assert.fail("expected to throw");
+ } catch (NullPointerException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
expected.getMessage()
+
.contains("RecordWithRequiredFields.unionField[UnionRecord].arrayField[0].mapField(\"a\").f4"));
+ }
+ }
+
+ @Test
+ public void testNPEForMapKeyErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.put(null, map.get("a")); // value is valid, but key is null
+ try {
+ writeObject(topLevelRecord);
+ Assert.fail("expected to throw");
+ } catch (NullPointerException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
expected.getMessage()
+ .contains("null key in map at
RecordWithRequiredFields.unionField[UnionRecord].arrayField[0].mapField"));
+ }
+ }
+
+ @Test
+ public void testShortPathNPEErrorClarity() throws Exception {
+ try {
+ writeObject(Schema.create(Schema.Type.STRING), null);
+ Assert.fail("expected to throw");
+ } catch (NullPointerException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
+ expected.getMessage().contains("null value for (non-nullable)
string"));
+ }
+ }
+
+ @Test
+ public void testNestedCCEErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.get("a").put("f4", 42); // not a string
+ try {
+ writeObject(topLevelRecord);
+ Assert.fail("expected to throw");
+ } catch (ClassCastException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
expected.getMessage()
+
.contains("RecordWithRequiredFields.unionField[UnionRecord].arrayField[0].mapField(\"a\").f4"));
+ }
+ }
+
+ @Test
+ public void testShortPathCCEErrorClarity() throws Exception {
+ try {
+ writeObject(Schema.create(Schema.Type.STRING), 42);
+ Assert.fail("expected to throw");
+ } catch (ClassCastException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
+ expected.getMessage().contains("value 42 (a java.lang.Integer)
cannot be cast to expected type string"));
+ }
+ }
+
+ @Test
+ public void testNestedATEErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.get("a").put("f3", 42); // not a n enum
Review comment:
s/a n/an/ ?
##########
File path: lang/java/avro/src/main/java/org/apache/avro/path/PathPredicate.java
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.avro.path;
+
+public interface PathPredicate extends PathElement {
Review comment:
What is the purpose of PathPredicate ?
When I read `Predicate` I expect to see boolean-valued expression.
##########
File path: share/test/schemas/RecordWithRequiredFields.avsc
##########
@@ -0,0 +1,53 @@
+{
+ "type" : "record",
+ "name" : "RecordWithRequiredFields",
+ "namespace" : "org.apache.avro",
+ "fields" : [ {
+ "name" : "f0",
Review comment:
IMO it would be easier to understand the test cases if `fN` fields were
named after their types, e.g. `stringField` and `enumField`.
Since this PR makes it easier to pinpoint the problematic field I see no
problem in using `stringField` several types in the schema.
##########
File path:
lang/java/avro/src/main/java/org/apache/avro/generic/TracingClassCastException.java
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.generic;
+
+import org.apache.avro.PathTracingException;
+import org.apache.avro.Schema;
+import org.apache.avro.path.PathElement;
+import org.apache.avro.util.SchemaUtil;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * a {@link NullPointerException} with extra fields used to trace back the path
+ * to a null value through an object graph
+ */
+public class TracingClassCastException extends ClassCastException implements
PathTracingException<ClassCastException> {
+ private final ClassCastException cause;
+ private final Object datum;
+ private final Schema expected;
+ private final boolean customCoderUsed;
+ private final List<PathElement> reversePath;
+
+ public TracingClassCastException(ClassCastException cause, Object datum,
Schema expected, boolean customCoderUsed) {
+ this.cause = cause;
+ this.datum = datum;
+ this.expected = expected;
+ this.customCoderUsed = customCoderUsed;
+ this.reversePath = new ArrayList<>(1); // assume short
+ }
+
+ @Override
+ public void tracePath(PathElement step) {
+ reversePath.add(step);
+ }
+
+ @Override
+ public ClassCastException getCause() {
+ return cause;
+ }
+
+ /**
+ * @return a hopefully helpful error message
+ */
+ @Override
+ public ClassCastException summarize(Schema root) {
+ StringBuilder sb = new StringBuilder();
+ sb.append("value ").append(SchemaUtil.describe(datum));
+ sb.append(" cannot be cast to expected type
").append(SchemaUtil.describe(expected));
+ if (reversePath == null || reversePath.isEmpty()) {
+ // very simple "shallow" NPE, no nesting at all, or custom coders used
means we
+ // have no data
+ if (customCoderUsed) {
+ sb.append(". no further details available as custom coders were used");
Review comment:
s/no/No/
##########
File path:
lang/java/avro/src/main/java/org/apache/avro/PathTracingException.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.avro;
Review comment:
can this class be moved to `o.a.a.path` package too ?
##########
File path:
lang/java/avro/src/main/java/org/apache/avro/generic/TracingNullPointException.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.generic;
+
+import org.apache.avro.PathTracingException;
+import org.apache.avro.Schema;
+import org.apache.avro.path.MapKeyPredicate;
+import org.apache.avro.path.PathElement;
+import org.apache.avro.util.SchemaUtil;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * a {@link NullPointerException} with extra fields used to trace back the path
+ * to a null value through an object graph
+ */
+public class TracingNullPointException extends NullPointerException
+ implements PathTracingException<NullPointerException> {
+ private final NullPointerException cause;
+ private final Schema expected;
+ private final boolean customCoderUsed;
+ private final List<PathElement> reversePath;
+
+ public TracingNullPointException(NullPointerException cause, Schema
expected, boolean customCoderUsed) {
+ this.cause = cause;
+ this.expected = expected;
+ this.customCoderUsed = customCoderUsed;
+ this.reversePath = new ArrayList<>(1); // assume short
+ }
+
+ @Override
+ public void tracePath(PathElement step) {
+ reversePath.add(step);
+ }
+
+ @Override
+ public NullPointerException getCause() {
+ return cause;
+ }
+
+ /**
+ * @return a hopefully helpful error message
+ */
+ @Override
+ public NullPointerException summarize(Schema root) {
+ StringBuilder sb = new StringBuilder();
+ sb.append("null value for (non-nullable) ");
+ if (reversePath == null || reversePath.isEmpty()) {
+ // very simple "shallow" NPE, no nesting at all, or custom coders used
means we
+ // have no data
+ if (customCoderUsed) {
+ sb.append("field or map key. no further details available as custom
coders were used");
Review comment:
s/no/No/
##########
File path:
lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericDatumWriter.java
##########
@@ -323,14 +326,139 @@ public void writeDoesNotAllowJavaEnumForGenericEnum()
throws IOException {
public void writeFieldWithDefaultWithExplicitNullDefaultInSchema() throws
Exception {
Schema schema = schemaWithExplicitNullDefault();
GenericRecord record = createRecordWithDefaultField(schema);
- writeObject(schema, record);
+ writeObject(record);
}
@Test
public void writeFieldWithDefaultWithoutExplicitNullDefaultInSchema() throws
Exception {
Schema schema = schemaWithoutExplicitNullDefault();
GenericRecord record = createRecordWithDefaultField(schema);
- writeObject(schema, record);
+ writeObject(record);
+ }
+
+ @Test
+ public void testNestedNPEErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.get("a").put("f4", null);
+ try {
+ writeObject(topLevelRecord);
+ Assert.fail("expected to throw");
+ } catch (NullPointerException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
expected.getMessage()
+
.contains("RecordWithRequiredFields.unionField[UnionRecord].arrayField[0].mapField(\"a\").f4"));
+ }
+ }
+
+ @Test
+ public void testNPEForMapKeyErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.put(null, map.get("a")); // value is valid, but key is null
+ try {
+ writeObject(topLevelRecord);
+ Assert.fail("expected to throw");
+ } catch (NullPointerException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
expected.getMessage()
+ .contains("null key in map at
RecordWithRequiredFields.unionField[UnionRecord].arrayField[0].mapField"));
+ }
+ }
+
+ @Test
+ public void testShortPathNPEErrorClarity() throws Exception {
+ try {
+ writeObject(Schema.create(Schema.Type.STRING), null);
+ Assert.fail("expected to throw");
+ } catch (NullPointerException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
+ expected.getMessage().contains("null value for (non-nullable)
string"));
+ }
+ }
+
+ @Test
+ public void testNestedCCEErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.get("a").put("f4", 42); // not a string
+ try {
+ writeObject(topLevelRecord);
+ Assert.fail("expected to throw");
+ } catch (ClassCastException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
expected.getMessage()
+
.contains("RecordWithRequiredFields.unionField[UnionRecord].arrayField[0].mapField(\"a\").f4"));
+ }
+ }
+
+ @Test
+ public void testShortPathCCEErrorClarity() throws Exception {
+ try {
+ writeObject(Schema.create(Schema.Type.STRING), 42);
+ Assert.fail("expected to throw");
+ } catch (ClassCastException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
+ expected.getMessage().contains("value 42 (a java.lang.Integer)
cannot be cast to expected type string"));
+ }
+ }
+
+ @Test
+ public void testNestedATEErrorClarity() throws Exception {
+ GenericData.Record topLevelRecord = buildComplexRecord();
+ @SuppressWarnings("unchecked")
+ Map<String, GenericData.Record> map = (Map<String, GenericData.Record>)
((List<GenericData.Record>) ((GenericData.Record) topLevelRecord
+ .get("unionField")).get("arrayField")).get(0).get("mapField");
+ map.get("a").put("f3", 42); // not a n enum
+ try {
+ writeObject(topLevelRecord);
+ Assert.fail("expected to throw");
+ } catch (AvroTypeException expected) {
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
expected.getMessage()
+
.contains("RecordWithRequiredFields.unionField[UnionRecord].arrayField[0].mapField(\"a\").f3"));
+ Assert.assertTrue("unexpected message " + expected.getMessage(),
+ expected.getMessage().contains("42 (a java.lang.Integer)"));
Review comment:
should the exception message say something about `enum` ?
line 415 has a comment mentioning enum
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 727777)
Time Spent: 3h (was: 2h 50m)
> add union branch, array index and map key "path" information to serialization
> errors
> ------------------------------------------------------------------------------------
>
> Key: AVRO-3375
> URL: https://issues.apache.org/jira/browse/AVRO-3375
> Project: Apache Avro
> Issue Type: Bug
> Components: java
> Affects Versions: 1.11.0
> Reporter: radai rosenblatt
> Priority: Major
> Labels: pull-request-available
> Time Spent: 3h
> Remaining Estimate: 0h
>
> https://issues.apache.org/jira/browse/AVRO-2514 improved serialization error
> messages greatly by providing more information about the "location" of a
> piece of erroneous data in the object graph handed to a serializer.
>
> however, a few more things can be improved:
> * no info printed about which union branches were selected/used
> * no info printed about array indexes or map keys
> * (subjective) text could be made more concise.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
