-
Notifications
You must be signed in to change notification settings - Fork 30
fix: support spaces in directive argument values #665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0e9f194
6e32e47
7788436
e27ab88
c59ecae
d30df29
38590a3
67796d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| // Unless explicitly stated otherwise all files in this repository are licensed | ||
| // under the Apache License Version 2.0. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| // Copyright 2023-present Datadog, Inc. | ||
|
|
||
| package code | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func Test_parseDirectiveArgs(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| prefix string | ||
| comment string | ||
| want []DirectiveArgument | ||
| wantOk bool | ||
| }{ | ||
| { | ||
| name: "valid directive with two args", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:span span.name:rootHandler resource.name:\"GET /\"", | ||
| want: []DirectiveArgument{ | ||
| {Key: "span.name", Value: "rootHandler"}, | ||
| {Key: "resource.name", Value: "GET /"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "args with spaces double quote", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:span span.name:rootHandler resource.name:\"GET /\" foo:\"bar\" ", | ||
| want: []DirectiveArgument{ | ||
| {Key: "span.name", Value: "rootHandler"}, | ||
| {Key: "resource.name", Value: "GET /"}, | ||
| {Key: "foo", Value: "bar"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "args with spaces single quote", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:span span.name:rootHandler resource.name:'GET /' foo:'bar'", | ||
| want: []DirectiveArgument{ | ||
| {Key: "span.name", Value: "rootHandler"}, | ||
| {Key: "resource.name", Value: "GET /"}, | ||
| {Key: "foo", Value: "bar"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "single and double quotes", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span span.name:'root handler' resource.name:"GET /home"`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "span.name", Value: "root handler"}, | ||
| {Key: "resource.name", Value: "GET /home"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "valid directive with one arg", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:span service.name:my-service", | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: "my-service"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "prefix matches at start but is not full word", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:spanExtra service.name:my-service", | ||
| want: nil, | ||
| wantOk: false, | ||
| }, | ||
| { | ||
| name: "non-matching prefix", | ||
| prefix: "//dd:span", | ||
| comment: "//other:span span.name:foo", | ||
| want: nil, | ||
| wantOk: false, | ||
| }, | ||
| { | ||
| name: "only prefix with no arguments", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:span", | ||
| want: nil, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "arg with only key and no colon", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:span standalone_arg", | ||
| want: []DirectiveArgument{ | ||
| {Key: "standalone_arg", Value: ""}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "arg with multiple colons", | ||
| prefix: "//dd:span", | ||
| comment: "//dd:span foo:bar:baz", | ||
| want: []DirectiveArgument{ | ||
| {Key: "foo", Value: "bar:baz"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "unclosed double quote value", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `"my-service`}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "unclosed single quote value", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:'my-service`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: "'my-service"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "missing space between quoted args", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service"resource.name:"GET /"`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `my-service"resource.name:"GET /`}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "quote starts but ends with different quote type", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service'`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `"my-service'`}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
|
Comment on lines
+112
to
+147
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find those surprising -- I reckon it's almost guaranteed that the outcome is not what the user intended...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which one concretely? what do you think it should be the preferred behavior?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect a lot of the "improperly terminated" cases to result in an error or a warning being emitted.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rarguelloF Ensure that the value is a valid Go string literal using double quote, fail on anything else (unless it's an unquoted string without spaces). |
||
| } | ||
| for _, tt := range testCases { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got, ok := parseDirectiveArgs(tt.prefix, tt.comment) | ||
| assert.Equal(t, tt.wantOk, ok) | ||
| assert.Equal(t, tt.want, got) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that demonstrates what happens in cases where the quoted string is broken (i.e, never closed / no space after close & before next / ...)? Those seem to be missing and they're the edge cases I worry about :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! added a few more test cases, please let me know if the behavior makes sense, or if there's any other test missing 🙏