[fx-annotate] Verify invalid tags passed to fx.Annotate #835 (#1051)

Tags passed with fx.Annotate follow the struct format
```
fx.Annotate(f, fx.ParamTags(`group:”myGroup”`))
```
Where the tag is of the format `<tag_name>:”tag_value”` and tags are
separated by space.

If a user accidentally passes an invalid tag such as mismatched
quotes(#835), the issue is not identified and no error is thrown. This
is not desired as the tag gets passed to dig, where it’s ignored as it’s
not a valid group tag.

This commit adds checks to identify if invalid tags are passed to
fx.Annotate. If identified then it throws an error with the right tag
format specified.
The following checks are now enforced -
- tag key can only be group, optional or name ( dig only accepts these
tags as valid keys currently)
- In cases of multiple tags they must be separated by space . Eg
``group:"some" optional:"true"``vim
 - the tag values must be contained within double quotes.

For e.g. -
```
fx.Annotate(f, fx.ParamTags(`group:'myGroup"`))
```
Should be an invalid tag as it starts from a single quote and ends with
a double quote (mismatched quotes instead of “...”) We added checks for
ParamTag and ResultTag to verify and capture this invalid tag.
Now, when we pass this invalid tag we get the following error :
```
 Encountered error while applying annotation using fx.Annotate to function, 
tag value should start with double quote i.e. key:"value"
```
Components Modified
annotated.go  - add checks to verify invalid tag usage
annotated_test.go - Add test cases to verify functionality of changes
module_test.go - FIx a typo in testcase
Refs : #835

annotated.go

@@ -144,6 +144,75 @@ type paramTagsAnnotation struct {
 }
 
 var _ Annotation = paramTagsAnnotation{}
+var (
+	errTagSyntaxSpace            = errors.New(`multiple tags are not separated by space`)
+	errTagKeySyntax              = errors.New("tag key is invalid, Use group, name or optional as tag keys")
+	errTagValueSyntaxQuote       = errors.New(`tag value should start with double quote. i.e. key:"value" `)
+	errTagValueSyntaxEndingQuote = errors.New(`tag value should end in double quote. i.e. key:"value" `)
+)
+
+// Collections of key value pairs within a tag should be separated by a space.
+// Eg: `group:"some" optional:"true"`.
+func verifyTagsSpaceSeparated(tagIdx int, tag string) error {
+	if tagIdx > 0 && tag != "" && tag[0] != ' ' {
+		return errTagSyntaxSpace
+	}
+	return nil
+}
+
+// verify tag values are delimited with double quotes.
+func verifyValueQuote(value string) (string, error) {
+	// starting quote should be a double quote
+	if value[0] != '"' {
+		return "", errTagValueSyntaxQuote
+	}
+	// validate tag value is within quotes
+	i := 1
+	for i < len(value) && value[i] != '"' {
+		if value[i] == '\\' {
+			i++
+		}
+		i++
+	}
+	if i >= len(value) {
+		return "", errTagValueSyntaxEndingQuote
+	}
+	return value[i+1:], nil
+
+}
+
+// Check whether the tag follows valid struct.
+// format and returns an error if it's invalid. (i.e. not following
+// tag:"value" space-separated list )
+// Currently dig accepts only 'name', 'group', 'optional' as valid tag keys.
+func verifyAnnotateTag(tag string) error {
+	tagIdx := 0
+	validKeys := map[string]struct{}{"group": {}, "optional": {}, "name": {}}
+	for ; tag != ""; tagIdx++ {
+		if err := verifyTagsSpaceSeparated(tagIdx, tag); err != nil {
+			return err
+		}
+		i := 0
+		if strings.TrimSpace(tag) == "" {
+			return nil
+		}
+		// parsing the key i.e. till reaching colon :
+		for i < len(tag) && tag[i] != ':' {
+			i++
+		}
+		key := strings.TrimSpace(tag[:i])
+		if _, ok := validKeys[key]; !ok {
+			return errTagKeySyntax
+		}
+		value, err := verifyValueQuote(tag[i+1:])
+		if err != nil {
+			return err
+		}
+		tag = value
+	}
+	return nil
+
+}
 
 // Given func(T1, T2, T3, ..., TN), this generates a type roughly
 // equivalent to,
@@ -159,11 +228,19 @@ var _ Annotation = paramTagsAnnotation{}
 //
 // If there has already been a ParamTag that was applied, this
 // will return an error.
+//
+// If the tag is invalid and has mismatched quotation for example,
+// (`tag_name:"tag_value') , this will return an error.
 
 func (pt paramTagsAnnotation) apply(ann *annotated) error {
 	if len(ann.ParamTags) > 0 {
 		return errors.New("cannot apply more than one line of ParamTags")
 	}
+	for _, tag := range pt.tags {
+		if err := verifyAnnotateTag(tag); err != nil {
+			return err
+		}
+	}
 	ann.ParamTags = pt.tags
 	return nil
 }
@@ -285,10 +362,18 @@ var _ Annotation = resultTagsAnnotation{}
 //
 // If there has already been a ResultTag that was applied, this
 // will return an error.
+//
+// If the tag is invalid and has mismatched quotation for example,
+// (`tag_name:"tag_value') , this will return an error.
 func (rt resultTagsAnnotation) apply(ann *annotated) error {
 	if len(ann.ResultTags) > 0 {
 		return errors.New("cannot apply more than one line of ResultTags")
 	}
+	for _, tag := range rt.tags {
+		if err := verifyAnnotateTag(tag); err != nil {
+			return err
+		}
+	}
 	ann.ResultTags = rt.tags
 	return nil
 }

annotated_test.go

@@ -1474,6 +1474,131 @@ func TestAnnotate(t *testing.T) {
 	})
 }
 
