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.

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

parser: return errors if there are syntax bugs in metric names wrapped by a function #330

Merged
merged 3 commits into from
Jun 9, 2021

Conversation

bom-d-van
Copy link
Collaborator

What issue is this change attempting to solve?

In the current implementation, carbonapi returns 400 errors for queries like ns1.ns2.foo-[a b] or ns1.ns2.foo-{a, b}.
However, if these types of queries happened to be wrapped around within a function, like sumSeries, that accepts
metrics list, no errors are returned. As they are treated as individual metrics and sent to storage, storage would
returns 400 as well.

How does this change solve the problem? Why is this the best approach?

This commit fixes the issue, by generating a parser error if spaces and comma appears inappropriately within []/{}.
At the same time, nested range list (like [[]]) and unclosed ([/{) or unopened (]/}) ranges or value list are also
reported as errors, instead of treating them as proper query and forwarding them to storages.

As consequence of this change, some of the requests that are originally returning 200 would be returned with 400
status codes.

How can we be sure this works as expected?

Original tests still pass and new test cases are added.

@bom-d-van bom-d-van force-pushed the parser/various-space-comma-bug-fixes branch 3 times, most recently from eb6a810 to 540ea62 Compare June 7, 2021 15:33
…d by a function

In the current implementation, carbonapi returns 400 errors for queries like ns1.ns2.foo-[a b] or ns1.ns2.foo-{a, b}.
However, if these types of queries happened to be wrapped around within a function, like sumSeries, that accepts
metrics list, no errors are returned. As they are treated as individual metrics and sent to storage, storage would
returns 400 as well.

This commit fixes the issue, by generating a parser error if spaces and comma appears inappropriately within []/{}.
At the same time, nested range list (like [[]]) and unclosed ([/{) or unopened (]/}) ranges or value list are also
reported as errors, instead of treating them as proper query and forwarding them to storages.

As consequence of this change, some of the requests that are originally returning 200 would be returned with 400
status codes.
@bom-d-van bom-d-van force-pushed the parser/various-space-comma-bug-fixes branch from 540ea62 to bd2bfe0 Compare June 7, 2021 16:09
* Ignoring makezero due to panics in the package
* Ignoring errorlint temporarilly
* Update golangci-lint to version v1.40.1
@bom-d-van bom-d-van force-pushed the parser/various-space-comma-bug-fixes branch from bd2bfe0 to b3e5511 Compare June 7, 2021 16:19
@grzkv
Copy link
Member

grzkv commented Jun 8, 2021

@bom-d-van Can we please keep different changes in separate PRs? Linter change makes sense to do separately, and then back-merge from master.

@grzkv
Copy link
Member

grzkv commented Jun 8, 2021

@bom-d-van I believe, we need to manually test a change in syntax like this one before merging to master, because it is potentially breaking. We try to keep the promise of master always being in a working condition.

@bom-d-van
Copy link
Collaborator Author

Can we please keep different changes in separate PRs? Linter change makes sense to do separately, and then back-merge from master.

I would appreciate we just do it in one MR. The commit is already different.

@bom-d-van
Copy link
Collaborator Author

I believe, we need to manually test a change in syntax like this one before merging to master, because it is potentially breaking. We try to keep the promise of master always being in a working condition.

I already tested it on a staging server. What else do you propose?
There might be new cases that we need to cover, we can include them in unit tests.

pkg/parser/interface.go Outdated Show resolved Hide resolved
@grzkv
Copy link
Member

grzkv commented Jun 8, 2021

Can we please keep different changes in separate PRs? Linter change makes sense to do separately, and then back-merge from master.

I would appreciate we just do it in one MR. The commit is already different.

Sure. I'd appreciate if we did it in the future though.

@grzkv
Copy link
Member

grzkv commented Jun 8, 2021

I already tested it on a staging server. What else do you propose?

Great. This is exactly what I had in mind.

@bom-d-van bom-d-van force-pushed the parser/various-space-comma-bug-fixes branch from a08e802 to 81bf8cc Compare June 8, 2021 14:17
@bom-d-van bom-d-van merged commit 4d262f0 into master Jun 9, 2021
@bom-d-van bom-d-van deleted the parser/various-space-comma-bug-fixes branch June 9, 2021 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants