thethingsindustries / protoc-gen-fieldmask

Generate field mask utilities from proto files

protoc-gen-fieldmask

A protoc plug-in, which generates fieldmask utilities. Compatible with gogoproto extensions.

Installation:

GO111MODULE=on go install .

Usage:

For example, from root of this repository:

protoc -Itestdata -Ivendor --fieldmask_out=lang=gogo:$GOPATH/src testdata/testdata.proto

Note, you will need to run GO111MODULE=on go mod vendor before running the command above.

protoc-gen-fieldmask's Issues

Wrong oneof sub-fields

Summary

Nested oneOf use the wrong sub-paths during SetFields.

Steps to Reproduce

1. Create a message which contains two sub messages (i.e make messages instead of trivial types part of the one-of).
2. Try to use SetFields with a valid field mask.
3. The validation fails.

I've encountered while working on SetApplicationPubSub

https://github.com/TheThingsNetwork/lorawan-stack/blob/67cc2b31b079f9e9162f997306386a22b4bccf18/api/applicationserver_pubsub.proto#L72-L77

The generated code uses the wrong subfields while recursing on the oneof (subs instead of oneOfSubs).
https://github.com/TheThingsNetwork/lorawan-stack/blob/67cc2b31b079f9e9162f997306386a22b4bccf18/pkg/ttnpb/applicationserver_pubsub.pb.setters.fm.go#L362-L385

What do you see now?

The subfields of the upper message are used (subs).

What do you want to see instead?

The subfields of the oneof message being used (oneOfSubs).

Environment

How do you propose to implement this?

Changing the code generation to respect the correct subs.

Can you do this yourself and submit a Pull Request?

Yes

Rewrite modules using templates

Summary:

Modules at modules should be refactored to use templates (https://golang.org/pkg/text/template) instead of using strings.Builder for clarity.

What is already there? What do you see now?

strings.Builder and bunch of Write calls

What is missing? What do you want to see?

Clear templates
See https://github.com/lyft/protoc-gen-validate/tree/v0.0.13/templates for an example

How do you propose implementing this?

Rewrite implementations to use templates

Group nested field paths

Summary:

Field paths, which modify the same nested path should be grouped.

Consider

protoc-gen-fieldmask/testdata/testdata.pb.fm.go

Lines 54 to 98 in 7fc131b

	case "a.a":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.a.a":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.a.b":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.a.c":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.a.d":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.b":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.c":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.d":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)
	case "a.e":
	if dst.A == nil {
	dst.A = &Test_TestNested{}
	}
	dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)

Instead of having a case per path, we could have:

case "a.a", "a.a.a", /* other cases... */ "a.e":
   if dst.A == nil {
		dst.A = &Test_TestNested{}
	}
   dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)

or

// `switch {` somewhere above in a loop over paths...
case strings.HasPrefix(path, "a."):
   if dst.A == nil {
		dst.A = &Test_TestNested{}
	}
   dst.A.SetFields(src.A, _pathsWithoutPrefix("a", paths)...)

What is already there? What do you see now?

Case per path

What is missing? What do you want to see?

Grouped paths in a case

How do you propose implementing this?

After generating the path set, handle top-level paths as just set(with =), and group nested paths by the top-level field and generate a case with SetFields per group(using either a for loop and additional switch or simply matching the final fields).

Non-reflective copying

Summary:

Copies should be made without reflection

What is already there? What do you see now?

Copies for some types are made via reflection

What is missing? What do you want to see?

Non-reflective copying for:

• maps
• customtypes
• Any
• Struct

How do you propose implementing this?

• Maps: via parsing the additional *Entry message type(work has been started on this one already in feature/maps)
• Customtypes: via the Marshal/Unmarshal
• Any: ?
• Struct: ?

Group paths

Summary:

{S,G}etFields methods should take paths ...string to []struct{ field string; subPaths []string; } and loop over the result using a switch, which matches by the field field, where if subPaths is non-zero {S,G}etFields with subFields is called and the field is set using = otherwise

Replaces #7

processPaths utility should make a copy of slice before sorting

Summary:

