diff --git a/cmd/protolock/main.go b/cmd/protolock/main.go index 1c79ce9..d876c18 100644 --- a/cmd/protolock/main.go +++ b/cmd/protolock/main.go @@ -51,7 +51,7 @@ var ( func main() { // exit if no command (i.e. help, -h, --help, init, status, or commit) if len(os.Args) < 2 { - fmt.Println(info + usage) + fmt.Print(info + usage) os.Exit(0) } @@ -75,7 +75,7 @@ func main() { // switch through known commands switch os.Args[1] { case "-h", "--help", "help": - fmt.Println(usage) + fmt.Print(usage) case "init": r, err := protolock.Init(*cfg) diff --git a/parse.go b/parse.go index faaba73..85b6c28 100644 --- a/parse.go +++ b/parse.go @@ -80,13 +80,14 @@ type Map struct { } type Field struct { - ID int `json:"id,omitempty"` - Name string `json:"name,omitempty"` - Type string `json:"type,omitempty"` - IsRepeated bool `json:"is_repeated,omitempty"` - IsOptional bool `json:"optional,omitempty"` - IsRequired bool `json:"required,omitempty"` - Options []Option `json:"options,omitempty"` + ID int `json:"id,omitempty"` + Name string `json:"name,omitempty"` + Type string `json:"type,omitempty"` + IsRepeated bool `json:"is_repeated,omitempty"` + IsOptional bool `json:"optional,omitempty"` + IsRequired bool `json:"required,omitempty"` + Options []Option `json:"options,omitempty"` + OneofParent string `json:"oneof_parent,omitempty"` } type Service struct { @@ -326,11 +327,12 @@ func parseMessage(m *proto.Message) Message { for _, el := range oo.Elements { if f, ok := el.(*proto.OneOfField); ok { fields = append(fields, Field{ - ID: f.Sequence, - Name: f.Name, - Type: f.Type, - IsRepeated: false, - Options: parseOptions(f.Options), + ID: f.Sequence, + Name: f.Name, + Type: f.Type, + IsRepeated: false, + Options: parseOptions(f.Options), + OneofParent: oo.Name, }) } } diff --git a/parse_test.go b/parse_test.go index c3c8e1d..507165f 100644 --- a/parse_test.go +++ b/parse_test.go @@ -196,6 +196,22 @@ message TestNestedEnumOption { } ` +const protoWithOneof = ` +syntax = "proto3"; + +import "testdata/test.proto"; + +package test; + +message Channel { + oneof test_oneof { + int64 id = 1; + } + string name = 2; + string description = 3; +} +` + var gpfPath = filepath.Join("testdata", "getProtoFiles") func TestParseSingleQuoteReservedNames(t *testing.T) { @@ -415,6 +431,19 @@ func TestParseWithoutEntryOptionsWithRPCOptions(t *testing.T) { assert.Len(t, entry.Services[0].RPCs[0].Options, 2) } +func TestParseWithOneof(t *testing.T) { + r := strings.NewReader(protoWithOneof) + + entry, err := Parse("test:protoWithOneof", r) + assert.NoError(t, err) + + assert.Len(t, entry.Messages, 1) + assert.Len(t, entry.Messages[0].Fields, 3) + assert.Equal(t, "test_oneof", entry.Messages[0].Fields[0].OneofParent) + assert.Equal(t, "", entry.Messages[0].Fields[1].OneofParent) + assert.Equal(t, "", entry.Messages[0].Fields[2].OneofParent) +} + func TestGetProtoFilesFiltersDirectories(t *testing.T) { files, err := getProtoFiles(gpfPath, "") require.NoError(t, err) diff --git a/proto.lock b/proto.lock index 798980f..665d0d4 100644 --- a/proto.lock +++ b/proto.lock @@ -503,12 +503,14 @@ { "id": 4, "name": "name", - "type": "string" + "type": "string", + "oneof_parent": "test_oneof" }, { "id": 9, "name": "is_active", - "type": "bool" + "type": "bool", + "oneof_parent": "test_oneof" } ] }, diff --git a/rules.go b/rules.go index 0f45b23..d2ba413 100644 --- a/rules.go +++ b/rules.go @@ -41,6 +41,10 @@ var ( Name: "NoChangingRPCSignature", Func: NoChangingRPCSignature, }, + { + Name: "NoMovingExistingFieldsIntoOrOutOfOneof", + Func: NoMovingExistingFieldsIntoOrOutOfOneof, + }, } strict = true @@ -827,6 +831,62 @@ func NoChangingRPCSignature(cur, upd Protolock) ([]Warning, bool) { return nil, true } +// Existing fields must not be moved into or out of a oneof. This is a backwards-incompatible change in the Go protobuf stubs. +// per https://google.aip.dev/180#moving-into-oneofs +func NoMovingExistingFieldsIntoOrOutOfOneof(cur, upd Protolock) ([]Warning, bool) { + var warnings []Warning + + // check all message fields + curFieldMap := getFieldMap(cur) + updFieldMap := getFieldMap(upd) + + // if a field name from the current Protolock has a OneofParent entry + // that differs from the updated Protolock, then a warning should be added + for path, msgMap := range curFieldMap { + for msgName, fieldMap := range msgMap { + for fieldName, field := range fieldMap { + updField, ok := updFieldMap[path][msgName][fieldName] + if ok && updField.OneofParent != field.OneofParent { + if len(updField.OneofParent) == 0 { + msg := fmt.Sprintf( + `"%s" was moved out of oneof "%s"`, + fieldName, field.OneofParent, + ) + warnings = append(warnings, Warning{ + Filepath: OSPath(path), + Message: msg, + }) + } else if len(field.OneofParent) == 0 { + msg := fmt.Sprintf( + `"%s" was moved into oneof "%s"`, + fieldName, updField.OneofParent, + ) + warnings = append(warnings, Warning{ + Filepath: OSPath(path), + Message: msg, + }) + } else { + msg := fmt.Sprintf( + `"%s" was moved from oneof "%s" into of oneof "%s"`, + fieldName, field.OneofParent, updField.OneofParent, + ) + warnings = append(warnings, Warning{ + Filepath: OSPath(path), + Message: msg, + }) + } + } + } + } + } + + if warnings != nil { + return warnings, false + } + + return nil, true +} + func getReservedFieldsRecursive(reservedIDMap lockIDsMap, reservedNameMap lockNamesMap, filepath Protopath, prefix string, msg Message) { msgName := prefix + msg.Name for _, id := range msg.ReservedIDs { @@ -916,7 +976,7 @@ func getFieldIDNameRecursive(fieldIDNameMap lockFieldIDNameMap, filepath Protopa fieldIDNameMap[filepath][msgName][mp.Field.ID] = mp.Field.Name } for _, nestedMsg := range msg.Messages { - getFieldIDNameRecursive(fieldIDNameMap, filepath, msgName + nestedPrefix, nestedMsg) + getFieldIDNameRecursive(fieldIDNameMap, filepath, msgName+nestedPrefix, nestedMsg) } } @@ -1027,7 +1087,7 @@ func getMapMapRecursive(nameTypeMap lockMapMap, filepath Protopath, prefix strin } for _, nestedMsg := range msg.Messages { - getMapMapRecursive(nameTypeMap, filepath, msgName + nestedPrefix, nestedMsg) + getMapMapRecursive(nameTypeMap, filepath, msgName+nestedPrefix, nestedMsg) } } @@ -1063,7 +1123,7 @@ func getFieldMapRecursive(nameTypeMap lockFieldMap, filepath Protopath, prefix s nameTypeMap[filepath][msgName][mp.Field.Name] = mp.Field } for _, nestedMsg := range msg.Messages { - getFieldMapRecursive(nameTypeMap, filepath, msgName + nestedPrefix, nestedMsg) + getFieldMapRecursive(nameTypeMap, filepath, msgName+nestedPrefix, nestedMsg) } } diff --git a/rules_test.go b/rules_test.go index f5892a7..750ea42 100644 --- a/rules_test.go +++ b/rules_test.go @@ -908,6 +908,26 @@ message B { } ` +const shouldFlagFieldMovedToFromOneofProto = `syntax = "proto3"; +package test; + +message Channel { + oneof test_oneof { + int64 id = 1; + } + string name = 2; + string description = 3; +} + +message NextRequest {} +message PreviousRequest {} + +service ChannelChanger { + rpc Next(stream NextRequest) returns (Channel); + rpc Previous(PreviousRequest) returns (stream Channel); +} +` + func TestParseOnReader(t *testing.T) { r := strings.NewReader(simpleProto) _, err := Parse("simpleProto", r) @@ -1099,6 +1119,26 @@ func TestShouldConflictReusingFieldsNestedMessages(t *testing.T) { assert.Len(t, warnings, 1) } +func TestMovingFieldIntoOrOutOfOneof(t *testing.T) { + SetDebug(true) + curLock := parseTestProto(t, simpleProto) + updLock := parseTestProto(t, shouldFlagFieldMovedToFromOneofProto) + + warnings, ok := NoMovingExistingFieldsIntoOrOutOfOneof(curLock, updLock) + assert.False(t, ok) + assert.Len(t, warnings, 1) + assert.Equal(t, "\"id\" was moved into oneof \"test_oneof\"", warnings[0].Message) + + warnings, ok = NoMovingExistingFieldsIntoOrOutOfOneof(updLock, updLock) + assert.True(t, ok) + assert.Len(t, warnings, 0) + + warnings, ok = NoMovingExistingFieldsIntoOrOutOfOneof(updLock, curLock) + assert.False(t, ok) + assert.Len(t, warnings, 1) + assert.Equal(t, "\"id\" was moved out of oneof \"test_oneof\"", warnings[0].Message) +} + func parseTestProto(t *testing.T, proto string) Protolock { r := strings.NewReader(proto) entry, err := Parse("proto", r)