From a1790f2d0c40b29e65b76457a3019626f87e9158 Mon Sep 17 00:00:00 2001 From: David Norton Date: Sat, 18 Apr 2015 12:28:47 -0400 Subject: [PATCH 1/2] fix #2337: panic if tag key isn't double quoted --- CHANGELOG.md | 1 + cmd/influxd/server_integration_test.go | 6 +++ database.go | 73 +++++++++++++++++--------- server.go | 15 ++++-- tx.go | 5 +- 5 files changed, 72 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 653e9e767cc..1cfcdb2bcc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - [#2325](https://github.com/influxdb/influxdb/pull/2325): Cluster open fixes - [#2326](https://github.com/influxdb/influxdb/pull/2326): Fix parse error in CREATE CONTINUOUS QUERY - [#2300](https://github.com/influxdb/influxdb/pull/2300): Refactor integration tests. Properly close Graphite/OpenTSDB listeners. +- [#2338](https://github.com/influxdb/influxdb/pull/2338): Fix panic if tag key isn't double quoted when it should have been ## v0.9.0-rc25 [2015-04-15] diff --git a/cmd/influxd/server_integration_test.go b/cmd/influxd/server_integration_test.go index 3d01ec904af..a996c9e98e3 100644 --- a/cmd/influxd/server_integration_test.go +++ b/cmd/influxd/server_integration_test.go @@ -838,6 +838,12 @@ func runTestsData(t *testing.T, testName string, nodes Cluster, database, retent query: `select foo from "%DB%"."%RP%".where_events where tennant = 'paul' AND time > 1s AND (foo = 'bar' OR foo = 'baz')`, expected: `{"results":[{"series":[{"name":"where_events","columns":["time","foo"],"values":[["2009-11-10T23:00:02Z","bar"],["2009-11-10T23:00:03Z","baz"]]}]}]}`, }, + { + name: "where on tag that should be double quoted but isn't", + queryDb: "%DB%", + query: `show series where data-center = 'foo'`, + expectPattern: `invalid expression: .*`, + }, // LIMIT and OFFSET tests diff --git a/database.go b/database.go index 084f0ce51f3..4a5d77a8558 100644 --- a/database.go +++ b/database.go @@ -281,15 +281,19 @@ func (m *Measurement) seriesByTags(tags map[string]string) *Series { // filters walks the where clause of a select statement and returns a map with all series ids // matching the where clause and any filter expression that should be applied to each -func (m *Measurement) filters(stmt *influxql.SelectStatement) map[uint64]influxql.Expr { +func (m *Measurement) filters(stmt *influxql.SelectStatement) (map[uint64]influxql.Expr, error) { seriesIdsToExpr := make(map[uint64]influxql.Expr) if stmt.Condition == nil || stmt.OnlyTimeDimensions() { for _, id := range m.seriesIDs { seriesIdsToExpr[id] = nil } - return seriesIdsToExpr + return seriesIdsToExpr, nil + } + ids, _, _, err := m.walkWhereForSeriesIds(stmt.Condition, seriesIdsToExpr) + + if err != nil { + return nil, err } - ids, _, _ := m.walkWhereForSeriesIds(stmt.Condition, seriesIdsToExpr) // ensure every id is in the map for _, id := range ids { @@ -298,7 +302,7 @@ func (m *Measurement) filters(stmt *influxql.SelectStatement) map[uint64]influxq } } - return seriesIdsToExpr + return seriesIdsToExpr, nil } // tagSets returns the unique tag sets that exist for the given tag keys. This is used to determine @@ -308,9 +312,12 @@ func (m *Measurement) filters(stmt *influxql.SelectStatement) map[uint64]influxq // {"region": "uswest", "service": "redis"}, {"region": "uswest", "service": "mysql"}, etc... // This will also populate the TagSet objects with the series IDs that match each tagset and any // influx filter expression that goes with the series -func (m *Measurement) tagSets(stmt *influxql.SelectStatement, dimensions []string) []*influxql.TagSet { +func (m *Measurement) tagSets(stmt *influxql.SelectStatement, dimensions []string) ([]*influxql.TagSet, error) { // get the unique set of series ids and the filters that should be applied to each - filters := m.filters(stmt) + filters, err := m.filters(stmt) + if err != nil { + return nil, err + } // build the tag sets var tagStrings []string @@ -348,32 +355,35 @@ func (m *Measurement) tagSets(stmt *influxql.SelectStatement, dimensions []strin a = append(a, tagSets[s]) } - return a + return a, nil } // idsForExpr will return a collection of series ids, a bool indicating if the result should be // used (it'll be false if it's a time expr) and a field expression if the passed in expression is against a field. -func (m *Measurement) idsForExpr(n *influxql.BinaryExpr) (seriesIDs, bool, influxql.Expr) { +func (m *Measurement) idsForExpr(n *influxql.BinaryExpr) (seriesIDs, bool, influxql.Expr, error) { name, ok := n.LHS.(*influxql.VarRef) value := n.RHS if !ok { - name, _ = n.RHS.(*influxql.VarRef) + name, ok = n.RHS.(*influxql.VarRef) + if !ok { + return nil, false, nil, fmt.Errorf("invalid expression: %s", n.String()) + } value = n.LHS } // ignore time literals if _, ok := value.(*influxql.TimeLiteral); ok || name.Val == "time" { - return nil, false, nil + return nil, false, nil, nil } // if it's a field we can't collapse it so we have to look at all series ids for this if m.FieldByName(name.Val) != nil { - return m.seriesIDs, true, n + return m.seriesIDs, true, n, nil } tagVals, ok := m.seriesByTagKeyValue[name.Val] if !ok { - return nil, true, nil + return nil, true, nil, nil } // if we're looking for series with specific tag values @@ -386,7 +396,7 @@ func (m *Measurement) idsForExpr(n *influxql.BinaryExpr) (seriesIDs, bool, influ } else if n.Op == influxql.NEQ { ids = m.seriesIDs.reject(tagVals[str.Val]) } - return ids, true, nil + return ids, true, nil, nil } // if we're looking for series with tag values that match a regex @@ -408,32 +418,44 @@ func (m *Measurement) idsForExpr(n *influxql.BinaryExpr) (seriesIDs, bool, influ ids = ids.reject(tagVals[k]) } } - return ids, true, nil + return ids, true, nil, nil } - return nil, true, nil + return nil, true, nil, nil } // walkWhereForSeriesIds will recursively walk the where clause and return a collection of series ids, a boolean indicating if this return // value should be included in the resulting set, and an expression if the return is a field expression. // The map that it takes maps each series id to the field expression that should be used to evaluate it when iterating over its cursor. // Series that have no field expressions won't be in the map -func (m *Measurement) walkWhereForSeriesIds(expr influxql.Expr, filters map[uint64]influxql.Expr) (seriesIDs, bool, influxql.Expr) { +func (m *Measurement) walkWhereForSeriesIds(expr influxql.Expr, filters map[uint64]influxql.Expr) (seriesIDs, bool, influxql.Expr, error) { switch n := expr.(type) { case *influxql.BinaryExpr: switch n.Op { case influxql.EQ, influxql.NEQ, influxql.LT, influxql.LTE, influxql.GT, influxql.GTE, influxql.EQREGEX, influxql.NEQREGEX: // if it's a compare, then it's either a field expression or against a tag. we can return this - ids, shouldInclude, expr := m.idsForExpr(n) + ids, shouldInclude, expr, err := m.idsForExpr(n) + if err != nil { + return nil, false, nil, err + } + for _, id := range ids { filters[id] = expr } - return ids, shouldInclude, expr + + return ids, shouldInclude, expr, nil case influxql.AND, influxql.OR: // if it's an AND or OR we need to union or intersect the results var ids seriesIDs - l, il, lexpr := m.walkWhereForSeriesIds(n.LHS, filters) - r, ir, rexpr := m.walkWhereForSeriesIds(n.RHS, filters) + l, il, lexpr, err := m.walkWhereForSeriesIds(n.LHS, filters) + if err != nil { + return nil, false, nil, err + } + + r, ir, rexpr, err := m.walkWhereForSeriesIds(n.RHS, filters) + if err != nil { + return nil, false, nil, err + } if il && ir { // we should include both the LHS and RHS of the BinaryExpr in the return if n.Op == influxql.AND { @@ -442,7 +464,7 @@ func (m *Measurement) walkWhereForSeriesIds(expr influxql.Expr, filters map[uint ids = l.union(r) } } else if !il && !ir { // we don't need to include either so return nothing - return nil, false, nil + return nil, false, nil, nil } else if il { // just include the left side ids = l } else { // just include the right side @@ -492,7 +514,7 @@ func (m *Measurement) walkWhereForSeriesIds(expr influxql.Expr, filters map[uint } // finally return the ids and say that we should include them - return ids, true, nil + return ids, true, nil, nil } return m.idsForExpr(n) @@ -500,7 +522,7 @@ func (m *Measurement) walkWhereForSeriesIds(expr influxql.Expr, filters map[uint // walk down the tree return m.walkWhereForSeriesIds(n.Expr, filters) default: - return nil, false, nil + return nil, false, nil, nil } } @@ -574,7 +596,10 @@ func (m *Measurement) seriesIDsAllOrByExpr(expr influxql.Expr) (seriesIDs, error // Get series IDs that match the WHERE clause. filters := map[uint64]influxql.Expr{} - ids, _, _ := m.walkWhereForSeriesIds(expr, filters) + ids, _, _, err := m.walkWhereForSeriesIds(expr, filters) + if err != nil { + return nil, err + } return ids, nil } diff --git a/server.go b/server.go index 1b1b6a2287b..2f664648652 100644 --- a/server.go +++ b/server.go @@ -2503,7 +2503,10 @@ func (s *Server) executeDropSeriesStatement(stmt *influxql.DropSeriesStatement, if stmt.Condition != nil { // Get series IDs that match the WHERE clause. filters := map[uint64]influxql.Expr{} - ids, _, _ = m.walkWhereForSeriesIds(stmt.Condition, filters) + ids, _, _, err = m.walkWhereForSeriesIds(stmt.Condition, filters) + if err != nil { + return &Result{Err: err} + } // TODO: check return of walkWhereForSeriesIds for fields } else { @@ -2546,7 +2549,10 @@ func (s *Server) executeShowSeriesStatement(stmt *influxql.ShowSeriesStatement, if stmt.Condition != nil { // Get series IDs that match the WHERE clause. filters := map[uint64]influxql.Expr{} - ids, _, _ = m.walkWhereForSeriesIds(stmt.Condition, filters) + ids, _, _, err = m.walkWhereForSeriesIds(stmt.Condition, filters) + if err != nil { + return &Result{Err: err} + } // If no series matched, then go to the next measurement. if len(ids) == 0 { @@ -2761,7 +2767,10 @@ func (s *Server) executeShowTagValuesStatement(stmt *influxql.ShowTagValuesState if stmt.Condition != nil { // Get series IDs that match the WHERE clause. filters := map[uint64]influxql.Expr{} - ids, _, _ = m.walkWhereForSeriesIds(stmt.Condition, filters) + ids, _, _, err = m.walkWhereForSeriesIds(stmt.Condition, filters) + if err != nil { + return &Result{Err: err} + } // If no series matched, then go to the next measurement. if len(ids) == 0 { diff --git a/tx.go b/tx.go index e0fb85d331c..8cabd168bc8 100644 --- a/tx.go +++ b/tx.go @@ -122,7 +122,10 @@ func (tx *tx) CreateMapReduceJobs(stmt *influxql.SelectStatement, tagKeys []stri } // get the sorted unique tag sets for this query. - tagSets := m.tagSets(stmt, tagKeys) + tagSets, err := m.tagSets(stmt, tagKeys) + if err != nil { + return nil, err + } //jobs := make([]*influxql.MapReduceJob, 0, len(tagSets)) for _, t := range tagSets { From ed3ff1ce161391c5b6df7be1328f1a596ad55fc3 Mon Sep 17 00:00:00 2001 From: David Norton Date: Sat, 18 Apr 2015 13:12:54 -0400 Subject: [PATCH 2/2] fix #2337: code review change --- database.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/database.go b/database.go index 4a5d77a8558..d967f1c05bf 100644 --- a/database.go +++ b/database.go @@ -283,14 +283,15 @@ func (m *Measurement) seriesByTags(tags map[string]string) *Series { // matching the where clause and any filter expression that should be applied to each func (m *Measurement) filters(stmt *influxql.SelectStatement) (map[uint64]influxql.Expr, error) { seriesIdsToExpr := make(map[uint64]influxql.Expr) + if stmt.Condition == nil || stmt.OnlyTimeDimensions() { for _, id := range m.seriesIDs { seriesIdsToExpr[id] = nil } return seriesIdsToExpr, nil } - ids, _, _, err := m.walkWhereForSeriesIds(stmt.Condition, seriesIdsToExpr) + ids, _, _, err := m.walkWhereForSeriesIds(stmt.Condition, seriesIdsToExpr) if err != nil { return nil, err }