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.fix.patch
        6 kB
        James Baldassari
      2. AVRO-996.bug.patch
        3 kB
        Scott Carey

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        21d 40m 1 James Baldassari 07/Feb/12 07:24
        Resolved Resolved Closed Closed
        7d 17h 21m 1 Doug Cutting 15/Feb/12 00:46
        Doug Cutting made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        James Baldassari made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        James Baldassari added a comment -

        Thanks for the review. Committed.

        Show
        James Baldassari added a comment - Thanks for the review. Committed.
        Doug Cutting made changes -
        Assignee James Baldassari [ jbaldassari ]
        Fix Version/s 1.7.0 [ 12318848 ]
        Hide
        Doug Cutting added a comment -

        +1

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

        +1 looks good!

        Show
        Scott Carey added a comment - +1 looks good!
        James Baldassari made changes -
        Attachment AVRO-996.fix.patch [ 12511814 ]
        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
        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 -

        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 -

        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 -

        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
        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.
        Scott Carey made changes -
        Field Original Value New Value
        Attachment AVRO-996.bug.patch [ 12510815 ]
        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.
        Scott Carey created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development