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

fix: detect misquoted tag values and return an error #22754

Merged
merged 5 commits into from
Oct 27, 2021
Merged

fix: detect misquoted tag values and return an error #22754

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4643eba
fix: detect misquoted tag values and replace them
davidby-influx Oct 25, 2021
72ee1f3
fix: return an error for misquoted tag value
davidby-influx Oct 26, 2021
31d17de
fix: check for misquoted tag values in SHOW TAG VALUES
davidby-influx Oct 26, 2021
46327a6
chore: formatting
davidby-influx Oct 26, 2021
c9cfa50
chore: clarify error message
davidby-influx Oct 27, 2021
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
32 changes: 31 additions & 1 deletion tsdb/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1654,6 +1654,9 @@ func (s *Store) TagKeys(ctx context.Context, auth query.FineAuthorizer, shardIDs
if err != nil {
return nil, err
}
if err = isBadQuoteTagValueClause(filterExpr); err != nil {
return nil, err
}

// Get all the shards we're interested in.
is := IndexSet{Indexes: make([]Index, 0, len(shardIDs))}
Expand Down Expand Up @@ -1822,6 +1825,31 @@ func isTagKeyClause(e influxql.Expr) (bool, error) {
return false, nil
}

func isBadQuoteTagValueClause(e influxql.Expr) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should put in there permanently, but do any tests fail if we put this check on SELECT as well? might highlight a case we missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SELECT ... WHERE supports the syntax this disallows, so this will err on legal queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are the test failures where this test breaks correct queries in SELECT

--- FAIL: TestServer_Query_Where_With_Tags (0.57s)
    --- FAIL: TestServer_Query_Where_With_Tags/where_comparing_tag_and_field (0.00s)
        server_test.go:6457: where comparing tag and field: unexpected results
            query:  select foo from where_events where tennant != foo
            params:  map[db:[db0]]
            exp:    {"results":[{"statement_id":0,"series":[{"name":"where_events","columns":["time","foo"],"values":[["2009-11-10T23:00:02Z","bar"],["2009-11-10T23:00:03Z","baz"],["2009-11-10T23:00:04Z","bat"],["2009-11-10T23:00:05Z","bar"],["2009-11-10T23:00:06Z","bap"]]}]}]}
            actual: {"results":[{"statement_id":0,"error":"bad WHERE clause. tag value must be inside single quotes: tennant != foo"}]}
            
    --- FAIL: TestServer_Query_Where_With_Tags/where_comparing_tag_and_tag (0.00s)
        server_test.go:6457: where comparing tag and tag: unexpected results
            query:  select foo from where_events where tennant = tennant
            params:  map[db:[db0]]
            exp:    {"results":[{"statement_id":0,"series":[{"name":"where_events","columns":["time","foo"],"values":[["2009-11-10T23:00:02Z","bar"],["2009-11-10T23:00:03Z","baz"],["2009-11-10T23:00:04Z","bat"],["2009-11-10T23:00:05Z","bar"],["2009-11-10T23:00:06Z","bap"]]}]}]}
            actual: {"results":[{"statement_id":0,"error":"bad WHERE clause. tag value must be inside single quotes: tennant = tennant"}]}

switch e := e.(type) {
case *influxql.BinaryExpr:
switch e.Op {
case influxql.EQ, influxql.NEQ:
_, lOk := e.LHS.(*influxql.VarRef)
_, rOk := e.RHS.(*influxql.VarRef)
if lOk && rOk {
return fmt.Errorf("bad WHERE clause. tag value must be inside single quotes: %s", e.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("bad WHERE clause. tag value must be inside single quotes: %s", e.String())
return fmt.Errorf("bad WHERE clause for metaquery: either LHS or RHS must be a constant, not a tag or field: %s", e.String())

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should emphasize in the error that this is only a metaquery restriction, since normal selects are OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to mention single quotes so they know how to fix it. I don't think the fact that single quotes give literals and double quotes give fields and tags is very obvious. I also don't like LHS and RHS; too compiler-geeky. How about:

"bad WHERE clause for metaquery. one term must be a string literal tag value within single quotes: %s"

The user will know which term should be the tag value, presumably.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, though I'd slightly prefer : instead of . since that's generally the style we use for error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was formatting with period based on another message in that file:

"cannot delete data. DB contains shards using both inmem and tsi1 indexes. Please convert all shards to use the same index type to delete data."

Maybe I should have made a wider search for compound error statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compromise, because I hate two colons in a singe sentence. semi-colon.

}
case influxql.OR, influxql.AND:
if err := isBadQuoteTagValueClause(e.LHS); err != nil {
return err
} else if err = isBadQuoteTagValueClause(e.RHS); err != nil {
return err
} else {
return nil
}
}
case *influxql.ParenExpr:
return isBadQuoteTagValueClause(e.Expr)
}
return nil
}

// TagValues returns the tag keys and values for the provided shards, where the
// tag values satisfy the provided condition.
func (s *Store) TagValues(ctx context.Context, auth query.FineAuthorizer, shardIDs []uint64, cond influxql.Expr) ([]TagValues, error) {
Expand Down Expand Up @@ -1856,7 +1884,9 @@ func (s *Store) TagValues(ctx context.Context, auth query.FineAuthorizer, shardI
if err != nil {
return nil, err
}

if err = isBadQuoteTagValueClause(filterExpr); err != nil {
return nil, err
}
// Build index set to work on.
is := IndexSet{Indexes: make([]Index, 0, len(shardIDs))}
s.mu.RLock()
Expand Down