thethingsindustries / protoc-gen-fieldmask Goto Github PK
View Code? Open in Web Editor NEWGenerate field mask utilities from proto files
License: Apache License 2.0
Generate field mask utilities from proto files
License: Apache License 2.0
Nested oneOf
use the wrong sub-paths during SetFields
.
SetFields
with a valid field mask.I've encountered while working on SetApplicationPubSub
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
The subfields of the upper message are used (subs
).
The subfields of the oneof message being used (oneOfSubs
).
Changing the code generation to respect the correct subs
.
Yes
Only set oneof should be validated if not explicitly specified
If no explicit path is supplied to the validator of a oneof, it defaults to validate all possible oneofs
If no explicit path is supplied to the validator of a oneof, it should only validate the one, which set
Check, which oneof is set and validate it
yes
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.
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"
Comply with the spec
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
Just test_field
and test_field2
v0.3.3
I guess this has something to do with these lines
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
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
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).
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
Summary:
Oneof field names should be included in field paths.
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.
Migrate to gihub actions
https://github.com/TheThingsIndustries/infrastructure/issues/832
Travis
Github actions
CI
translate .travis.yml
into a github action
yes
Summary:
Wrappers at https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto are not supported
Steps to Reproduce:
What do you see now?
Unsuported fields
What do you want to see instead?
Supported fields
How do you propose implementing this?
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:
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
We need support select specified field for the slice of struct, which would be great.
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.
...
...
...
...
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:
How do you propose implementing this?
*Entry
message type(work has been started on this one already in feature/maps
)Marshal
/Unmarshal
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.
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.
Summary:
See TheThingsNetwork/lorawan-stack#194 (comment)
Steps to Reproduce:
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
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
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
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
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.