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

Some 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 the tag values
 must be contained within double quotes.

If a user accidentally passes an invalid tag with mismatched quotes,
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.

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
```

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"`),
 				)),
 			),
 		)