From 446950979a8f1de2d9ff8cb09e3cf728361f5423 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Mon, 28 Dec 2020 11:00:46 -0600 Subject: [PATCH] fix potential nil dereference in SignedRow.ToRows This used to be possible to hit, but I think now that Distinct on a set field returns a *Row rather than a SignedRow it isn't an issue. (I wasn't able to trigger it in the tests). Adding the fix anyway as it seems safer than not. The rest of the changes are test infrastructure to make it easy to call GRPC queries and verify the results as CSV. --- executor.go | 58 ++++++++++++---------- executor_test.go | 120 +++++++++++++++++++++++++++++++++++++++------ proto/interface.go | 50 +++++++++++++++++++ server.go | 4 ++ test/cluster.go | 29 +++++++++++ test/pilosa.go | 4 +- 6 files changed, 220 insertions(+), 45 deletions(-) diff --git a/executor.go b/executor.go index 6694deeb1..190ce07f3 100644 --- a/executor.go +++ b/executor.go @@ -6690,38 +6690,42 @@ func (s SignedRow) ToTable() (*pb.TableResponse, error) { func (s SignedRow) ToRows(callback func(*pb.RowResponse) error) error { ci := []*pb.ColumnInfo{{Name: s.Field(), Datatype: "int64"}} - negs := s.Neg.Columns() - for i := len(negs) - 1; i >= 0; i-- { - val, err := toNegInt64(negs[i]) - if err != nil { - return errors.Wrap(err, "converting uint64 to int64 (negative)") - } + if s.Neg != nil { + negs := s.Neg.Columns() + for i := len(negs) - 1; i >= 0; i-- { + val, err := toNegInt64(negs[i]) + if err != nil { + return errors.Wrap(err, "converting uint64 to int64 (negative)") + } - if err := callback(&pb.RowResponse{ - Headers: ci, - Columns: []*pb.ColumnResponse{ - &pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: val}}, - }, - }); err != nil { - return errors.Wrap(err, "calling callback") + if err := callback(&pb.RowResponse{ + Headers: ci, + Columns: []*pb.ColumnResponse{ + &pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: val}}, + }, + }); err != nil { + return errors.Wrap(err, "calling callback") + } + ci = nil } - ci = nil } - for _, id := range s.Pos.Columns() { - val, err := toInt64(id) - if err != nil { - return errors.Wrap(err, "converting uint64 to int64 (positive)") - } + if s.Pos != nil { + for _, id := range s.Pos.Columns() { + val, err := toInt64(id) + if err != nil { + return errors.Wrap(err, "converting uint64 to int64 (positive)") + } - if err := callback(&pb.RowResponse{ - Headers: ci, - Columns: []*pb.ColumnResponse{ - &pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: val}}, - }, - }); err != nil { - return errors.Wrap(err, "calling callback") + if err := callback(&pb.RowResponse{ + Headers: ci, + Columns: []*pb.ColumnResponse{ + &pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: val}}, + }, + }); err != nil { + return errors.Wrap(err, "calling callback") + } + ci = nil } - ci = nil } return nil } diff --git a/executor_test.go b/executor_test.go index e065004ef..ce51d514e 100644 --- a/executor_test.go +++ b/executor_test.go @@ -6797,44 +6797,69 @@ func TestVariousQueries(t *testing.T) { }) tests := []struct { - query string - verifier func(t *testing.T, resp pilosa.QueryResponse) + query string + qrVerifier func(t *testing.T, resp pilosa.QueryResponse) + csvVerifier func(t *testing.T, resp string) }{ { query: "Count(All())", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if resp.Results[0].(uint64) != 6 { t.Errorf("expected 6, got %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "6\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Count(Distinct(field=likenums))", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if resp.Results[0].(uint64) != 7 { t.Errorf("wrong count: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "7\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(field=likenums)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Columns(), []uint64{1, 2, 3, 4, 5, 6, 7}) { t.Errorf("wrong values: %+v %+v", resp.Results[0].(*pilosa.Row).Columns(), resp.Results[0].(*pilosa.Row)) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "1\n2\n3\n4\n5\n6\n7\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Count(Distinct(field=likes))", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if resp.Results[0].(uint64) != 7 { t.Errorf("wrong count: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "7\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(field=affinity)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(pilosa.SignedRow).Pos.Columns(), []uint64{0, 5, 10}) { t.Errorf("wrong positive records: %+v", resp.Results[0].(pilosa.SignedRow).Pos.Columns()) } @@ -6842,10 +6867,16 @@ func TestVariousQueries(t *testing.T) { t.Errorf("wrong negative records: %+v", resp.Results[0].(pilosa.SignedRow).Neg.Columns()) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "-10\n-5\n0\n5\n10\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(Row(affinity>=0),field=affinity)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(pilosa.SignedRow).Pos.Columns(), []uint64{0, 5, 10}) { t.Errorf("wrong positive records: %+v", resp.Results[0].(pilosa.SignedRow).Pos.Columns()) } @@ -6853,14 +6884,26 @@ func TestVariousQueries(t *testing.T) { t.Errorf("wrong negative records: %+v", resp.Results[0].(pilosa.SignedRow).Neg.Columns()) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "0\n5\n10\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Count(Distinct(Row(affinity>=0),field=affinity))", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if resp.Results[0].(uint64) != 3 { t.Errorf("wrong number of values: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "3\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, // Handling this case properly will require changing the way @@ -6879,58 +6922,103 @@ func TestVariousQueries(t *testing.T) { // }, { query: "Distinct(Row(affinity<0),field=likes)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"pilosa", "zebra", "icecream"}) { t.Errorf("wrong values: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "pilosa\nzebra\nicecream\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(Row(affinity>0),field=likes)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "pangolin", "icecream"}) { t.Errorf("wrong values: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "molecula\npangolin\nicecream\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(Row(likenums=1),field=likes)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "icecream"}) { t.Errorf("wrong values: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "molecula\nicecream\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(field=likes)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "pilosa", "pangolin", "zebra", "toucan", "dog", "icecream"}) { t.Errorf("wrong values: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "molecula\npilosa\npangolin\nzebra\ntoucan\ndog\nicecream\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(All(),field=likes)", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "pilosa", "pangolin", "zebra", "toucan", "dog", "icecream"}) { t.Errorf("wrong values: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "molecula\npilosa\npangolin\nzebra\ntoucan\ndog\nicecream\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, { query: "Distinct(field=likes )", - verifier: func(t *testing.T, resp pilosa.QueryResponse) { + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "pilosa", "pangolin", "zebra", "toucan", "dog", "icecream"}) { t.Errorf("wrong values: %+v", resp.Results[0]) } }, + csvVerifier: func(t *testing.T, resp string) { + exp := "molecula\npilosa\npangolin\nzebra\ntoucan\ndog\nicecream\n" + if resp != exp { + t.Errorf("expected '%s', got '%s'", exp, resp) + } + }, }, } for i, tst := range tests { t.Run(fmt.Sprintf("%d-%s", i, tst.query), func(t *testing.T) { resp := c.Query(t, "users", tst.query) - tst.verifier(t, resp) + tr := c.QueryGRPC(t, "users", tst.query) + if tst.qrVerifier != nil { + tst.qrVerifier(t, resp) + } + csvString := tr.ToCSVString() + // verify everything after header + tst.csvVerifier(t, csvString[strings.Index(csvString, "\n")+1:]) + + // TODO: add HTTP and Postgres and ability to convert + // those results to CSV to run through CSV verifier }) } } diff --git a/proto/interface.go b/proto/interface.go index 77ba3fd8d..321687e94 100644 --- a/proto/interface.go +++ b/proto/interface.go @@ -15,6 +15,8 @@ package proto import ( + "bytes" + "encoding/csv" "fmt" "io" "strings" @@ -325,3 +327,51 @@ func (c ConstRowser) ToRows(fn func(*RowResponse) error) error { return nil } + +func (m *TableResponse) ToCSV(w io.Writer) error { + writer := csv.NewWriter(w) + record := make([]string, len(m.Headers)) + for i, h := range m.Headers { + record[i] = h.Name + } + err := writer.Write(record) + if err != nil { + return errors.Wrap(err, "writing header") + } + for i, row := range m.Rows { + record = record[:0] + for colIndex, col := range row.Columns { + switch m.Headers[colIndex].Datatype { + case "[]string": + record = append(record, fmt.Sprintf("%v", col.GetStringArrayVal())) + case "[]uint64": + record = append(record, fmt.Sprintf("%v", col.GetUint64ArrayVal())) + case "string": + record = append(record, fmt.Sprintf("%v", col.GetStringVal())) + case "uint64": + record = append(record, fmt.Sprintf("%v", col.GetUint64Val())) + case "decimal": + record = append(record, fmt.Sprintf("%v", col.GetDecimalVal().String())) + case "bool": + record = append(record, fmt.Sprintf("%v", col.GetBoolVal())) + case "int64": + record = append(record, fmt.Sprintf("%v", col.GetInt64Val())) + } + } + err := writer.Write(record) + if err != nil { + return errors.Wrapf(err, "writing row %d", i) + } + } + writer.Flush() + return nil +} + +func (m *TableResponse) ToCSVString() string { + buf := &bytes.Buffer{} + err := m.ToCSV(buf) + if err != nil { + panic(fmt.Sprintf("shouldn't get an error writing to bytes.Buffer, got: %v", err)) + } + return buf.String() +} diff --git a/server.go b/server.go index b664e4c87..213d738d1 100644 --- a/server.go +++ b/server.go @@ -499,6 +499,10 @@ func (s *Server) InternalClient() InternalClient { return s.defaultClient } +func (s *Server) GRPCURI() URI { + return s.grpcURI +} + // UpAndDown brings the server up minimally and shuts it down // again; basically, it exists for testing holder open and close. func (s *Server) UpAndDown() error { diff --git a/test/cluster.go b/test/cluster.go index 74b1e31ff..bee281e58 100644 --- a/test/cluster.go +++ b/test/cluster.go @@ -26,6 +26,8 @@ import ( "time" "github.com/pilosa/pilosa/v2" + "github.com/pilosa/pilosa/v2/api/client" + "github.com/pilosa/pilosa/v2/proto" "github.com/pilosa/pilosa/v2/server" "github.com/pkg/errors" ) @@ -53,6 +55,33 @@ func (c *Cluster) Query(t testing.TB, index, query string) pilosa.QueryResponse return c.Nodes[0].QueryAPI(t, &pilosa.QueryRequest{Index: index, Query: query}) } +func (c *Cluster) QueryHTTP(t testing.TB, index, query string) (string, error) { + t.Helper() + if len(c.Nodes) == 0 { + t.Fatal("must have at least one node in cluster to QueryHTTP") + } + return c.Nodes[0].Query(t, index, "", query) +} + +func (c *Cluster) QueryGRPC(t testing.TB, index, query string) *proto.TableResponse { + t.Helper() + if len(c.Nodes) == 0 { + t.Fatal("must have at least one node in cluster to QueryGRPC") + } + + grpcClient, err := client.NewGRPCClient([]string{fmt.Sprintf("%s:%d", c.Nodes[0].Server.GRPCURI().Host, c.Nodes[0].Server.GRPCURI().Port)}, nil) + if err != nil { + t.Fatalf("getting GRPC client: %v", err) + } + + tableResp, err := grpcClient.QueryUnary(context.Background(), index, query) + if err != nil { + t.Fatalf("querying unary: %v", err) + } + + return tableResp +} + func (c *Cluster) GetNode(n int) *Command { return c.Nodes[n] } diff --git a/test/pilosa.go b/test/pilosa.go index d7fdf7aaa..264e3f4c9 100644 --- a/test/pilosa.go +++ b/test/pilosa.go @@ -196,7 +196,7 @@ func (m *Command) Client() *http.InternalClient { } // Query executes a query against the program through the HTTP API. -func (m *Command) Query(t *testing.T, index, rawQuery, query string) (string, error) { +func (m *Command) Query(t testing.TB, index, rawQuery, query string) (string, error) { resp := Do(t, "POST", fmt.Sprintf("%s/index/%s/query?%s", m.URL(), index, rawQuery), query) if resp.StatusCode != gohttp.StatusOK { return "", fmt.Errorf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) @@ -285,7 +285,7 @@ func (m *Command) RecalculateCaches(t *testing.T) error { } // Do executes http.Do() with an http.NewRequest(). -func Do(t *testing.T, method, urlStr string, body string) *httpResponse { +func Do(t testing.TB, method, urlStr string, body string) *httpResponse { t.Helper() req, err := gohttp.NewRequest( method,