func _processPaths(paths []string) map[string][]string {
	sort.Strings(paths)

This mutates paths, and because SetFields takes paths from callers, it may mutate external path slices. Note that variadic arguments do not make a copy of those slices.

Steps to Reproduce:

1. For the variadic-isn't-copy: https://play.golang.org/p/4jmGeK6efG4
2. We already know that sort mutates

How do you propose implementing this?

Just reassign paths to a copy (paths = append(paths[:0:0], paths...))

What can you do yourself and what do you need help with?

I can do it, but probably faster if you do this for the next version

Oneof fields should be included in path

Summary:

Oneof field names should be included in field paths.

Support field mask for repeated struct

Summary

We need support select specified field for the slice of struct, which would be great.

Why do we need this?

message demo {
  repeated A a=1;
}

message A {
  int32 a=1;
  int32 b=2;
}

In some situations, we need to support return the specified b field to clients only.

What is already there? What do you see now?

...

What is missing? What do you want to see?

...

Environment

...

How do you propose to implement this?

...

Can you do this yourself and submit a Pull Request?

Add fieldmask.enable option to protos

Summary:

Plugin should consider fieldmask.enable option(true by default) and do not generate utilities for protos with fieldmask.enable = false

What is already there? What do you see now?

No option

What is missing? What do you want to see?

Option

How do you propose implementing this?

Parse the option and evaluate

Generate validators per-fieldpath

Summary:

The plugin should generate a func (*T) ValidateFields(...string) error method for each message.
Required by TheThingsNetwork/lorawan-stack#51

What is already there? What do you see now?

Only SetFields

What is missing? What do you want to see?

func (*T) ValidateFields(...string) error

How do you propose implementing this?

Follow the same structure as SetFields, instead of copying fields(terminal paths) - validate it according to the annotations.
If no fields are specified to ValidateFields (i.e. it's called as ValidateFields()), all fields are validated.
This lets ValidateFields have the same recursive structure as SetFields

The validation tags in protos could look the same as https://github.com/lyft/protoc-gen-validate
In fact, we could reuse the tag definitions they already have.

What can you do yourself and what do you need help with?

I will add basic Validate method implementation and extend goType.
@KrishnaIyer will then take over and add actual validator tags and their implementation as needed

Inconsistent spacing in PathBuilder module

Summary:

See TheThingsNetwork/lorawan-stack#194 (comment)

Steps to Reproduce:

1. Generate code
2. Look at it

What do you see now?

TheThingsNetwork/lorawan-stack#194 (comment)

What do you want to see instead?

Not TheThingsNetwork/lorawan-stack#194 (comment)

How do you propose implementing this?

Fix the printing directives

Oneof validation

Summary

Only set oneof should be validated if not explicitly specified

Steps to Reproduce

1. Generate validation code for a oneof

What do you see now?

If no explicit path is supplied to the validator of a oneof, it defaults to validate all possible oneofs

What do you want to see instead?

If no explicit path is supplied to the validator of a oneof, it should only validate the one, which set

How do you propose to implement this?

Check, which oneof is set and validate it

Can you do this yourself and submit a Pull Request?

yes

Migrate to github actions

Summary

Migrate to gihub actions

Why do we need this?

https://github.com/TheThingsIndustries/infrastructure/issues/832

What is already there? What do you see now?

Travis

What is missing? What do you want to see?

Github actions

Environment

CI

How do you propose to implement this?

translate .travis.yml into a github action

Can you do this yourself and submit a Pull Request?

yes

Question on forked dependencies

There are 2 forked dependencies in use: https://github.com/TheThingsIndustries/protoc-gen-validate and https://github.com/TheThingsIndustries/protoc-gen-star

This causes troubles becase all consumers of protoc-gen-fieldmask module also have to add replace directives to their go.mod files, e.g.

replace github.com/lyft/protoc-gen-star => github.com/TheThingsIndustries/protoc-gen-star v0.4.14-gogo.4

replace github.com/envoyproxy/protoc-gen-validate => github.com/TheThingsIndustries/protoc-gen-validate v0.3.0-java-fieldmask.2

Is there any strong reason to keep using these forks? Are this forks long-running i.e. the updates will never land into upstream? If so, maybe we should rework them so only the required functionality is kept, and other is discarded, to break upstream dependency.

As another option, would it make sense to move required functionality from https://github.com/TheThingsIndustries/protoc-gen-validate directly to this repository and again remove the dependency? E.g. as protoc-gen-fieldmask supports only Go we don't need upstream features for Java and Python. Probably we could just copy required code here so it's all in one place.

WDYT?

Sorry if my questions make no sense, as I may have incorrect assumptions about these forks.

Expose top level and nested field mask paths

Summary:

Expose top level and nested field mask paths.

What is already there? What do you see now?

It's not exposed. There is a method on each entity that copies the slice.

In my opinion it feels weird making this a method on the entity, as it has nothing to do with the entity nor is it an interface implementation. Also, copying protects against writing, but I don't think we should bother too much about that.

What is missing? What do you want to see?

Expose the top level and nested field mask paths.

How do you propose implementing this?

Remove the prepending _ and the method.

Do not prefix oneof fields with the oneof type names

Summary

Currently, each oneof field is prefixed with the type name. According to https://developers.google.com/protocol-buffers/docs/proto3#using-oneof

In your generated code, oneof fields have the same getters and setters as regular fields.

and from https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/field-mask

Field masks treat fields in oneofs just as regular fields.

Some background discussed in slack from @rvolosatovs:

So the only reason oneof container name is part of the path is to be able to select the oneof container without knowledge of which is set. E.g. to be able to SetFields on a Message and select the Payload regardless of what the "actual" payload it is.
I was not aware of this spec point when implementing this, so I just did what made sense.
This can be changed, but we should check that we don't have a use case for selecting "either of oneofs"

Why do we need this?

Comply with the spec

What is already there? What do you see now?

Every oneof field is prefixed with the type name, for example generating field masks from:

message Test2 {
    oneof provider {
      bool test_field = 1;
      string test_field2 = 2;
    };
}

results in the following allowed field masks: provider, provider.test_field and provider.test_field2

What is missing? What do you want to see?

Just test_field and test_field2

Environment

v0.3.3

How do you propose to implement this?

I guess this has something to do with these lines

Can you do this yourself and submit a Pull Request?

@rvolosatovs

Make test fails on MacOS

make test fails on MacOS. When not suppressing output in Makefile:

$ make clean
rm -rf .work dist
find ./testdata -name '*.pb.go' -delete -or -name '*.pb.fm.go' -delete
$ make test
Regenerating golden files...
Running tests...
TMPDIR="/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask/.work/tmp.THv" WORKDIR="/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask/.work/tmp.THv" PROTOC="docker run --user `id -u` --rm --mount type=bind,src=/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask,dst=/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask -e IN_TEST -w /Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask thethingsindustries/protoc:3.0.15" go test -regenerate
--- FAIL: TestGolden (1.25s)
    main_test.go:57: Running 'docker run --user 501 --rm --mount type=bind,src=/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask,dst=/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask -e IN_TEST -w /Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask thethingsindustries/protoc:3.0.15 --plugin=protoc-gen-fieldmask=/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask/.work/tmp.THv/go-build351307003/b001/protoc-gen-fieldmask.test -Ivendor -Itestdata --fieldmask_out=/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask/.work/tmp.THv/fieldmask-test141648980 --gogo_out=/Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask/.work/tmp.THv/fieldmask-test141648980 testdata/testdata.proto'...
    main_test.go:60: Output:
        /Users/johan/dev/go/src/github.com/TheThingsIndustries/protoc-gen-fieldmask/.work/tmp.THv/go-build351307003/b001/protoc-gen-fieldmask.test: program not found or is not executable
        --fieldmask_out: protoc-gen-fieldmask: Plugin failed with status code 1.
    main_test.go:66: Error: exit status 1
FAIL
exit status 1
FAIL	github.com/TheThingsIndustries/protoc-gen-fieldmask	1.260s
make: *** [test] Error 1

Fix spelling

Summary:
Fix a minor typo in ttnpb.pb.util.fm.go.

What is already there? What do you see now?

sligthly -> slightly

What is missing? What do you want to see?

Make the fix.

How do you propose implementing this?

A simple spelling correction and rebuilding the generator.

What can you do yourself and what do you need help with?

Yeah one PR coming up.

Support wrapper types

Summary:

Wrappers at https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto are not supported

Steps to Reproduce:

1. 1cd2151

What do you see now?

Unsuported fields

What do you want to see instead?

Supported fields

How do you propose implementing this?

9aa6f4e