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 2021/03/25 16:17:58 UTC
[GitHub] [avro] subhashb opened a new pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
subhashb opened a new pull request #1156:
URL: https://github.com/apache/avro/pull/1156
The exception message now includes the field name on which the type exception was raised.
Closes: AVRO-2720
Make sure you have checked _all_ steps below.
### Jira
- [x] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title.
- https://issues.apache.org/jira/browse/AVRO-2720
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [x] 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 the 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
- [x] In case of new functionality, my PR adds documentation that describes how to use it.
NA
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
kojiromike commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876512539
@subhashb I'm still seeing an error in github:
--
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] kojiromike commented on a change in pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
kojiromike commented on a change in pull request #1156:
URL: https://github.com/apache/avro/pull/1156#discussion_r608734712
##########
File path: lang/py/avro/errors.py
##########
@@ -44,9 +44,9 @@ class IgnoredLogicalType(AvroWarning):
class AvroTypeException(AvroException):
"""Raised when datum is not an example of schema."""
- def __init__(self, expected_schema, datum):
+ def __init__(self, expected_schema, name, datum):
Review comment:
These exceptions predate me, but I think it's a mistake that they have required arguments to their `__init__` methods. I'd suggest that they should be polymorphic in that their first argument should be either the concrete exception message _or_ these kwargs.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] subhashb closed pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb closed pull request #1156:
URL: https://github.com/apache/avro/pull/1156
--
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] subhashb commented on a change in pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on a change in pull request #1156:
URL: https://github.com/apache/avro/pull/1156#discussion_r609103745
##########
File path: lang/py/avro/errors.py
##########
@@ -44,9 +44,9 @@ class IgnoredLogicalType(AvroWarning):
class AvroTypeException(AvroException):
"""Raised when datum is not an example of schema."""
- def __init__(self, expected_schema, datum):
+ def __init__(self, expected_schema, name, datum):
Review comment:
Changed in https://github.com/apache/avro/pull/1156/commits/5d34e2c380df85a0d43894f46c1ea01ee2305c1d
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] subhashb commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876503996
@kojiromike I have resynced this branch with `master` and resolved conflicts. There is a flaky test that has failed (`test_minimum_speed`), otherwise looks good.
--
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] kojiromike commented on a change in pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
kojiromike commented on a change in pull request #1156:
URL: https://github.com/apache/avro/pull/1156#discussion_r609685645
##########
File path: lang/py/avro/errors.py
##########
@@ -44,10 +44,18 @@ class IgnoredLogicalType(AvroWarning):
class AvroTypeException(AvroException):
"""Raised when datum is not an example of schema."""
- def __init__(self, expected_schema, datum):
- pretty_expected = json.dumps(json.loads(str(expected_schema)), indent=2)
- fail_msg = "The datum {} is not an example of the schema {}".format(datum, pretty_expected)
- super(AvroTypeException, self).__init__(fail_msg)
+ def __init__(self, **kwargs):
+ if any(key not in kwargs.keys() for key in ['expected_schema', 'name', 'datum']):
Review comment:
Oops, this is not what I meant. For now, I think what you had before was OK. We just need to recognize that these exception classes are not an external or supported API for downstream users who want to extend them.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] subhashb commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-815302833
@kojiromike Addressed review comments.
[SchemaResolutionException](https://github.com/apache/avro/blob/42822886c28ea74a744abb7e7a80a942c540faa5/lang/py3/avro/io.py#L82) still accepts named arguments. I thought we can retain it as-is for now. Let me know if you think otherwise.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] subhashb commented on a change in pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on a change in pull request #1156:
URL: https://github.com/apache/avro/pull/1156#discussion_r609103608
##########
File path: lang/py/avro/test/test_io.py
##########
@@ -390,14 +391,39 @@ def test_field_order(self):
print('Datum Read: %s' % datum_read)
self.assertEqual(datum_to_read, datum_read)
- def test_type_exception(self):
+ def test_type_exception_int(self):
print_test_name('TEST TYPE EXCEPTION')
- writers_schema = avro.schema.parse("""\
- {"type": "record", "name": "Test",
- "fields": [{"name": "F", "type": "int"},
- {"name": "E", "type": "int"}]}""")
+ writers_schema = avro.schema.parse(json.dumps({
+ "type": "record", "name": "Test",
+ "fields": [
+ {"name": "F", "type": "int"},
+ {"name": "E", "type": "int"}]}))
datum_to_write = {'E': 5, 'F': 'Bad'}
- self.assertRaises(avro.errors.AvroTypeException, write_datum, datum_to_write, writers_schema)
+ with self.assertRaises(avro.errors.AvroTypeException) as exc:
+ write_datum(datum_to_write, writers_schema)
+ assert str(exc.exception) == 'The datum "Bad" provided for "F" is not an example of the schema "int"'
+
+ def test_type_exception_long(self):
+ writers_schema = avro.schema.parse(json.dumps({
+ "type": "record", "name": "Test",
+ "fields": [
+ {"name": "foo", "type": "long"}]}))
+ datum_to_write = {'foo': 5.0}
+ with self.assertRaises(avro.errors.AvroTypeException) as exc:
+ write_datum(datum_to_write, writers_schema)
+ assert str(exc.exception) == 'The datum "5.0" provided for "foo" is not an example of the schema "long"'
+
+ def test_type_exception_record(self):
+ writers_schema = avro.schema.parse(json.dumps({
+ "type": "record", "name": "Test",
+ "fields": [
+ {"name": "foo", "type": "long"}]}))
+ datum_to_write = ('foo', 5.0)
+ pretty_expected = json.dumps(json.loads(str(writers_schema)), indent=2)
+ with self.assertRaises(avro.errors.AvroTypeException) as exc:
+ write_datum(datum_to_write, writers_schema)
+ assert str(exc.exception) == \
Review comment:
Changed in https://github.com/apache/avro/pull/1156/commits/5d34e2c380df85a0d43894f46c1ea01ee2305c1d
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] subhashb commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-877229720
Apparently, rebasing the 6 commits into master causes conflicts since they are applied one by one. `merge` doesn't have this issue because it applies the last state onto master but results in loss of history.
Closing this PR in favor of a clean slate PR #1287.
--
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] subhashb commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876527284
Ha! That's strange. I see that this branch has no conflicts.
Is Github showing an older version for you? How many commits do you see? There should be 6 in total.
--
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] kojiromike commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
kojiromike commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876552015
I see six commits, yes. But I still see the error.
--
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] kojiromike commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
kojiromike commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876873393
@subhashb if I check out this PR by doing `gh pr checkout 1156` (using the [github cli](https://cli.github.com/)) and then rebase master into it, I do see conflicts:
```
22:38:37 $ gh pr checkout 1156
remote: Enumerating objects: 46, done.
remote: Counting objects: 100% (41/41), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 46 (delta 39), reused 40 (delta 39), pack-reused 5
Unpacking objects: 100% (46/46), 15.63 KiB | 432.00 KiB/s, done.
From github.com:apache/avro
* [new ref] refs/pull/1156/head -> 2720-display-invalid-field-data
Switched to branch '2720-display-invalid-field-data'
22:38:47 $ git status
On branch 2720-display-invalid-field-data
nothing to commit, working tree clean
22:38:49 $ git rebase origin/master
Auto-merging lang/py/avro/test/test_io.py
CONFLICT (content): Merge conflict in lang/py/avro/test/test_io.py
Auto-merging lang/py/avro/io.py
CONFLICT (content): Merge conflict in lang/py/avro/io.py
Auto-merging lang/py/avro/errors.py
CONFLICT (content): Merge conflict in lang/py/avro/errors.py
error: could not apply 6c84ab2af... AVRO-2720: Enhance AvroTypeException message to include field name
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 6c84ab2af... AVRO-2720: Enhance AvroTypeException message to include field name
```
--
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] subhashb commented on a change in pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on a change in pull request #1156:
URL: https://github.com/apache/avro/pull/1156#discussion_r610087922
##########
File path: lang/py/avro/errors.py
##########
@@ -44,10 +44,18 @@ class IgnoredLogicalType(AvroWarning):
class AvroTypeException(AvroException):
"""Raised when datum is not an example of schema."""
- def __init__(self, expected_schema, datum):
- pretty_expected = json.dumps(json.loads(str(expected_schema)), indent=2)
- fail_msg = "The datum {} is not an example of the schema {}".format(datum, pretty_expected)
- super(AvroTypeException, self).__init__(fail_msg)
+ def __init__(self, **kwargs):
+ if any(key not in kwargs.keys() for key in ['expected_schema', 'name', 'datum']):
Review comment:
Agree 👍
Reverted to its original form in https://github.com/apache/avro/pull/1156/commits/f4ad82f8afa062d5c70e154d175977b69a5ce320
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] subhashb commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876571581
Hmm, I am not sure what's happening here. I tried merging this branch into master locally, and it worked.
--
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] subhashb commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876828933
Yep, they seem to. 😕
--
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] kojiromike commented on pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
kojiromike commented on pull request #1156:
URL: https://github.com/apache/avro/pull/1156#issuecomment-876592628
@subhashb do the commits listed in github have the same sha hashes as the commits you have locally?
--
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] subhashb commented on a change in pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
subhashb commented on a change in pull request #1156:
URL: https://github.com/apache/avro/pull/1156#discussion_r601643707
##########
File path: lang/py/avro/io.py
##########
@@ -155,7 +155,7 @@ def validate(expected_schema, datum, raise_on_error=False):
else:
# the current node was not valid.
if raise_on_error:
- raise avro.errors.AvroTypeException(current_node.schema, current_node.datum)
+ raise avro.errors.AvroTypeException(current_node.schema, current_node.name, current_node.datum)
Review comment:
Will this be a breaking change? Even if others use AvroTypeException directly, this `init` API should be internal.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a change in pull request #1156: AVRO-2720: Enhance AvroTypeException message to include field name
Posted by GitBox <gi...@apache.org>.
kojiromike commented on a change in pull request #1156:
URL: https://github.com/apache/avro/pull/1156#discussion_r608736873
##########
File path: lang/py/avro/test/test_io.py
##########
@@ -390,14 +391,39 @@ def test_field_order(self):
print('Datum Read: %s' % datum_read)
self.assertEqual(datum_to_read, datum_read)
- def test_type_exception(self):
+ def test_type_exception_int(self):
print_test_name('TEST TYPE EXCEPTION')
- writers_schema = avro.schema.parse("""\
- {"type": "record", "name": "Test",
- "fields": [{"name": "F", "type": "int"},
- {"name": "E", "type": "int"}]}""")
+ writers_schema = avro.schema.parse(json.dumps({
+ "type": "record", "name": "Test",
+ "fields": [
+ {"name": "F", "type": "int"},
+ {"name": "E", "type": "int"}]}))
datum_to_write = {'E': 5, 'F': 'Bad'}
- self.assertRaises(avro.errors.AvroTypeException, write_datum, datum_to_write, writers_schema)
+ with self.assertRaises(avro.errors.AvroTypeException) as exc:
+ write_datum(datum_to_write, writers_schema)
+ assert str(exc.exception) == 'The datum "Bad" provided for "F" is not an example of the schema "int"'
+
+ def test_type_exception_long(self):
+ writers_schema = avro.schema.parse(json.dumps({
+ "type": "record", "name": "Test",
+ "fields": [
+ {"name": "foo", "type": "long"}]}))
+ datum_to_write = {'foo': 5.0}
+ with self.assertRaises(avro.errors.AvroTypeException) as exc:
+ write_datum(datum_to_write, writers_schema)
+ assert str(exc.exception) == 'The datum "5.0" provided for "foo" is not an example of the schema "long"'
+
+ def test_type_exception_record(self):
+ writers_schema = avro.schema.parse(json.dumps({
+ "type": "record", "name": "Test",
+ "fields": [
+ {"name": "foo", "type": "long"}]}))
+ datum_to_write = ('foo', 5.0)
+ pretty_expected = json.dumps(json.loads(str(writers_schema)), indent=2)
+ with self.assertRaises(avro.errors.AvroTypeException) as exc:
+ write_datum(datum_to_write, writers_schema)
+ assert str(exc.exception) == \
Review comment:
Consider `assertRaisesRegex` to test the message.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org