Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix string fields w/ trailing slashes #7679

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Fix string fields w/ trailing slashes #7679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions models/points.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,8 +1038,8 @@ func scanFieldValue(buf []byte, i int) (int, []byte) {
start := i
quoted := false
for i < len(buf) {
// Only escape char for a field value is a double-quote
if buf[i] == '\\' && i+1 < len(buf) && buf[i+1] == '"' {
// Only escape char for a field value is a double-quote and backslash
if buf[i] == '\\' && i+1 < len(buf) && (buf[i+1] == '"' || buf[i+1] == '\\') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From recollection of when I did the er-escape-backslash branch, I don't think this quite captures the desirable behaviour.

If the rule is "the only escapable characters are a double quote and a backslash" then what happens if we hit a backslash that isn't followed by a double quote or a backslash. For example:

cpu value="foo\s"

This current approach I think will skip over the single slash silently, but I feel like maybe it should be an error? Possibly we have discussed this before..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It think it should skip over the single \. If it returns an error, that's going to break existing clients.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, in that case LGTM 👍

i += 2
continue
}
Expand Down
15 changes: 15 additions & 0 deletions models/points_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1086,6 +1086,21 @@ func TestParsePointWithStringWithCommas(t *testing.T) {
},
time.Unix(1, 0)),
)

// string w/ trailing escape chars
test(t, `cpu,host=serverA,region=us-east str="foo\\",str2="bar" 1000000000`,
NewTestPoint(
"cpu",
models.NewTags(map[string]string{
"host": "serverA",
"region": "us-east",
}),
models.Fields{
"str": "foo\\", // trailing escape char
"str2": "bar",
},
time.Unix(1, 0)),
)
}

func TestParsePointQuotedMeasurement(t *testing.T) {
Expand Down