Avro
  1. Avro
  2. AVRO-996

Java: SpecificRecord builder pattern object copy fails with unions in some cases

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6.0, 1.6.1
    • Fix Version/s: 1.6.2
    • Component/s: java
    • Labels:
      None

      Description

      I have a union of multiple records that fails to copy correctly with the MySpecificRecord.newBuilder(recordToCopy).build(); pattern.

      I have a reproducible test case that I will attach shortly.

      1. AVRO-996.bug.patch
        3 kB
        Scott Carey
      2. AVRO-996.fix.patch
        6 kB
        James Baldassari

        Activity

        Hide
        Scott Carey added a comment -

        contains a new context.avdl with a record containing a union of records, and a test that fails when using it.

        We probably need a Specific record for testing containing every type, and some non-trivial complex unions, arrays, and maps as well as various field with and without defaults.

        Show
        Scott Carey added a comment - contains a new context.avdl with a record containing a union of records, and a test that fails when using it. We probably need a Specific record for testing containing every type, and some non-trivial complex unions, arrays, and maps as well as various field with and without defaults.
        Hide
        Scott Carey added a comment -

        I would like to see this bug fixed for 1.6.2+ but I have not had time to look into it in more detail and cannot do so for the next couple weeks.

        Show
        Scott Carey added a comment - I would like to see this bug fixed for 1.6.2+ but I have not had time to look into it in more detail and cannot do so for the next couple weeks.
        Hide
        James Baldassari added a comment -

        I'll try to find some time this week to take a look at it. Thanks for attaching the test case.

        Show
        James Baldassari added a comment - I'll try to find some time this week to take a look at it. Thanks for attaching the test case.
        Hide
        James Baldassari added a comment -

        I had some time to work on it tonight. I found that the root cause of this bug was the incorrect handling of union types by GenericData#deepCopy(Schema, Object). Patch coming soon.

        Show
        James Baldassari added a comment - I had some time to work on it tonight. I found that the root cause of this bug was the incorrect handling of union types by GenericData#deepCopy(Schema, Object). Patch coming soon.
        Hide
        James Baldassari added a comment -

        While investigating this issue I also found a couple of other bugs related to the specific record builder, this time in the generated code:

        • The build() method does not validate that all required (i.e. non-nullable) fields have been set
        • It's possible for build() to throw NPE when defaultValue(Field) returns null and then the result of that call is set in a primitive field

        I'm also going to include the fixes for the above issues in this patch, unless anyone thinks it would be better to split them out into a separate issue/patch.

        Show
        James Baldassari added a comment - While investigating this issue I also found a couple of other bugs related to the specific record builder, this time in the generated code: The build() method does not validate that all required (i.e. non-nullable) fields have been set It's possible for build() to throw NPE when defaultValue(Field) returns null and then the result of that call is set in a primitive field I'm also going to include the fixes for the above issues in this patch, unless anyone thinks it would be better to split them out into a separate issue/patch.
        Hide
        James Baldassari added a comment -

        Thinking about this some more, fixing the first issue (lack of validation in build()) may technically not be backwards-compatible. The code will be, but the behavior might not be. The second change (fixing the NPE in build()) should be fine.

        Show
        James Baldassari added a comment - Thinking about this some more, fixing the first issue (lack of validation in build()) may technically not be backwards-compatible. The code will be, but the behavior might not be. The second change (fixing the NPE in build()) should be fine.
        Hide
        James Baldassari added a comment -

        Here's a patch that fixes union copying in a builder. This includes Scott's original test (which now passes) as well as a new test I created which copies Interop instances. I decided not to include the other fixes I mentioned in this patch. I'll create separate issues for those.

        Show
        James Baldassari added a comment - Here's a patch that fixes union copying in a builder. This includes Scott's original test (which now passes) as well as a new test I created which copies Interop instances. I decided not to include the other fixes I mentioned in this patch. I'll create separate issues for those.
        Hide
        Scott Carey added a comment -

        +1 looks good!

        Show
        Scott Carey added a comment - +1 looks good!
        Hide
        Doug Cutting added a comment -

        +1

        Show
        Doug Cutting added a comment - +1
        Hide
        James Baldassari added a comment -

        Thanks for the review. Committed.

        Show
        James Baldassari added a comment - Thanks for the review. Committed.

          People

          • Assignee:
            James Baldassari
            Reporter:
            Scott Carey
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development