mennanov / fieldmask-utils

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

fix bug fix bug: panic: reflect.Value.Interface: cannot return value obtained from unexported field or method in 'StructToMap' #16

Closed zhuliquan closed 3 years ago

zhuliquan commented 3 years ago

case src struct: type UnexportStruct struct { Field1 *timestamp.Timestamp // github.com/golang/protobuf/ptypes/timestamp Field2 int } mask: []string{"Field1", "Field2"} StructToMap will get panic when deep copy 'state' (unexported field) in timestamp

codecov[bot] commented 3 years ago

Codecov Report

Merging #16 (a50d8e2) into master (7ae38b1) will decrease coverage by 1.17%. The diff coverage is 100.00%.

:exclamation: Current head a50d8e2 differs from pull request most recent head 6d5220d. Consider uploading reports for the commit 6d5220d to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
- Coverage   88.01%   86.83%   -1.18%     
==========================================
  Files           2        2              
  Lines         317      319       +2     
==========================================
- Hits          279      277       -2     
- Misses         20       22       +2     
- Partials       18       20       +2     
Impacted Files Coverage Δ
copy.go 81.59% <100.00%> (-1.83%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7ae38b1...6d5220d. Read the comment docs.

mennanov commented 3 years ago

Please, let me know if this code in this branch fixes your problem: https://github.com/mennanov/fieldmask-utils/tree/issue17_fix

zhuliquan commented 3 years ago

Sorry for late reply, This code: https://github.com/mennanov/fieldmask-utils/tree/issue17_fix can solve my problem.

zhuliquan commented 3 years ago

I Just write go test for StructToStruct without srcField.CanInterface(), It also work, so I think this change (in 1structtoStruct) is meaningless. In structToStruct, program will terminal at #83 if dst.CanSet() && dst.Type().AssignableTo(src.Type()) && filter.IsEmpty() {. It not need deeply copy like StructToMap. If mask contain unexport field, the function will return error (Can't set a value on a destination field...), instead of throwing panic. My test code:

func TestStructToStruct_WithUnExportField(t *testing.T) {
    // struct: timestamp has 'state' which is unexport field in github.com/golang/protobuf/ptypes/timestamp.
    type UnExportStruct struct {
        Field1 *timestamp.Timestamp
        Field2 int
    }
    src := &UnExportStruct{
        Field1: &timestamp.Timestamp{
            Seconds: 1,
            Nanos:   1,
        },
        Field2: 2,
    }

    dst := &UnExportStruct{}
    mask, _ := fieldmask_utils.MaskFromProtoFieldMask(&field_mask.FieldMask{Paths: []string{"Field1", "Field2"}}, func(s string) string { return s })
    err := fieldmask_utils.StructToStruct(mask, src, dst)
    assert.Nil(t, err)
    expected := &UnExportStruct{
        Field1: &timestamp.Timestamp{
            Seconds: 1,
            Nanos:   1,
        },
        Field2: 2,
    }
    assert.Equal(t, expected, dst)
    src.Field1.Seconds = 10000
    dst.Field1.Seconds = 10
    assert.NotEqual(t, dst.Field1.Seconds, src.Field1.Seconds)
}
mennanov commented 3 years ago

Both checks srcField.CanInterface() are needed as with the new version proto generator all proto structs have unexported fields and tests fail without those checks. Please, update to the most recent main branch and check whether it works for you

zhuliquan commented 3 years ago

now code tag v0.4.0 can solve problem (panic: reflect.Value.Interface: cannot return value obtained from unexported field or method in 'StructToMap')