mennanov / fieldmask-utils

Protobuf Field Mask Go utils
MIT License
229 stars 26 forks source link

Ambiguous merging of nullable structs with StructToStruct #45

Open uabjabborov opened 2 weeks ago

uabjabborov commented 2 weeks ago

The following new test case for copy.go is failing. I added it in my local copy.

I wonder what the correct behavior should be in this case.

func TestStructToStruct_NestedNilParentStruct_NonNilDst(t *testing.T) {
    type A struct {
        Field1 string
        Field2 int
    }
    type B struct {
        Field1 string
        Field2 int
        A      *A
    }
    type C struct {
        Field1 string
        B      B
    }

    // Given: src.B.A is nil, dst.B.A is not nil
    src := &C{
        Field1: "src C field1",
        B: B{
            Field1: "src StringerB field1",
            Field2: 1,
            A:      nil, // nil struct
        },
    }
    dst := &C{
        Field1: "dst C field1",
        B: B{
            Field1: "dst StringerB field1",
            Field2: 2,
            A: &A{
                Field1: "dst StringerA field1",
                Field2: 10,
            },
        },
    }

    // Given: mask contains subfield of nil field in src, i.e. src.B.A
    mask := fieldmask_utils.MaskFromString("B{Field1,A{Field2}}")

    // When: StructToStruct is called
    err := fieldmask_utils.StructToStruct(mask, src, dst)

    // Then: no error is returned
    require.NoError(t, err)

    // Then: dst B.A is not modified
    assert.Equal(t, &C{
        Field1: "dst C field1",
        B: B{
            Field1: src.B.Field1,
            Field2: 2,
            A: &A{
                Field1: "dst StringerA field1",
                Field2: 10,
            },
        },
    }, dst)
}
mennanov commented 1 week ago

I think the current behavior is correct: if the source is nil then the destination should also be nil as a result.

If you need to keep the non-nil destination fields then you would need smth like a MapVisitor but for the structToStruct function. It is not implemented yet, contributions are welcome though.

If you decide to implement it then consider adding a generic visitor option that can be used in both functions: StructToMap and StructToStruct