mennanov / fieldmask-utils

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

Fix a bug in MaskInverse.Filter(). #6

Closed mennanov closed 4 years ago

mennanov commented 4 years ago

Make the filtering logic consistent for nil and empty subMasks in MaskInverse.Filter()

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 38


Totals Coverage Status
Change from base Build 35: 0.1%
Covered Lines: 1450
Relevant Lines: 1665

💛 - Coveralls
elvizlai commented 4 years ago

@mennanov should revert... for test case blew, not worked:

the Info.Address kept which should be masked.

func TestStructToStruct_MaskInverseFromMask2(t *testing.T) {
    type Info struct {
        Address string
        Mobile  string
    }

    type User struct {
        Id   string
        Name string
        Info *Info
    }

    userSrc := &User{
        Id:   "uuid",
        Name: "foo",
        Info: &Info{
            Address: "addr1",
            Mobile:  "123",
        },
    }

    userDst := &User{}
    // mask name & info.address
    mask := fieldmask_utils.MaskInverse{"Name": fieldmask_utils.Mask{}, "Info": fieldmask_utils.Mask{"Address": fieldmask_utils.Mask{}}}
    err := fieldmask_utils.StructToStruct(mask, userSrc, userDst)
    require.NoError(t, err)

    assert.Equal(t, &User{
        Id: userSrc.Id,
        Info: &Info{
            Mobile: userSrc.Info.Mobile,
        },
    }, userDst)
}

the previous test case is not correct:

should change from

        Friends: []*testproto.User{
            {Username: "friend1"},
            {Username: "friend2"},
        },

to

        Friends: []*testproto.User{
            {Id: 2},
            {Id: 3},
        },

In this scenario Mask means which to keep. MaskInverse means which to hide.

mennanov commented 4 years ago

Ok, now i see: we understand masks differently. Given the following code:

userSrc := &testproto.User{
                SomeField: "value",
        Friends: []*testproto.User{
            {Id: 2, Username: "friend1"},
            {Id: 3, Username: "friend2"},
        },
    }
// and the fieldmask:
fieldmask_utils.Mask{"Friends": fieldmask_utils.Mask{"Username": fieldmask_utils.Mask{}}}

what would you expect as an output? My expectation is:

testproto.User{
        Friends: []*testproto.User{
            {Username: "friend1"},
            {Username: "friend2"},
        },
    }

It's simple, as you said: Mask selects what needs to be copied, MaskInverse does the opposite.

So why do you expect it to be

Friends: []*testproto.User{
            {Id: 2},
            {Id: 3},
        },

?

MaskInverse may have nested Masks, and they still should behave as Masks, not MaskInverse. The converse is also true: Mask may have any kinds of masks nested in it and they still should behave as advertised. If you want to make all the nested masks in the MaskInverse behave as MaskInverse then they really should be MaskInverse, as i said earlier, just recursively convert all of them to MaskInverse if that's what you want

elvizlai commented 4 years ago

@mennanov Yes, i now know what you mean is

what to erase should be in MaskInverse
what to kept should be in Mask

MaseInverse(A, Mask(B)) --> erase A but keep B

What i know about filed_mask is using paths to convert as Mask and sometimes make it Inverse. Mostly, using DSL to describe it, but not go code(or maybe generated go code).

Using Mask or MaskInverse only, but not combination. Maybe what we need is a simple strategy to convert Mask as MaskInverse.

mennanov commented 4 years ago

Ok, then we need something like https://github.com/mennanov/fieldmask-utils/blob/master/mask.go#L102

func MaskInverseFromPaths(paths []string, naming func(string) string) (MaskInverse, error)

Will that help you? I can implement this function later today

elvizlai commented 4 years ago

@mennanov yes, that should be fine, thanks.

https://github.com/mennanov/fieldmask-utils/blob/4892f147ae6408ffccaf5eeecd3b0572b17e59aa/mask.go#L98

using fm.GetPaths() instead of fm.Paths, or fm may nil