mennanov / fieldmask-utils

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

Convert protobuf message to map after applying a field mask doesn't work #41

Closed marcoshuck closed 4 months ago

marcoshuck commented 5 months ago

Context

I'm trying to convert a proto message to map using this library in order to perform a PATCH update in an endpoint. I'm getting an empty map instead of a single field map with the respective value.

Relevant code

mask := &fieldmaskpb.FieldMask{Paths: []string{"title"}}
mask.Normalize()
task := &tasksv1.Task{
  Title: "test",
}
if !mask.IsValid(task) {
  return nil, errors.New("invalid mask")
}
protoMask, err := fieldmask_utils.MaskFromProtoFieldMask(mask, func(s string) string { return s })
if err != nil {
  return nil, err
}
m := make(map[string]any)
if err = fieldmask_utils.StructToMap(protoMask, task, m); err != nil {
  return nil, err
}
fmt.Println("Map:", m)

Current behavior

m is empty

Expected behavior

m := map[string]any{
  "title": "test",
}

Reference

Protobuf messages: https://github.com/marcoshuck/todo/blob/main/api/tasks/v1/tasks.proto#L60

marcoshuck commented 5 months ago

@mennanov any chances you can take a look at this?

mennanov commented 5 months ago

I'll take a look within the next few days

mennanov commented 5 months ago

I wrote a similar test that passes:

func TestTitleBug(t *testing.T) {
    mask := &fieldmaskpb.FieldMask{Paths: []string{"title"}}
    mask.Normalize()
    task := &testproto.Task{
        Title: "test",
    }
    require.True(t, mask.IsValid(task))
    protoMask, err := fieldmask_utils.MaskFromProtoFieldMask(mask, func(s string) string { return generator.CamelCase(s) })
    require.Nil(t, err)
    m := make(map[string]any)
    err = fieldmask_utils.StructToMap(protoMask, task, m)
    require.Nil(t, err)
    assert.Equal(t, map[string]any{"Title": "test"}, m)
}

Try using generator.CamelCase(s) which converts a lower case field name (in the field mask) into a camel case (golang struct naming).

marcoshuck commented 5 months ago

Thanks for the information, will give it a try! We should probably add this test case as it would help future devs that find this issue.

I can take care of creating a PR for this test and adding some docs about the generator package (which I was completely unaware of).

mennanov commented 5 months ago

A PR is welcomed!

mennanov commented 4 months ago

Resolved in PR #42