rename strictMode -> strict, implement + test NoChangingFieldNames

nilslice · May 10, 2018 · 4f6ad73 · 4f6ad73
1 parent 181417d
commit 4f6ad73
Show file tree

Hide file tree

Showing 3 changed files with 141 additions and 16 deletions.
diff --git a/cmd/protolock/main.go b/cmd/protolock/main.go
@@ -10,17 +10,17 @@ import (
 )
 
 var (
-	debug      = flag.Bool("debug", false, "toggle debug mode for verbose output")
-	strictMode = flag.Bool("strict", true, "toggle strict mode, to determine which rules are enforced")
+	debug  = flag.Bool("debug", false, "toggle debug mode for verbose output")
+	strict = flag.Bool("strict", true, "toggle strict mode, to determine which rules are enforced")
 )
 
 func main() {
 	flag.Parse()
 
 	// XXX: currently here as placeholder until better CLI implementation
-	// is completed. This includes debug and strictMode vars in block above.
+	// is completed. This includes debug and strict vars in block above.
 	protolock.SetDebug(*debug)
-	protolock.SetStrictMode(*strictMode)
+	protolock.SetStrict(*strict)
 
 	if len(os.Args) < 2 {
 		os.Exit(0)

diff --git a/rules.go b/rules.go
@@ -15,13 +15,13 @@ var (
 		NoRemovingRPCs,
 	}
 
-	strictMode = true
-	debug      = false
+	strict = true
+	debug  = false
 )
 
-// SetStrictMode enables the user to toggle strict mode on and off.
-func SetStrictMode(mode bool) {
-	strictMode = mode
+// SetStrict enables the user to toggle strict mode on and off.
+func SetStrict(mode bool) {
+	strict = mode
 }
 
 // SetDebug enables the user to toggle debug mode on and off.
@@ -68,10 +68,14 @@ type lockIDsMap map[string]map[string]map[int]int
 */
 type lockNamesMap map[string]map[string]map[string]int
 
-// lockTypesMap:
+// lockFieldMap:
 // table of filepath -> message name -> field name -> field type
 type lockFieldMap map[string]map[string]map[string]Field
 
+// lockFieldIDNameMap:
+// table of filepath -> message name -> field ID -> field name
+type lockFieldIDNameMap map[string]map[string]map[int]string
+
 // NoUsingReservedFields compares the current vs. updated Protolock definitions
 // and will return a list of warnings if any message's previously reserved fields
 // are now being used as part of the same message.
@@ -160,7 +164,7 @@ func NoUsingReservedFields(cur, upd Protolock) ([]Warning, bool) {
 // and will return a list of warnings if any reserved field has been removed. This
 // rule is only enforced when strict mode is enabled.
 func NoRemovingReservedFields(cur, upd Protolock) ([]Warning, bool) {
-	if !strictMode {
+	if !strict {
 		return nil, true
 	}
 
@@ -319,23 +323,71 @@ func NoChangingFieldTypes(cur, upd Protolock) ([]Warning, bool) {
 // will return a list of warnings if any message's previous fields have been
 // renamed. This rule is only enforced when strict mode is enabled.
 func NoChangingFieldNames(cur, upd Protolock) ([]Warning, bool) {
-	if !strictMode {
+	if debug {
+		beginRuleDebug("NoChangingFieldNames")
+	}
+
+	if !strict {
 		return nil, true
 	}
 
+	curFieldMap := getFieldsIDName(cur)
+	updFieldMap := getFieldsIDName(upd)
+
+	var warnings []Warning
+	// check that the current Protolock messages' field names are equal to
+	// their relative messages' field names in the updated Protolock
+	for path, msgMap := range curFieldMap {
+		for msgName, fieldMap := range msgMap {
+			for fieldID, fieldName := range fieldMap {
+				updFieldName, ok := updFieldMap[path][msgName][fieldID]
+				if ok {
+					if updFieldName != fieldName {
+						msg := fmt.Sprintf(
+							`%s field: "%s" (ID: %d) has an updated name, previously "%s"`,
+							msgName, updFieldName, fieldID, fieldName,
+						)
+						warnings = append(warnings, Warning{
+							Filepath: path,
+							Message:  msg,
+						})
+					}
+				}
+			}
+		}
+	}
+
+	if debug {
+		concludeRuleDebug("NoChangingFieldNames", warnings)
+	}
+
+	if warnings != nil {
+		return warnings, false
+	}
+
 	return nil, true
 }
 
 // NoRemovingRPCs compares the current vs. updated Protolock definitions and
 // will return a list of warnings if any RPCs provided by a Service have been
 // removed. This rule is only enforced when strict mode is enabled.
 func NoRemovingRPCs(cur, upd Protolock) ([]Warning, bool) {
-	if !strictMode {
+	if !strict {
 		return nil, true
 	}
 	return nil, true
 }
 
+// NoDeprecatingFieldsWithoutReserve compares the current vs. updated Protolock
+// definitions and will return a list of warnings if any field has been removed
+// without a corresponding reservation of that field or name.
+func NoDeprecatingFieldsWithoutReserve(cur, upd Protolock) ([]Warning, bool) {
+	if !strict {
+
+	}
+	return nil, true
+}
+
 // getReservedFields gets all the reserved field numbers and names, and stashes
 // them in a lockIDsMap and lockNamesMap to be checked against.
 func getReservedFields(lock Protolock) (lockIDsMap, lockNamesMap) {
@@ -368,6 +420,26 @@ func getReservedFields(lock Protolock) (lockIDsMap, lockNamesMap) {
 	return reservedIDMap, reservedNameMap
 }
 
+func getFieldsIDName(lock Protolock) lockFieldIDNameMap {
+	fieldIDNameMap := make(lockFieldIDNameMap)
+
+	for _, def := range lock.Definitions {
+		if fieldIDNameMap[def.Filepath] == nil {
+			fieldIDNameMap[def.Filepath] = make(map[string]map[int]string)
+		}
+		for _, msg := range def.Def.Messages {
+			for _, field := range msg.Fields {
+				if fieldIDNameMap[def.Filepath][msg.Name] == nil {
+					fieldIDNameMap[def.Filepath][msg.Name] = make(map[int]string)
+				}
+				fieldIDNameMap[def.Filepath][msg.Name][field.ID] = field.Name
+			}
+		}
+	}
+
+	return fieldIDNameMap
+}
+
 // getNonReservedFields gets all the reserved field numbers and names, and stashes
 // them in a lockNamesMap to be checked against.
 func getNonReservedFields(lock Protolock) lockNamesMap {
@@ -413,15 +485,15 @@ func getFieldMap(lock Protolock) lockFieldMap {
 }
 
 func beginRuleDebug(name string) {
-	fmt.Println("run rule:", name)
+	fmt.Println("RUN RULE:", name)
 }
 
 func concludeRuleDebug(name string, warnings []Warning) {
-	fmt.Println("warnings:", len(warnings))
+	fmt.Println("# Warnings:", len(warnings))
 	for i, w := range warnings {
 		msg := fmt.Sprintf("%d). %s [%s]", i+1, w.Message, w.Filepath)
 		fmt.Println(msg)
 	}
-	fmt.Println("end:", name)
+	fmt.Println("END RULE:", name)
 	fmt.Println("===")
 }
diff --git a/rules_test.go b/rules_test.go
@@ -189,12 +189,65 @@ service ChannelChanger {
 }
 `
 
+const noChangingFieldNamesProto = `syntax = "proto3";
+package test;
+
+message Channel {
+  int64 id = 1;
+  string name = 2;
+  string description = 3;
+  string foo = 4;
+  bool bar = 5;
+}
+
+message NextRequest {}
+message PreviousRequest {}
+
+service ChannelChanger {
+  rpc Next(stream NextRequest) returns (Channel);
+  rpc Previous(PreviousRequest) returns (stream Channel);
+}
+`
+
+const changingFieldNamesProto = `syntax = "proto3";
+package test;
+
+message Channel {
+  int64 id = 1;
+  string name_2 = 2;
+  string description_3 = 3;
+  string foo_baz = 4;
+  bool bar = 5;
+}
+
+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(r)
 	assert.NoError(t, err)
 }
 
+func TestChangingFieldNames(t *testing.T) {
+	SetDebug(true)
+	curLock := parseTestProto(t, noChangingFieldNamesProto)
+	updLock := parseTestProto(t, changingFieldNamesProto)
+
+	warnings, ok := NoChangingFieldNames(curLock, updLock)
+	assert.False(t, ok)
+	assert.Len(t, warnings, 3)
+
+	warnings, ok = NoChangingFieldNames(updLock, updLock)
+	assert.True(t, ok)
+	assert.Len(t, warnings, 0)
+}
 func TestUsingReservedFields(t *testing.T) {
 	SetDebug(true)
 	curLock := parseTestProto(t, noUsingReservedFieldsProto)