Avro
  1. Avro
  2. AVRO-1544

Union of enum and null can result in NPE on validate if null is not first

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.6
    • Fix Version/s: 1.7.8
    • Component/s: java
    • Labels:
      None

      Description

      The ENUM case in validate is missing a null check for datum:

          case ENUM:
            return schema.getEnumSymbols().contains(datum.toString());
      

      This leads to a surprising error where a NPE is thrown for a union of enum and null when the enum is first in the union. If null is first it works. The fix is a simple. I'm creating a patch that adds the fix and a unit test for this case.

      1. AVRO-1544-v2.patch
        2 kB
        Matthew Hayes
      2. AVRO-1544.patch
        2 kB
        Matthew Hayes

        Activity

        Hide
        Hudson added a comment -

        SUCCESS: Integrated in AvroJava #471 (See https://builds.apache.org/job/AvroJava/471/)
        AVRO-1544. Java: Fix GenericData#validate for unions with null. Contributed by Matthew Hayes. (cutting: rev 1615764)

        • /avro/trunk/CHANGES.txt
        • /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/generic/GenericData.java
        • /avro/trunk/lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericData.java
        Show
        Hudson added a comment - SUCCESS: Integrated in AvroJava #471 (See https://builds.apache.org/job/AvroJava/471/ ) AVRO-1544 . Java: Fix GenericData#validate for unions with null. Contributed by Matthew Hayes. (cutting: rev 1615764) /avro/trunk/CHANGES.txt /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/generic/GenericData.java /avro/trunk/lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericData.java
        Hide
        Matthew Hayes added a comment -

        Awesome thanks!

        Show
        Matthew Hayes added a comment - Awesome thanks!
        Hide
        Doug Cutting added a comment -

        I committed this. Thanks Matthew!

        Show
        Doug Cutting added a comment - I committed this. Thanks Matthew!
        Hide
        ASF subversion and git services added a comment -

        Commit 1615764 from Doug Cutting in branch 'avro/trunk'
        [ https://svn.apache.org/r1615764 ]

        AVRO-1544. Java: Fix GenericData#validate for unions with null. Contributed by Matthew Hayes.

        Show
        ASF subversion and git services added a comment - Commit 1615764 from Doug Cutting in branch 'avro/trunk' [ https://svn.apache.org/r1615764 ] AVRO-1544 . Java: Fix GenericData#validate for unions with null. Contributed by Matthew Hayes.
        Hide
        Sean Busbey added a comment -

        I think the patch is ready to apply, but am sadly not a committer.

        Show
        Sean Busbey added a comment - I think the patch is ready to apply, but am sadly not a committer.
        Hide
        Matthew Hayes added a comment -

        Any objections to applying the patch?

        Show
        Matthew Hayes added a comment - Any objections to applying the patch?
        Hide
        Sean Busbey added a comment -

        +1 LGTM. Ran through without fix and test failed, after fix test passed.

        Show
        Sean Busbey added a comment - +1 LGTM. Ran through without fix and test failed, after fix test passed.
        Hide
        Matthew Hayes added a comment -

        Thanks for taking a look. Attaching v2 of patch, addressing all three issues raised.

        Show
        Matthew Hayes added a comment - Thanks for taking a look. Attaching v2 of patch, addressing all three issues raised.
        Hide
        Sean Busbey added a comment -

        AVRO-997 is def not backwards compatible. It's waiting for 1.8.0. Once this goes in, I'll rebase it onto the current trunk so it can happily keep waiting.

        A few points of feedback:

        • TestGenericData has several lines with trailing whitespace, could you clean those up?
        • Could you change the test to use GenericData.EnumSymbol instead of String? (that way the tests won't have to change when 1.8.0 changes to strict enforcement of use of GenericData.EnumSymbol.)
        • How about changing the check to be consistent with the fail-first style elsewhere in GenericData? e.g.
          + if (datum == null) return false;
          return schema.getEnumSymbols().contains(datum.toString());
          
        Show
        Sean Busbey added a comment - AVRO-997 is def not backwards compatible. It's waiting for 1.8.0. Once this goes in, I'll rebase it onto the current trunk so it can happily keep waiting. A few points of feedback: TestGenericData has several lines with trailing whitespace, could you clean those up? Could you change the test to use GenericData.EnumSymbol instead of String? (that way the tests won't have to change when 1.8.0 changes to strict enforcement of use of GenericData.EnumSymbol.) How about changing the check to be consistent with the fail-first style elsewhere in GenericData? e.g. + if (datum == null ) return false ; return schema.getEnumSymbols().contains(datum.toString());
        Hide
        Matthew Hayes added a comment -

        Attaching patch with null check and unit test exposing the issue.

        Testing done: "mvn test"

        To run the single test: "mvn -Dtest=TestGenericData#testValidateNullableEnum test"

        Note that this is somewhat related to AVRO-997. I see that the latest patch there includes this change:

             case ENUM:
        +      if (!isEnum(datum)) return false;
               return schema.getEnumSymbols().contains(datum.toString());
        

        I believe this change would probably address this issue as well. However, I think this particular change may not be backwards compatible. Currently you can pass in strings as I do in the unit test to validate and it works.

        Show
        Matthew Hayes added a comment - Attaching patch with null check and unit test exposing the issue. Testing done: "mvn test" To run the single test: "mvn -Dtest=TestGenericData#testValidateNullableEnum test" Note that this is somewhat related to AVRO-997 . I see that the latest patch there includes this change: case ENUM: + if (!isEnum(datum)) return false ; return schema.getEnumSymbols().contains(datum.toString()); I believe this change would probably address this issue as well. However, I think this particular change may not be backwards compatible. Currently you can pass in strings as I do in the unit test to validate and it works.

          People

          • Assignee:
            Matthew Hayes
            Reporter:
            Matthew Hayes
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development