cleanup, catch case where field has been removed, but is not "reserved"

nilslice · May 10, 2018 · 7c228c5 · 7c228c5
1 parent 4f6ad73
commit 7c228c5
Show file tree

Hide file tree

Showing 7 changed files with 76 additions and 20 deletions.
diff --git a/cmd/protolock/main.go b/cmd/protolock/main.go
@@ -76,7 +76,7 @@ func main() {
 }
 
 func saveToLockFile(r io.Reader) error {
-	lockfile, err := os.Create("proto.lock")
+	lockfile, err := os.Create(protolock.LockFileName)
 	if err != nil {
 		return err
 	}

diff --git a/commit.go b/commit.go
@@ -9,8 +9,8 @@ import (
 // Commit will return an io.Reader with the lock representation data for caller to
 // use as needed.
 func Commit() (io.Reader, error) {
-	if _, err := os.Stat(lockFileName); err != nil && os.IsNotExist(err) {
-		fmt.Println(`no proto.lock found, please run "init" first`)
+	if _, err := os.Stat(LockFileName); err != nil && os.IsNotExist(err) {
+		fmt.Println(`No "proto.lock" file found, first run "init"`)
 		os.Exit(1)
 	}
 	updated, err := getUpdatedLock()

diff --git a/init.go b/init.go
@@ -13,8 +13,8 @@ const protoSuffix = ".proto"
 // Init will return an io.Reader with the lock representation data for caller to
 // use as needed.
 func Init() (io.Reader, error) {
-	if _, err := os.Stat(lockFileName); err == nil && !os.IsNotExist(err) {
-		fmt.Println(`a proto.lock file was already found, use "commit" to update`)
+	if _, err := os.Stat(LockFileName); err == nil && !os.IsNotExist(err) {
+		fmt.Println(`A "proto.lock" file was already found, use "commit" to update`)
 		os.Exit(1)
 	}
 	updated, err := getUpdatedLock()

diff --git a/parse.go b/parse.go
@@ -13,7 +13,7 @@ import (
 	"github.com/emicklei/proto"
 )
 
-const lockFileName = "proto.lock"
+const LockFileName = "proto.lock"
 
 type Protolock struct {
 	Definitions []Definition `json:"definitions,omitempty"`
@@ -153,7 +153,7 @@ func withMessage(m *proto.Message) {
 
 // openLockFile opens and returns the lock file on disk for reading.
 func openLockFile() (io.ReadCloser, error) {
-	f, err := os.Open(lockFileName)
+	f, err := os.Open(LockFileName)
 	if err != nil {
 		return nil, err
 	}
@@ -209,8 +209,6 @@ func getUpdatedLock() (*Protolock, error) {
 	// files is a map of filepaths to string buffers to be joined into the
 	// proto.lock file.
 	files := make(map[string]Entry)
-	// TODO: consider:
-	// files := make(map[*proto.Package]Entry)
 
 	root, err := os.Getwd()
 	if err != nil {

diff --git a/rules.go b/rules.go
@@ -9,6 +9,7 @@ var (
 	ruleFuncs = []RuleFunc{
 		NoUsingReservedFields,
 		NoRemovingReservedFields,
+		NoDeprecatingFieldsWithoutReserve,
 		NoChangingFieldIDs,
 		NoChangingFieldTypes,
 		NoChangingFieldNames,
@@ -118,7 +119,7 @@ func NoUsingReservedFields(cur, upd Protolock) ([]Warning, bool) {
 			for id, count := range mm {
 				if count > 1 {
 					msg := fmt.Sprintf(
-						"%s is re-using ID: %d, a reserved field",
+						`"%s" is re-using ID: %d, a reserved field`,
 						msgName, id,
 					)
 					warnings = append(warnings, Warning{
@@ -137,7 +138,7 @@ func NoUsingReservedFields(cur, upd Protolock) ([]Warning, bool) {
 			for name, count := range mm {
 				if count > 1 {
 					msg := fmt.Sprintf(
-						`%s is re-using name: "%s", a reserved field`,
+						`"%s" is re-using name: "%s", a reserved field`,
 						msgName, name,
 					)
 					warnings = append(warnings, Warning{
@@ -182,7 +183,7 @@ func NoRemovingReservedFields(cur, upd Protolock) ([]Warning, bool) {
 			for id := range idMap {
 				if _, ok := updReservedIDMap[path][msgName][id]; !ok {
 					msg := fmt.Sprintf(
-						"%s is missing ID: %d, a reserved field",
+						`"%s" is missing ID: %d, a reserved field`,
 						msgName, id,
 					)
 					warnings = append(warnings, Warning{
@@ -198,7 +199,7 @@ func NoRemovingReservedFields(cur, upd Protolock) ([]Warning, bool) {
 			for name := range nameMap {
 				if _, ok := updReservedNameMap[path][msgName][name]; !ok {
 					msg := fmt.Sprintf(
-						`%s is missing name: "%s", a reserved field`,
+						`"%s" is missing name: "%s", a reserved field`,
 						msgName, name,
 					)
 					warnings = append(warnings, Warning{
@@ -241,7 +242,7 @@ func NoChangingFieldIDs(cur, upd Protolock) ([]Warning, bool) {
 				if ok {
 					if updFieldID != fieldID {
 						msg := fmt.Sprintf(
-							`%s field: "%s" has a different ID: %d, previously %d`,
+							`"%s" field: "%s" has a different ID: %d, previously %d`,
 							msgName, fieldName, updFieldID, fieldID,
 						)
 						warnings = append(warnings, Warning{
@@ -284,7 +285,7 @@ func NoChangingFieldTypes(cur, upd Protolock) ([]Warning, bool) {
 				if ok {
 					if updField.Type != field.Type {
 						msg := fmt.Sprintf(
-							`%s field: "%s" has a different type: %s, previously %s`,
+							`"%s" field: "%s" has a different type: %s, previously %s`,
 							msgName, fieldName, updField.Type, field.Type,
 						)
 						warnings = append(warnings, Warning{
@@ -295,7 +296,7 @@ func NoChangingFieldTypes(cur, upd Protolock) ([]Warning, bool) {
 
 					if updField.IsRepeated != field.IsRepeated {
 						msg := fmt.Sprintf(
-							`%s field: "%s" has a different "repeated" status: %t, previously %t`,
+							`"%s" field: "%s" has a different "repeated" status: %t, previously %t`,
 							msgName, fieldName, updField.IsRepeated, field.IsRepeated,
 						)
 						warnings = append(warnings, Warning{
@@ -344,7 +345,7 @@ func NoChangingFieldNames(cur, upd Protolock) ([]Warning, bool) {
 				if ok {
 					if updFieldName != fieldName {
 						msg := fmt.Sprintf(
-							`%s field: "%s" (ID: %d) has an updated name, previously "%s"`,
+							`"%s" field: "%s" ID: %d has an updated name, previously "%s"`,
 							msgName, updFieldName, fieldID, fieldName,
 						)
 						warnings = append(warnings, Warning{
@@ -383,8 +384,61 @@ func NoRemovingRPCs(cur, upd Protolock) ([]Warning, bool) {
 // without a corresponding reservation of that field or name.
 func NoDeprecatingFieldsWithoutReserve(cur, upd Protolock) ([]Warning, bool) {
 	if !strict {
+		return nil, true
+	}
+
+	if debug {
+		beginRuleDebug("NoDeprecatingFieldsWithoutReserve")
+	}
+
+	var warnings []Warning
+	// check that if a field name from the current Protolock is not retained
+	// in the updated Protolock, then the field's name and ID shoule become
+	// reserved within the parent message
+	curFieldMap := getFieldMap(cur)
+	updFieldMap := getFieldMap(upd)
+	for path, msgMap := range curFieldMap {
+		for msgName, fieldMap := range msgMap {
+			for fieldName, field := range fieldMap {
+				_, ok := updFieldMap[path][msgName][fieldName]
+				if !ok {
+					// check that the field name and ID are
+					// both in the reserved fields for this
+					// message
+					resIDsMap, resNamesMap := getReservedFields(upd)
+					if _, ok := resIDsMap[path][msgName][field.ID]; !ok {
+						msg := fmt.Sprintf(
+							`"%s" ID: "%d" has been removed, but is not "reserved"`,
+							msgName, field.ID,
+						)
+						warnings = append(warnings, Warning{
+							Filepath: path,
+							Message:  msg,
+						})
+					}
+					if _, ok := resNamesMap[path][msgName][field.Name]; !ok {
+						msg := fmt.Sprintf(
+							`"%s" field: "%s" has been removed, but is not "reserved"`,
+							msgName, field.Name,
+						)
+						warnings = append(warnings, Warning{
+							Filepath: path,
+							Message:  msg,
+						})
+					}
+				}
+			}
+		}
+	}
 
+	if debug {
+		concludeRuleDebug("NoDeprecatingFieldsWithoutReserve", warnings)
 	}
+
+	if warnings != nil {
+		return warnings, false
+	}
+
 	return nil, true
 }
 
@@ -420,6 +474,8 @@ func getReservedFields(lock Protolock) (lockIDsMap, lockNamesMap) {
 	return reservedIDMap, reservedNameMap
 }
 
+// getFieldsIDName gets all the fields mapped by the field ID to its name for
+// all messages.
 func getFieldsIDName(lock Protolock) lockFieldIDNameMap {
 	fieldIDNameMap := make(lockFieldIDNameMap)
 

diff --git a/rules_test.go b/rules_test.go
@@ -213,6 +213,7 @@ const changingFieldNamesProto = `syntax = "proto3";
 package test;
 
 message Channel {
+  reserved "name", "foo";
   int64 id = 1;
   string name_2 = 2;
   string description_3 = 3;
@@ -242,12 +243,13 @@ func TestChangingFieldNames(t *testing.T) {
 
 	warnings, ok := NoChangingFieldNames(curLock, updLock)
 	assert.False(t, ok)
-	assert.Len(t, warnings, 3)
+	assert.Len(t, warnings, 4)
 
 	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)

diff --git a/testdata/test.proto b/testdata/test.proto
@@ -2,12 +2,12 @@ syntax = "proto3";
 package dataset;
 
 message Channel {
-  reserved 4, 8 to 11;
+  reserved 6, 8 to 11;
   int64 id = 1;
   string name = 2;
   string description = 3;
   string foo = 4;
-  string bar = 5;
+  int32 age = 5;
 }
 
 message NextRequest {}