You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/02/07 23:59:43 UTC
[GitHub] [avro] radai-rosenblatt opened a new pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
radai-rosenblatt opened a new pull request #1526:
URL: https://github.com/apache/avro/pull/1526
Make sure you have checked _all_ steps below.
### Jira
- [ ] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-3375
- In case you are adding a dependency, check if the license complies with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
- [ ] only a single test so far, just to demo the issue
### Commits
- [ ] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on a change in pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r808992349
##########
File path: lang/java/avro/src/main/java/org/apache/avro/path/package-info.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.
+ */
+
+/**
+ * Interfaces and base classes for AvroPath. This functionality is
Review comment:
Perfect!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807591360
##########
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:
do you maybe mean why i added all the intermediate interfaces with no content?
reason for that is that if a full blown AvroPath implementation does follow these interfaces will likely be required.
I can remove all the intermediates if thats the concern
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1042970809
i'd appreciate a 1.11.1, but i'll take what i can get
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1042936631
@RyanSkraba i see master is already 1.12 - can i file a backport PR for this into 1.11 after this one is merged to master?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] edited a comment on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] edited a comment on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
❌ Deploy Preview for *avro-website-preview* failed.
🔨 Explore the source changes: a0652c9b97aa8bf47f67209fd215aeffa914d88b
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/62052fda2535bc00087ac649](https://app.netlify.com/sites/avro-website-preview/deploys/62052fda2535bc00087ac649)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1042406980
synchronized is back, package-info.java and package.html added
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807297220
##########
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:
will do
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035400286
Hello! This looks like a really interesting and *really* compelling feature, and also *really quite close* to a solution to AVRO-992!
(Where something like AVRO-992 would be a selector into an avro datum (probably a record), you've defined a selector into a schema.)
This might be better served by writing up a little spec for your pathstep and getting feedback on that in the mailing list (or by comments on a file in the PR). What would be really amazing would be to start with a "standard" way to express error messages across SDKs, and finish with tools to transform schemas based on these selectors.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] edited a comment on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] edited a comment on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
👷 Deploy Preview for *avro-website-preview* processing.
🔨 Explore the source changes: 0e62671a795148e34861ae70f17c3a9bf7b7d73e
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/620530814225830008b0da73](https://app.netlify.com/sites/avro-website-preview/deploys/620530814225830008b0da73)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1039668331
@RyanSkraba @martin-g - Code has been cleaned up and the "path steps" have been converted over to something that might one day form part of an AvroPath implementation.
i'll send out an email to the dev mailing list later today about formalizing at least enough syntax for tracking the locations of errors in graphs, but otherrwise this PR is ready for review
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807297907
##########
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:
i think simply because i initially solved for generic records. i will be moving all tracking* classes to the path package
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r801818332
##########
File path: lang/java/avro/src/main/java/org/apache/avro/generic/TracingNullPointException.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.Schema;
+
+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 {
+ private final NullPointerException cause;
+ private final Schema expected;
+ private final List<PathStep> reversePath;
+
+ public TracingNullPointException(NullPointerException cause, Schema expected) {
+ this.cause = cause;
+ this.expected = expected;
+ this.reversePath = new ArrayList<>(1); //assume short
+ }
+
+ public void tracePath(PathStep step) {
+ reversePath.add(step);
+ }
+
+ @Override
+ public synchronized NullPointerException getCause() {
Review comment:
thats the parent signature, so thats how my IDE copied it. no real reason though - so will fix
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] edited a comment on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] edited a comment on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
❌ Deploy Preview for *avro-website-preview* failed.
🔨 Explore the source changes: 6ea00047c52b642ef3a01a7606bdfac276f94eb0
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/62052f3913ccf6000974d347](https://app.netlify.com/sites/avro-website-preview/deploys/62052f3913ccf6000974d347)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807306981
##########
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:
PathElement is clear.
I don't understand the purpose of the specialization (PathPredicate, or any other name for it).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1044041569
Cherry-picked to [branch-1.11](https://github.com/apache/avro/commit/b0fbd92ddce1f44bd7b8b41f14069ec2e360087b).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807602502
##########
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:
Let's see what others will say.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r801817666
##########
File path: lang/java/avro/src/main/java/org/apache/avro/generic/TracingNullPointException.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.Schema;
+
+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 {
+ private final NullPointerException cause;
+ private final Schema expected;
+ private final List<PathStep> reversePath;
+
+ public TracingNullPointException(NullPointerException cause, Schema expected) {
+ this.cause = cause;
+ this.expected = expected;
+ this.reversePath = new ArrayList<>(1); //assume short
+ }
+
+ public void tracePath(PathStep step) {
+ reversePath.add(step);
+ }
+
+ @Override
+ public synchronized NullPointerException getCause() {
+ return cause;
+ }
+
+ /**
+ * @return a hopefully helpful error message
+ */
+ public NullPointerException summarize() {
+ StringBuilder sb = new StringBuilder();
+ PathStep lastStep = reversePath.get(0);
Review comment:
will fix
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] edited a comment on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] edited a comment on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
❌ Deploy Preview for *avro-website-preview* failed.
🔨 Explore the source changes: 0e62671a795148e34861ae70f17c3a9bf7b7d73e
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/620530814225830008b0da73](https://app.netlify.com/sites/avro-website-preview/deploys/620530814225830008b0da73)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] edited a comment on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] edited a comment on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
👷 Deploy Preview for *avro-website-preview* processing.
🔨 Explore the source changes: a0652c9b97aa8bf47f67209fd215aeffa914d88b
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/62052fda2535bc00087ac649](https://app.netlify.com/sites/avro-website-preview/deploys/62052fda2535bc00087ac649)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807299806
##########
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:
jspath calls these "location paths" - https://github.com/dfilatov/jspath#location-path. i'll rename to something similar. LocationElement perhaps?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807300253
##########
File path: share/test/schemas/RecordWithRequiredFields.avsc
##########
@@ -0,0 +1,53 @@
+{
+ "type" : "record",
+ "name" : "RecordWithRequiredFields",
+ "namespace" : "org.apache.avro",
+ "fields" : [ {
+ "name" : "f0",
Review comment:
will do
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035520355
yeah I agree there's a very large overlap between my step classes and avro-992 (not 100% though - for example the step for null map keys).
i'll start moving my path classes to a more "api-like" location and see if i can come up with a spec.
i'll probably base it on the eventual implementation they wrote - https://github.com/dcaoyuan/avpath
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1039668331
@RyanSkraba @martin-g - Code has been cleaned up and the "path steps" have been converted over to something that might one day form part of an AvroPath implementation.
i'll send out an email to the dev mailing list later today about formalizing at least enough syntax for tracking the locations of errors in graphs, but otherrwise this PR is ready for review
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] edited a comment on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] edited a comment on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
❌ Deploy Preview for *avro-website-preview* failed.
🔨 Explore the source changes: ffd8809c96d51a826a3a44cdb4ed9dc7b8b2e566
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/6205321813ccf60009754781](https://app.netlify.com/sites/avro-website-preview/deploys/6205321813ccf60009754781)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1042997847
Hmm I don't consider error messages and strings to be part of the API or a breaking change! I vote for cherry-picking to 1.11.1 (remember that it's not semver... as awkward as it is, we've always consider these to be minor and not patch releases).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1042989431
Well, I am not sure. There are also other new features in branch-1.11, especially in the Rust SDK.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba merged pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged pull request #1526:
URL: https://github.com/apache/avro/pull/1526
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] edited a comment on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] edited a comment on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
👷 Deploy Preview for *avro-website-preview* processing.
🔨 Explore the source changes: ffd8809c96d51a826a3a44cdb4ed9dc7b8b2e566
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/6205321813ccf60009754781](https://app.netlify.com/sites/avro-website-preview/deploys/6205321813ccf60009754781)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r801713057
##########
File path: lang/java/avro/src/main/java/org/apache/avro/generic/TracingNullPointException.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.Schema;
+
+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 {
+ private final NullPointerException cause;
+ private final Schema expected;
+ private final List<PathStep> reversePath;
+
+ public TracingNullPointException(NullPointerException cause, Schema expected) {
+ this.cause = cause;
+ this.expected = expected;
+ this.reversePath = new ArrayList<>(1); //assume short
+ }
+
+ public void tracePath(PathStep step) {
+ reversePath.add(step);
+ }
+
+ @Override
+ public synchronized NullPointerException getCause() {
Review comment:
why `synchronized` ?
##########
File path: lang/java/avro/src/main/java/org/apache/avro/generic/TracingNullPointException.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.Schema;
+
+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 {
+ private final NullPointerException cause;
+ private final Schema expected;
+ private final List<PathStep> reversePath;
+
+ public TracingNullPointException(NullPointerException cause, Schema expected) {
+ this.cause = cause;
+ this.expected = expected;
+ this.reversePath = new ArrayList<>(1); //assume short
+ }
+
+ public void tracePath(PathStep step) {
+ reversePath.add(step);
+ }
+
+ @Override
+ public synchronized NullPointerException getCause() {
+ return cause;
+ }
+
+ /**
+ * @return a hopefully helpful error message
+ */
+ public NullPointerException summarize() {
+ StringBuilder sb = new StringBuilder();
+ PathStep lastStep = reversePath.get(0);
Review comment:
it looks possible this may throw IndexOutOfBoundException
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1033271915
@martin-g - addressed your comments (and also handled the case of a null map key, which i dont think avro currently handles correctly).
im going to replicate this work for other exception types and writer subclasses
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] netlify[bot] commented on pull request #1526: WIP AVRO-3375 - initial impl for better NPE messages
Posted by GitBox <gi...@apache.org>.
netlify[bot] commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1035055291
👷 Deploy Preview for *avro-website-preview* processing.
🔨 Explore the source changes: 6ea00047c52b642ef3a01a7606bdfac276f94eb0
🔍 Inspect the deploy log: [https://app.netlify.com/sites/avro-website-preview/deploys/62052f3913ccf6000974d347](https://app.netlify.com/sites/avro-website-preview/deploys/62052f3913ccf6000974d347)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on a change in pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on a change in pull request #1526:
URL: https://github.com/apache/avro/pull/1526#discussion_r807565398
##########
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:
i've renamed things to more closely follow jspath. i've also added javadocs to all interfaces and classes. hopefully its clearer now?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1042424342
build failure is a connection timeout downloading https://repo.maven.apache.org/maven2/commons-cli/commons-cli/1.5.0/commons-cli-1.5.0.pom
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1526: AVRO-3375: Better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1042955631
It is a new feature, so it should be only in 1.12.
Maybe we should release both 1.11.1 and 1.12.0 ?! But it is more work for the release manager.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] radai-rosenblatt commented on pull request #1526: AVRO-3375 - better error messages out of generic and specific DatumWriters
Posted by GitBox <gi...@apache.org>.
radai-rosenblatt commented on pull request #1526:
URL: https://github.com/apache/avro/pull/1526#issuecomment-1041153313
addressed review comments.
also changed MapKeyPredicate to use square brackets like the others for consistency
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org