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

lverma14 · 2023-02-28T02:38:49Z

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

abhinav · 2023-02-28T04:05:50Z

annotated.go

+	for _, tag := range pt.tags {
+		if strings.Contains(tag, "'") {
+			correctedTag := strings.ReplaceAll(tag, "'", "\"")
+			return errors.New("the ParamTags tag has invalid quotations, did you mean " + correctedTag + " ?")
+		}
+	}


Hi, just doing a quick drive-by.
This seems a bit too specific. What if my tag value has ' in it?

type foo struct { Value string `name:"a'bc"`

Fx does not have any rules rejecting that, AFAIK.

Y'all might be able to borrow struct tag validation from the structtag lint check in go vet.
https://cs.opensource.google/go/go/+/master:src/cmd/vendor/golang.org/x/tools/go/analysis/passes/structtag/structtag.go;drc=31881f87873b84709a49ca17195bbe5b3f683acf;l=212

(We don't need the xml/json specialization there.)

Thanks @abhinav, that's really helpful! I'll look into that

JacobOaks

I saw this is still a draft but some initial thoughts :)

JacobOaks · 2023-02-28T17:13:31Z

annotated.go

+	for _, tag := range rt.tags {
+		if strings.Contains(tag, "'") {
+			correctedTag := strings.ReplaceAll(tag, "'", "\"")
+			return errors.New("the ResultTags tag has invalid quotations, did you mean " + correctedTag + " ?")


Do we want to surround correctedTag with quotes or backticks to help demarcate it in the error message?

Oh yeah, good catch thanks!

JacobOaks · 2023-02-28T17:14:49Z

annotated_test.go

+		require.Error(t, err)
+		assert.Contains(t, err.Error(), `the ParamTags tag has invalid quotations, did you mean name:"something" ?`)


Thoughts on using ErrorContains instead? It looks like it can combine all 3 of these lines in one.

JacobOaks · 2023-02-28T17:19:35Z

annotated_test.go

@@ -1347,6 +1347,78 @@ func TestAnnotate(t *testing.T) {
 		assert.Contains(t, err.Error(), "encountered error while applying annotation using fx.Annotate to go.uber.org/fx_test.TestAnnotate.func1(): cannot apply more than one line of ResultTags")
 	})

+	t.Run("invalid ParamTag quotations", func(t *testing.T) {


+1 to @abhinav 's comment about additional error cases that these tests don't cover.

I also think we can convert these tests to table tests since much of the logic is the same between tests (https://github.com/uber-go/guide/blob/master/style.md#test-tables)

i.e. the test case struct could look something like

type test struct { name string giveAnnotation fx.Annotation // set this to result of either fx.ParamTags or fx.ResultTags wantErr string }

codecov · 2023-02-28T19:34:33Z

Codecov Report

Merging #1051 (9e136fd) into master (6285a02) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
+ Coverage   98.50%   98.52%   +0.02%     
==========================================
  Files          39       39              
  Lines        2937     2989      +52     
==========================================
+ Hits         2893     2945      +52     
  Misses         37       37              
  Partials        7        7

Impacted Files	Coverage Δ
annotated.go	`99.23% <100.00%> (+0.04%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

JacobOaks

Submitting on behalf of @r-hang for group code review.

Let's try to refactor this:

Let's try to separate separate failure cases into different functions to make the code easier to understand
Let's try without the "did you mean?" suggestions for now, for this case it's probably not worth the added complexity

JacobOaks · 2023-03-03T21:10:04Z

annotated_test.go

+	}{
+		{
+			give:           "ParamTags wrong ending quote",
+			wantErr:        errTagValueSyntaxEndingQuote + "`name:\"something\"` ?",


Considering specifying in the test message whether it is the key or the value that has the incorrect quote

JacobOaks · 2023-03-03T21:14:18Z

annotated_test.go

+			wantErr:        errTagValueSpace,
+			giveAnnotation: fx.ParamTags(`name: "something"`),
+		},
+		{


The set of tests for ResultTags looks similar to for ParamTags, can we de-duplicate these tests?

JacobOaks · 2023-03-03T21:15:31Z

annotated_test.go

+			err := app.Err()
+			assert.ErrorContains(t, err, tt.wantErr)


We can combine these
assert.ErrorContains(t, app.Err(), tt.wantErr)

JacobOaks · 2023-03-03T21:16:42Z

annotated_test.go

+		})
+	}
+
+	tests1 := []struct {


Let's put two separate sub-tests, one for your valid cases and one for your invalid cases

JacobOaks · 2023-03-03T21:17:13Z

annotated_test.go

+		{
+			give:           "ParamTags Tag Empty with extra spaces",
+			giveAnnotation: fx.ParamTags(`name:"versionNum"`, `  `),
+		},


repeated test cases

JacobOaks · 2023-03-03T21:17:33Z

annotated_test.go

+			err := app.Err()
+			require.NoError(t, err)


Same comment as above, can combine these

r-hang · 2023-03-08T07:44:28Z

annotated.go

+// Given a tag with key, value pair, they must be separaed by a space.
+// Eg: `group:"some" optional:"true"`
+// return an error if in the case of multiple tags, they aren't separated
+// space. Return nil on success.


what do you think about this shortened form?

Suggested change

// Given a tag with key, value pair, they must be separaed by a space.

// Eg: `group:"some" optional:"true"`

// return an error if in the case of multiple tags, they aren't separated

// space. Return nil on success.

// Collections of key value pairs within a tag should be separated by a space.

// Eg: `group:"some" optional:"true"`

I think we can drop the comments on the function returns since they can be picked up quickly from the code.

r-hang · 2023-03-08T07:45:41Z

annotated.go

+// return an error if in the case of multiple tags, they aren't separated
+// space. Return nil on success.
+func verifyTagsSpaceSeparated(numTags int, tag string) error {
+	if numTags > 0 && tag != "" && tag[0] != ' ' {


numTags makes me think it's the number of tags in the annotation and that check should be > 1. I think something like tagIdx might make the purposes clearer?

r-hang · 2023-03-08T07:48:42Z

annotated.go

+// Given a value from a tag verify that it is contained between double quotes
+// Return an error if the invalid quote format is used for tag value.
+// On sucess return the remaining tag with nil error


What are your thoughts on a shorter comment like "verify tag values are delimited with double quotes"?

I think we can drop documentation on the error return value since it's clear that should happen if the validation fails.

r-hang · 2023-03-08T07:59:45Z

annotated.go

+func verifyAnnotateTag(tag string) error {
+	numTags := 0
+	for ; tag != ""; numTags++ {
+		validKeys := map[string]bool{"group": true, "optional": true, "name": true}


nit: we commonly use map[string]struct{} to map[string]bool as an empty struct consumes zero bytes of storage.

Nice! That'd be really handy

r-hang · 2023-03-08T08:00:37Z

annotated.go

+			return err
+		}
+		i := 0
+		// return if tag is empty


I think this comment can be removed.

r-hang · 2023-03-08T08:05:18Z

annotated.go

+			i++
+		}
+		key := strings.TrimSpace(tag[:i])
+		// if key is not equal to group name or optional


I think the comment can be removed given the good naming of the map lookup

r-hang · 2023-03-08T08:07:54Z

annotated.go

+// Return an error if the invalid quote format is used for tag value.
+// On sucess return the remaining tag with nil error
+func verifyValueQuote(value string) (string, error) {
+	// starting quote should be a double quote


Could we just check if the value string is prefixed and suffixed with double quotes. Could the step to validate tag values is within quotes be replaced by counting the total number of double quotes and ensuring that is exactly 2?

The total number of double quotes can be more than 2, as it's valid for a struct string to have quotes in it.
key:"val/"ue" would be a valid tag with more than 2 double quotes. Hence, I don't think it'd be possible unless I misunderstood?

Ah understood that this approach does not work because value actually contains the rest of the tag strings.

r-hang · 2023-03-09T20:21:39Z

annotated.go

+func verifyAnnotateTag(tag string) error {
+	tagIdx := 0
+	for ; tag != ""; tagIdx++ {
+		validKeys := map[string]struct{}{"group": {}, "optional": {}, "name": {}}


Could we move this map to a top-level variable since those values seems pretty static and we can prevent re-initializing on every loop?

Thanks! Great catch!

annotated_test.go

r-hang · 2023-03-09T20:29:27Z

annotated.go

@@ -144,6 +144,75 @@ type paramTagsAnnotation struct {
 }

 var _ Annotation = paramTagsAnnotation{}
+var (
+	errTagSyntaxSpace            = errors.New(` multiple tags are not separated by space`)


Are the spaces needed here and in the next line?

r-hang · 2023-03-09T20:33:37Z

annotated_test.go

+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.give, func(t *testing.T) {


Since we're already running additional subtests what are your thoughts on prefixing the description string with the param or result case.

r-hang

lgtm modulo some minor fixups!

r-hang · 2023-03-09T21:11:36Z

annotated_test.go

+	}
+	for _, tt := range tests {
+		t.Run(tt.give, func(t *testing.T) {
+			app := NewForTest(t,


I see you're also using t.Parallel() at the top-level. You'll also need to call t.Parallel() within these subtests if you want them to run in parallel.

r-hang · 2023-03-09T21:12:28Z

annotated.go

+// Given a string tag the function checks if 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


Suggested change

// Currently dig accepts only 'name', 'group', 'optional' as valid tag keys

// Currently dig accepts only 'name', 'group', 'optional' as valid tag keys.

r-hang · 2023-03-09T21:12:52Z

annotated.go

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


Suggested change

// (`tag_name:"tag_value') , this will return an error

// (`tag_name:"tag_value') , this will return an error.

r-hang · 2023-03-09T21:15:19Z

annotated.go

+// If the tag is invalid and has mismatched quotation for example,
+// (`tag_name:"tag_value') , this will return an error


Suggested change

// If the tag is invalid and has mismatched quotation for example,

// (`tag_name:"tag_value') , 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.

r-hang · 2023-03-09T21:16:32Z

annotated.go

+	return nil
+}
+
+// verify tag values are delimited with double quotes


Suggested change

// verify tag values are delimited with double quotes

// verify tag values are delimited with double quotes.

annotated.go

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 : uber-go#835

sywhang

👍

abhinav reviewed Feb 28, 2023

View reviewed changes

lverma14 marked this pull request as draft February 28, 2023 04:07

JacobOaks reviewed Feb 28, 2023

View reviewed changes

lverma14 force-pushed the go-1213_Verify_invalid_tags_fx_annotate branch from 9dea8c4 to 14a3cc6 Compare February 28, 2023 21:31

lverma14 marked this pull request as ready for review March 3, 2023 00:32

lverma14 marked this pull request as draft March 3, 2023 00:37

lverma14 force-pushed the go-1213_Verify_invalid_tags_fx_annotate branch 3 times, most recently from c647465 to 1c3d56b Compare March 3, 2023 21:00

JacobOaks reviewed Mar 3, 2023

View reviewed changes

lverma14 force-pushed the go-1213_Verify_invalid_tags_fx_annotate branch 4 times, most recently from 7bc8882 to 0ad3a6c Compare March 4, 2023 02:33

lverma14 requested a review from JacobOaks March 4, 2023 02:35

lverma14 marked this pull request as ready for review March 4, 2023 02:36

r-hang reviewed Mar 8, 2023

View reviewed changes

lverma14 force-pushed the go-1213_Verify_invalid_tags_fx_annotate branch from f541237 to 49cd76d Compare March 9, 2023 03:17

r-hang reviewed Mar 9, 2023

View reviewed changes

lverma14 force-pushed the go-1213_Verify_invalid_tags_fx_annotate branch from 49cd76d to 592d0b1 Compare March 9, 2023 21:03

r-hang approved these changes Mar 9, 2023

View reviewed changes

lverma14 force-pushed the go-1213_Verify_invalid_tags_fx_annotate branch 2 times, most recently from b248a81 to 172f322 Compare March 9, 2023 23:13

lverma14 force-pushed the go-1213_Verify_invalid_tags_fx_annotate branch from 172f322 to 9e136fd Compare March 9, 2023 23:19

sywhang approved these changes Mar 10, 2023

View reviewed changes

lverma14 merged commit 7e9108b into uber-go:master Mar 10, 2023

		require.Error(t, err)
		assert.Contains(t, err.Error(), `the ParamTags tag has invalid quotations, did you mean name:"something" ?`)

	// Currently dig accepts only 'name', 'group', 'optional' as valid tag keys
	// Currently dig accepts only 'name', 'group', 'optional' as valid tag keys.

	// (`tag_name:"tag_value') , this will return an error
	// (`tag_name:"tag_value') , 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

	// verify tag values are delimited with double quotes
	// verify tag values are delimited with double quotes.

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

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

Conversation

lverma14 commented Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobOaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobOaks Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2023 • edited Loading

Codecov Report

JacobOaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-hang Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-hang Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-hang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sywhang left a comment

Choose a reason for hiding this comment

lverma14 commented Feb 28, 2023 •

edited

Loading

JacobOaks Feb 28, 2023 •

edited

Loading

codecov bot commented Feb 28, 2023 •

edited

Loading

r-hang Mar 8, 2023 •

edited

Loading

r-hang Mar 9, 2023 •

edited

Loading