+func TestAnnotateApplyFail(t *testing.T) {
+	type a struct{}
+	type b struct{ a *a }
+	newA := func() *a { return &a{} }
+	newB := func(a *a) *b {
+		return &b{a}
+	}
+
+	var (
+		errTagSyntaxSpace            = `multiple tags are not separated by space`
+		errTagKeySyntax              = "tag key is invalid, Use group, name or optional as tag keys"
+		errTagValueSyntaxQuote       = `tag value should start with double quote. i.e. key:"value" `
+		errTagValueSyntaxEndingQuote = `tag value should end in double quote. i.e. key:"value" `
+	)
+	tests := []struct {
+		give                 string
+		wantErr              string
+		giveAnnotationParam  fx.Annotation
+		giveAnnotationResult fx.Annotation
+	}{
+		{
+			give:                 "Tags value invalid ending quote",
+			wantErr:              errTagValueSyntaxEndingQuote,
+			giveAnnotationParam:  fx.ParamTags(`name:"something'`),
+			giveAnnotationResult: fx.ResultTags(`name:"something'`),
+		},
+		{
+			give:                 "Tags value wrong starting quote",
+			wantErr:              errTagValueSyntaxQuote,
+			giveAnnotationParam:  fx.ParamTags(`name:"something" optional:'true"`),
+			giveAnnotationResult: fx.ResultTags(`name:"something" optional:'true"`),
+		},
+		{
+			give:                 "Tags multiple tags not separated by space",
+			wantErr:              errTagSyntaxSpace,
+			giveAnnotationParam:  fx.ParamTags(`name:"something"group:"something"`),
+			giveAnnotationResult: fx.ResultTags(`name:"something"group:"something"`),
+		},
+		{
+			give:                 "Tags key not equal to group, name or optional",
+			wantErr:              errTagKeySyntax,
+			giveAnnotationParam:  fx.ParamTags(`name1:"something"`),
+			giveAnnotationResult: fx.ResultTags(`name1:"something"`),
+		},
+		{
+			give:                 "Tags key empty",
+			wantErr:              errTagKeySyntax,
+			giveAnnotationParam:  fx.ParamTags(`:"something"`),
+			giveAnnotationResult: fx.ResultTags(`:"something"`),
+		},
+	}
+	for _, tt := range tests {
+		t.Run("Param "+tt.give, func(t *testing.T) {
+			app := NewForTest(t,
+				fx.Provide(
+					fx.Annotate(
+						newA,
+						tt.giveAnnotationParam,
+					),
+				),
+				fx.Invoke(newB),
+			)
+			assert.ErrorContains(t, app.Err(), tt.wantErr)
+		})
+		t.Run("Result "+tt.give, func(t *testing.T) {
+			app := NewForTest(t,
+				fx.Provide(
+					fx.Annotate(
+						newA,
+						tt.giveAnnotationResult,
+					),
+				),
+				fx.Invoke(newB),
+			)
+			assert.ErrorContains(t, app.Err(), tt.wantErr)
+		})
+	}
+}
+
+func TestAnnotateApplySuccess(t *testing.T) {
+	type a struct{}
+	type b struct{ a *a }
+	newA := func() *a { return &a{} }
+	newB := func(a *a) *b {
+		return &b{a}
+	}
+
+	tests := []struct {
+		give                 string
+		giveAnnotationParam  fx.Annotation
+		giveAnnotationResult fx.Annotation
+	}{
+		{
+			give:                 "ParamTags Tag Empty",
+			giveAnnotationParam:  fx.ParamTags(`  `),
+			giveAnnotationResult: fx.ResultTags(`  `),
+		},
+		{
+			give:                 "ParamTags Tag Empty with extra spaces",
+			giveAnnotationParam:  fx.ParamTags(`name:"versionNum"`, `  `),
+			giveAnnotationResult: fx.ResultTags(`   `, `group:"versionNum"`),
+		},
+		{
+			give:                 "ParamTags Tag with \\ ",
+			giveAnnotationParam:  fx.ParamTags(`name:"version\\Num"`, `  `),
+			giveAnnotationResult: fx.ResultTags(``, `group:"version\\Num"`),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.give, func(t *testing.T) {
+			app := NewForTest(t,
+				fx.Provide(
+					fx.Annotate(
+						newA,
+						tt.giveAnnotationParam,
+						tt.giveAnnotationResult,
+					),
+				),
+				fx.Invoke(newB),
+			)
+			require.NoError(t, app.Err())
+		})
+	}
+
+}
 func assertApp(
 	t *testing.T,
 	app interface {

module_test.go

@@ -518,8 +518,8 @@ func TestModuleFailures(t *testing.T) {
 		app := NewForTest(t,
 			fx.Module("module",
 				fx.Provide(fx.Annotate(newA,
-					fx.ParamTags(`"name:"A1"`),
-					fx.ParamTags(`"name:"A2"`),
+					fx.ParamTags(`name:"A1"`),
+					fx.ParamTags(`name:"A2"`),
 				)),
 			),
 		)