logcli: Query needs to be stored into url.RawQuery, and not url.Path (#2027)

* Query needs to be stored into url.RawQuery, and not url.Path

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

* QueryStringBuilder deals with queries, not paths. Removed unused method.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

* Fixed test.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pkg/logcli/client/client.go

@@ -21,12 +21,12 @@ import (
 )
 
 const (
-	queryPath       = "/loki/api/v1/query?query=%s&limit=%d&time=%d&direction=%s"
+	queryPath       = "/loki/api/v1/query"
 	queryRangePath  = "/loki/api/v1/query_range"
 	labelsPath      = "/loki/api/v1/labels"
 	labelValuesPath = "/loki/api/v1/label/%s/values"
 	seriesPath      = "/loki/api/v1/series"
-	tailPath        = "/loki/api/v1/tail?query=%s&delay_for=%d&limit=%d&start=%d"
+	tailPath        = "/loki/api/v1/tail"
 )
 
 // Client contains fields necessary to query a Loki instance
@@ -42,14 +42,13 @@ type Client struct {
 // excluding interfacer b/c it suggests taking the interface promql.Node instead of logproto.Direction b/c it happens to have a String() method
 // nolint:interfacer
 func (c *Client) Query(queryStr string, limit int, time time.Time, direction logproto.Direction, quiet bool) (*loghttp.QueryResponse, error) {
-	path := fmt.Sprintf(queryPath,
-		url.QueryEscape(queryStr), // query
-		limit,                     // limit
-		time.UnixNano(),           // start
-		direction.String(),        // direction
-	)
-
-	return c.doQuery(path, quiet)
+	qsb := util.NewQueryStringBuilder()
+	qsb.SetString("query", queryStr)
+	qsb.SetInt("limit", int64(limit))
+	qsb.SetInt("start", time.UnixNano())
+	qsb.SetString("direction", direction.String())
+
+	return c.doQuery(queryPath, qsb.Encode(), quiet)
 }
 
 // QueryRange uses the /api/v1/query_range endpoint to execute a range query
@@ -73,13 +72,13 @@ func (c *Client) QueryRange(queryStr string, limit int, from, through time.Time,
 		params.SetInt("interval", int64(interval.Seconds()))
 	}
 
-	return c.doQuery(params.EncodeWithPath(queryRangePath), quiet)
+	return c.doQuery(queryRangePath, params.Encode(), quiet)
 }
 
 // ListLabelNames uses the /api/v1/label endpoint to list label names
 func (c *Client) ListLabelNames(quiet bool) (*loghttp.LabelResponse, error) {
 	var labelResponse loghttp.LabelResponse
-	if err := c.doRequest(labelsPath, quiet, &labelResponse); err != nil {
+	if err := c.doRequest(labelsPath, "", quiet, &labelResponse); err != nil {
 		return nil, err
 	}
 	return &labelResponse, nil
@@ -89,7 +88,7 @@ func (c *Client) ListLabelNames(quiet bool) (*loghttp.LabelResponse, error) {
 func (c *Client) ListLabelValues(name string, quiet bool) (*loghttp.LabelResponse, error) {
 	path := fmt.Sprintf(labelValuesPath, url.PathEscape(name))
 	var labelResponse loghttp.LabelResponse
-	if err := c.doRequest(path, quiet, &labelResponse); err != nil {
+	if err := c.doRequest(path, "", quiet, &labelResponse); err != nil {
 		return nil, err
 	}
 	return &labelResponse, nil
@@ -102,26 +101,26 @@ func (c *Client) Series(matchers []string, from, through time.Time, quiet bool)
 	params.SetStringArray("match", matchers)
 
 	var seriesResponse loghttp.SeriesResponse
-	if err := c.doRequest(params.EncodeWithPath(seriesPath), quiet, &seriesResponse); err != nil {
+	if err := c.doRequest(seriesPath, params.Encode(), quiet, &seriesResponse); err != nil {
 		return nil, err
 	}
 	return &seriesResponse, nil
 }
 
-func (c *Client) doQuery(path string, quiet bool) (*loghttp.QueryResponse, error) {
+func (c *Client) doQuery(path string, query string, quiet bool) (*loghttp.QueryResponse, error) {
 	var err error
 	var r loghttp.QueryResponse
 
-	if err = c.doRequest(path, quiet, &r); err != nil {
+	if err = c.doRequest(path, query, quiet, &r); err != nil {
 		return nil, err
 	}
 
 	return &r, nil
 }
 
-func (c *Client) doRequest(path string, quiet bool, out interface{}) error {
+func (c *Client) doRequest(path, query string, quiet bool, out interface{}) error {
 
-	us, err := buildURL(c.Address, path)
+	us, err := buildURL(c.Address, path, query)
 	if err != nil {
 		return err
 	}
@@ -170,17 +169,17 @@ func (c *Client) doRequest(path string, quiet bool, out interface{}) error {
 
 // LiveTailQueryConn uses /api/prom/tail to set up a websocket connection and returns it
 func (c *Client) LiveTailQueryConn(queryStr string, delayFor int, limit int, from int64, quiet bool) (*websocket.Conn, error) {
-	path := fmt.Sprintf(tailPath,
-		url.QueryEscape(queryStr), // query
-		delayFor,                  // delay_for
-		limit,                     // limit
-		from,                      // start
-	)
-	return c.wsConnect(path, quiet)
+	qsb := util.NewQueryStringBuilder()
+	qsb.SetString("query", queryStr)
+	qsb.SetInt("delay_for", int64(delayFor))
+	qsb.SetInt("limit", int64(limit))
+	qsb.SetInt("from", from)
+
+	return c.wsConnect(tailPath, qsb.Encode(), quiet)
 }
 
-func (c *Client) wsConnect(path string, quiet bool) (*websocket.Conn, error) {
-	us, err := buildURL(c.Address, path)
+func (c *Client) wsConnect(path, query string, quiet bool) (*websocket.Conn, error) {
+	us, err := buildURL(c.Address, path, query)
 	if err != nil {
 		return nil, err
 	}
@@ -223,11 +222,12 @@ func (c *Client) wsConnect(path string, quiet bool) (*websocket.Conn, error) {
 }
 
 // buildURL concats a url `http://foo/bar` with a path `/buzz`.
-func buildURL(u, p string) (string, error) {
+func buildURL(u, p, q string) (string, error) {
 	url, err := url.Parse(u)
 	if err != nil {
 		return "", err
 	}
 	url.Path = path.Join(url.Path, p)
+	url.RawQuery = q
 	return url.String(), nil
 }

pkg/logcli/client/client_test.go

@@ -5,17 +5,17 @@ import "testing"
 func Test_buildURL(t *testing.T) {
 	tests := []struct {
 		name    string
-		u, p    string
+		u, p, q string
 		want    string
 		wantErr bool
 	}{
-		{"err", "8://2", "/bar", "", true},
-		{"strip /", "http://localhost//", "//bar", "http://localhost/bar", false},
-		{"sub path", "https://localhost/loki/", "/bar/foo", "https://localhost/loki/bar/foo", false},
+		{"err", "8://2", "/bar", "", "", true},
+		{"strip /", "http://localhost//", "//bar", "a=b", "http://localhost/bar?a=b", false},
+		{"sub path", "https://localhost/loki/", "/bar/foo", "c=d&e=f", "https://localhost/loki/bar/foo?c=d&e=f", false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := buildURL(tt.u, tt.p)
+			got, err := buildURL(tt.u, tt.p, tt.q)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("buildURL() error = %v, wantErr %v", err, tt.wantErr)
 				return

pkg/util/query_string_builder.go

@@ -46,16 +46,3 @@ func (b *QueryStringBuilder) SetFloat32(name string, value float32) {
 func (b *QueryStringBuilder) Encode() string {
 	return b.values.Encode()
 }
-
-// Encode returns the URL-encoded query string, prefixing it with the
-// input URL path.
-func (b *QueryStringBuilder) EncodeWithPath(path string) string {
-	queryString := b.Encode()
-	if path == "" {
-		return queryString
-	} else if queryString == "" {
-		return path
-	}
-
-	return path + "?" + queryString
-}

pkg/util/query_string_builder_test.go

@@ -14,12 +14,10 @@ func TestQueryStringBuilder(t *testing.T) {
 	tests := map[string]struct {
 		input           map[string]interface{}
 		expectedEncoded string
-		expectedPath    string
 	}{
 		"should return an empty query string on no params": {
 			input:           map[string]interface{}{},
 			expectedEncoded: "",
-			expectedPath:    "/test",
 		},
 		"should return the URL encoded query string parameters": {
 			input: map[string]interface{}{
@@ -31,7 +29,6 @@ func TestQueryStringBuilder(t *testing.T) {
 				"string":     "foo",
 			},
 			expectedEncoded: "float32=123.456&float64=123.456&float64int=12345&int32=32&int64=64&string=foo",
-			expectedPath:    "/test?float32=123.456&float64=123.456&float64int=12345&int32=32&int64=64&string=foo",
 		},
 	}
 
@@ -59,7 +56,6 @@ func TestQueryStringBuilder(t *testing.T) {
 			}
 
 			assert.Equal(t, testData.expectedEncoded, params.Encode())
-			assert.Equal(t, testData.expectedPath, params.EncodeWithPath("/test"))
 		})
 	}
 }