From 15984a641fd0ec99547d863c8424989709f26cc8 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Wed, 18 Nov 2020 10:06:21 +0200 Subject: [PATCH 1/2] [query] Fix /m3query returning 500 on invalid query argument --- .../dtest/docker/harness/query_api_test.go | 25 ++++++++++++++++--- .../handler/prometheus/native/read_common.go | 2 +- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/query_api_test.go b/src/cmd/tools/dtest/docker/harness/query_api_test.go index e7ab6ff4b0..bff0b4b15f 100644 --- a/src/cmd/tools/dtest/docker/harness/query_api_test.go +++ b/src/cmd/tools/dtest/docker/harness/query_api_test.go @@ -38,12 +38,14 @@ type urlTest struct { func TestInvalidInstantQueryReturns400(t *testing.T) { coord := singleDBNodeDockerResources.Coordinator() - instantQueryBadRequestTest := []urlTest{ + urlPrefix := []string{"", "prometheus/", "m3query/"} + + instantQueryBadRequestTest := addPrefixes(urlPrefix, []urlTest{ {"missing query", queryURL("query", "")}, {"invalid query", queryURL("query", "@!")}, {"invalid time", queryURL("time", "INVALID")}, {"invalid timeout", queryURL("timeout", "INVALID")}, - } + }) for _, tt := range instantQueryBadRequestTest { t.Run(tt.name, func(t *testing.T) { @@ -55,7 +57,9 @@ func TestInvalidInstantQueryReturns400(t *testing.T) { func TestInvalidRangeQueryReturns400(t *testing.T) { coord := singleDBNodeDockerResources.Coordinator() - queryBadRequestTest := []urlTest{ + urlPrefix := []string{"", "prometheus/", "m3query/"} + + queryBadRequestTest := addPrefixes(urlPrefix, []urlTest{ {"missing query", queryRangeURL("query", "")}, {"invalid query", queryRangeURL("query", "@!")}, {"missing start", queryRangeURL("start", "")}, @@ -65,7 +69,7 @@ func TestInvalidRangeQueryReturns400(t *testing.T) { {"missing step", queryRangeURL("step", "")}, {"invalid step", queryRangeURL("step", "INVALID")}, {"invalid timeout", queryRangeURL("timeout", "INVALID")}, - } + }) for _, tt := range queryBadRequestTest { t.Run(tt.name, func(t *testing.T) { @@ -74,6 +78,19 @@ func TestInvalidRangeQueryReturns400(t *testing.T) { } } +func addPrefixes(prefixes []string, tests []urlTest) []urlTest { + res := make([]urlTest, 0) + for _, prefix := range prefixes { + for _, t := range tests { + res = append(res, urlTest{ + fmt.Sprintf("%v %v", prefix, t.name), + fmt.Sprintf("%v%v", prefix, t.url), + }) + } + } + return res +} + func queryURL(key, value string) string { params := map[string]string{ "query": "foo", diff --git a/src/query/api/v1/handler/prometheus/native/read_common.go b/src/query/api/v1/handler/prometheus/native/read_common.go index f86ba1a304..b2c1760125 100644 --- a/src/query/api/v1/handler/prometheus/native/read_common.go +++ b/src/query/api/v1/handler/prometheus/native/read_common.go @@ -176,7 +176,7 @@ func read( parseOpts := engine.Options().ParseOptions() parser, err := promql.Parse(params.Query, params.Step, tagOpts, parseOpts) if err != nil { - return emptyResult, err + return emptyResult, xerrors.NewInvalidParamsError(err) } // Detect clients closing connections. From 661c9ae35af62bd81232983341df94f762e1f6c0 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Wed, 18 Nov 2020 11:07:15 +0200 Subject: [PATCH 2/2] PR comments --- .../dtest/docker/harness/query_api_test.go | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/query_api_test.go b/src/cmd/tools/dtest/docker/harness/query_api_test.go index bff0b4b15f..dd389afd66 100644 --- a/src/cmd/tools/dtest/docker/harness/query_api_test.go +++ b/src/cmd/tools/dtest/docker/harness/query_api_test.go @@ -36,30 +36,22 @@ type urlTest struct { } func TestInvalidInstantQueryReturns400(t *testing.T) { - coord := singleDBNodeDockerResources.Coordinator() - - urlPrefix := []string{"", "prometheus/", "m3query/"} + urlPrefixes := []string{"", "prometheus/", "m3query/"} - instantQueryBadRequestTest := addPrefixes(urlPrefix, []urlTest{ + urlTests := addPrefixes(urlPrefixes, []urlTest{ {"missing query", queryURL("query", "")}, {"invalid query", queryURL("query", "@!")}, {"invalid time", queryURL("time", "INVALID")}, {"invalid timeout", queryURL("timeout", "INVALID")}, }) - for _, tt := range instantQueryBadRequestTest { - t.Run(tt.name, func(t *testing.T) { - assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.url), "for query '%v'", tt.url) - }) - } + testInvalidQueryReturns400(t, urlTests) } func TestInvalidRangeQueryReturns400(t *testing.T) { - coord := singleDBNodeDockerResources.Coordinator() - - urlPrefix := []string{"", "prometheus/", "m3query/"} + urlPrefixes := []string{"", "prometheus/", "m3query/"} - queryBadRequestTest := addPrefixes(urlPrefix, []urlTest{ + urlTests := addPrefixes(urlPrefixes, []urlTest{ {"missing query", queryRangeURL("query", "")}, {"invalid query", queryRangeURL("query", "@!")}, {"missing start", queryRangeURL("start", "")}, @@ -71,7 +63,13 @@ func TestInvalidRangeQueryReturns400(t *testing.T) { {"invalid timeout", queryRangeURL("timeout", "INVALID")}, }) - for _, tt := range queryBadRequestTest { + testInvalidQueryReturns400(t, urlTests) +} + +func testInvalidQueryReturns400(t *testing.T, tests []urlTest) { + coord := singleDBNodeDockerResources.Coordinator() + + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.url), "for query '%v'", tt.url) })