Add support for checking whether an existing field has moved into or …

…out of a oneof (#151)

* Add support for checking whether an existing field has moved into or out of a oneof

* Add rule to var Rules and update proto.lock

* chore: fix unrelated go nit, remove excess newline

---------

Co-authored-by: Steve Manuel <steve@dylib.so>

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)

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,
 					})
 				}
 			}

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)

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"
               }
             ]
           },

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)
 	}
 }
 

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)