AVRO-1401: @Nullable does not work with byte[] #229
Conversation
| @@ -181,13 +181,15 @@ protected boolean isRecord(Object datum) { | |||
|
|
|||
| /** | |||
| * Returns true also for non-string-keyed maps, which are written as an array | |||
| * of key/value pair records. | |||
| * of key/value pair records. Returns false for array of bytes, since it should | |||
There was a problem hiding this comment.
I find it a bit strange that the javadoc comment only documents the exception cases and not the main behavior. I would suggest the following instead:
Returns true for arrays and false otherwise, with the following exceptions:
- Returns true for non-string-keyed maps, which are written as an array of key/value pair records.
- Returns false for arrays of bytes, since those should be treated as byte data type instead.
| return (datum instanceof Collection) | ||
| || datum.getClass().isArray() | ||
| || (c.isArray() && !(c.getComponentType() == Byte.TYPE)) |
There was a problem hiding this comment.
I would suggest c.getComponentType() != Byte.TYPE instead of !(c.getComponentType() == Byte.TYPE)
| } | ||
|
|
||
| @Test | ||
| public void testNullableByteArray() throws IOException { |
There was a problem hiding this comment.
Since there are no asserts in this test, could you document in a comment how this test would fail without the fix? Thanks!
There was a problem hiding this comment.
Instead of documenting the test, I rewrote it. It asserts for the output in the updated version
|
@zivanfi thanks for you comments. Updated the pull request, could you please have a look at it and share you opinion? |
| * of key/value pair records. | ||
| * Returns true for arrays and false otherwise, with the following exceptions: | ||
| * <ul> | ||
| * <li><p>Returns true for non-string-keyed maps, which are written as an array of key/value pair records.</p></li> |
There was a problem hiding this comment.
Nit: No need for the <p>-s inside the <li>-s.
| * <ul> | ||
| * <li><p>Returns true for non-string-keyed maps, which are written as an array of key/value pair records.</p></li> | ||
| * <li><p>Returns false for arrays of bytes, since those should be treated as byte data type instead.</p></li> | ||
| * <ul> |
|
Fixed closing tag for list. |
|
|
||
| @Test | ||
| public void testNullableByteArray() throws Exception { | ||
| checkReadWrite(new NullableBytesTest()); |
There was a problem hiding this comment.
As we are testing a nullable field I would also test the scenario when this field is really null.
There was a problem hiding this comment.
@gszadovszky updated the pul request with an additional test case to cover null field value.
Fix vulnerabilities in transitive dependencies
No description provided.