logcli: introduced QueryStringBuilder utility to clean up query string encoding (#1115)

pkg/logcli/client/client.go

@@ -8,11 +8,11 @@ import (
 	"log"
 	"net/http"
 	"net/url"
-	"strconv"
 	"strings"
 	"time"
 
 	"github.com/grafana/loki/pkg/loghttp"
+	"github.com/grafana/loki/pkg/util"
 
 	"github.com/gorilla/websocket"
 	"github.com/prometheus/common/config"
@@ -54,20 +54,20 @@ func (c *Client) Query(queryStr string, limit int, time time.Time, direction log
 // 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) QueryRange(queryStr string, limit int, from, through time.Time, direction logproto.Direction, step time.Duration, quiet bool) (*loghttp.QueryResponse, error) {
-	params := url.Values{}
-	params.Set("query", queryStr)
-	params.Set("limit", strconv.Itoa(limit))
-	params.Set("start", strconv.FormatInt(from.UnixNano(), 10))
-	params.Set("end", strconv.FormatInt(through.UnixNano(), 10))
-	params.Set("direction", direction.String())
+	params := util.NewQueryStringBuilder()
+	params.SetString("query", queryStr)
+	params.SetInt32("limit", limit)
+	params.SetInt("start", from.UnixNano())
+	params.SetInt("end", through.UnixNano())
+	params.SetString("direction", direction.String())
 
 	// The step is optional, so we do set it only if provided,
 	// otherwise we do leverage on the API defaults
 	if step != 0 {
-		params.Set("step", strconv.FormatInt(int64(step.Seconds()), 10))
+		params.SetInt("step", int64(step.Seconds()))
 	}
 
-	return c.doQuery(queryRangePath+"?"+params.Encode(), quiet)
+	return c.doQuery(params.EncodeWithPath(queryRangePath), quiet)
 }
 
 // ListLabelNames uses the /api/v1/label endpoint to list label names

pkg/util/query_string_builder.go

@@ -0,0 +1,55 @@
+package util
+
+import (
+	"net/url"
+	"strconv"
+)
+
+type QueryStringBuilder struct {
+	values url.Values
+}
+
+func NewQueryStringBuilder() *QueryStringBuilder {
+	return &QueryStringBuilder{
+		values: url.Values{},
+	}
+}
+
+func (b *QueryStringBuilder) SetString(name, value string) {
+	b.values.Set(name, value)
+}
+
+func (b *QueryStringBuilder) SetInt(name string, value int64) {
+	b.SetString(name, strconv.FormatInt(value, 10))
+}
+
+func (b *QueryStringBuilder) SetInt32(name string, value int) {
+	b.SetString(name, strconv.Itoa(value))
+}
+
+func (b *QueryStringBuilder) SetFloat(name string, value float64) {
+	b.SetString(name, strconv.FormatFloat(value, 'f', -1, 64))
+}
+
+func (b *QueryStringBuilder) SetFloat32(name string, value float32) {
+	b.SetString(name, strconv.FormatFloat(float64(value), 'f', -1, 32))
+}
+
+// Encode returns the URL-encoded query string based on key-value
+// parameters added to the builder calling Set functions.
+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

@@ -0,0 +1,65 @@
+package util
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestQueryStringBuilder(t *testing.T) {
+	t.Parallel()
+
+	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{}{
+				"float32":    float32(123.456),
+				"float64":    float64(123.456),
+				"float64int": float64(12345.0),
+				"int32":      32,
+				"int64":      int64(64),
+				"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",
+		},
+	}
+
+	for testName, testData := range tests {
+		testData := testData
+
+		t.Run(testName, func(t *testing.T) {
+			params := NewQueryStringBuilder()
+
+			for name, value := range testData.input {
+				switch value := value.(type) {
+				case string:
+					params.SetString(name, value)
+				case float32:
+					params.SetFloat32(name, value)
+				case float64:
+					params.SetFloat(name, value)
+				case int:
+					params.SetInt32(name, value)
+				case int64:
+					params.SetInt(name, value)
+				default:
+					require.Fail(t, fmt.Sprintf("Unknown data type for test fixture with name '%s'", name))
+				}
+			}
+
+			assert.Equal(t, testData.expectedEncoded, params.Encode())
+			assert.Equal(t, testData.expectedPath, params.EncodeWithPath("/test"))
+		})
+	}
+}