handle parsing error

Signed-off-by: Zhang Xin <[email protected]>
jaegertracing · Dec 20, 2024 · d8fc5f9 · d8fc5f9
1 parent a35d464
commit d8fc5f9
Show file tree

Hide file tree

Showing 3 changed files with 100 additions and 12 deletions.
diff --git a/cmd/query/app/handler_archive_test.go b/cmd/query/app/handler_archive_test.go
@@ -126,3 +126,34 @@ func TestArchiveTrace_WriteErrors(t *testing.T) {
 		require.EqualError(t, err, `500 error from server: {"data":null,"total":0,"limit":0,"offset":0,"errors":[{"code":500,"msg":"cannot save\ncannot save"}]}`+"\n")
 	}, querysvc.QueryServiceOptions{ArchiveSpanWriter: mockWriter})
 }
+
+func TestArchiveTrace_BadTimeWindow(t *testing.T) {
+	testCases := []struct {
+		name  string
+		query string
+	}{
+		{
+			name:  "Bad start time",
+			query: "start=a",
+		},
+		{
+			name:  "Bad end time",
+			query: "end=b",
+		},
+	}
+	mockWriter := &spanstoremocks.Writer{}
+	mockWriter.On("WriteSpan", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*model.Span")).
+		Return(nil).Times(2 * len(testCases))
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			withTestServer(t, func(ts *testServer) {
+				// make main reader return NotFound
+				ts.spanReader.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("spanstore.GetTraceParameters")).
+					Return(mockTrace, nil).Once()
+				var response structuredTraceResponse
+				err := postJSON(ts.server.URL+"/api/archive/"+mockTraceID.String()+"?"+tc.query, []string{}, &response)
+				require.Error(t, err)
+			}, querysvc.QueryServiceOptions{ArchiveSpanWriter: mockWriter})
+		})
+	}
+}
diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go
@@ -442,16 +442,20 @@ func (aH *APIHandler) getTrace(w http.ResponseWriter, r *http.Request) {
 	query := spanstore.GetTraceParameters{
 		TraceID: traceID,
 	}
-	startTime := r.FormValue(startTimeParam)
-	if startTime != "" {
-		// No need to handle err here, since time window is optional
-		t, _ := aH.queryParser.parseTime(r, startTimeParam, time.Microsecond)
+
+	if startTime := r.FormValue(startTimeParam); startTime != "" {
+		t, err := aH.queryParser.parseTime(r, startTimeParam, time.Microsecond)
+		if aH.handleError(w, err, http.StatusBadRequest) {
+			return
+		}
 		query.StartTime = t
 	}
-	endTime := r.FormValue(endTimeParam)
-	if endTime != "" {
-		// No need to handle err here, since time window is optional
-		t, _ := aH.queryParser.parseTime(r, endTimeParam, time.Microsecond)
+
+	if endTime := r.FormValue(endTimeParam); endTime != "" {
+		t, err := aH.queryParser.parseTime(r, endTimeParam, time.Microsecond)
+		if aH.handleError(w, err, http.StatusBadRequest) {
+			return
+		}
 		query.EndTime = t
 	}
 	trc, err := aH.queryService.GetTrace(r.Context(), query)
@@ -483,15 +487,20 @@ func (aH *APIHandler) archiveTrace(w http.ResponseWriter, r *http.Request) {
 	}
 
 	// QueryService.ArchiveTrace can now archive this traceID.
-	// No need to handle err here, since time window is optional
-	startTime, _ := aH.queryParser.parseTime(r, startTimeParam, time.Microsecond)
-	endTime, _ := aH.queryParser.parseTime(r, endTimeParam, time.Microsecond)
+	startTime, err := aH.queryParser.parseTime(r, startTimeParam, time.Microsecond)
+	if aH.handleError(w, err, http.StatusBadRequest) {
+		return
+	}
+	endTime, err := aH.queryParser.parseTime(r, endTimeParam, time.Microsecond)
+	if aH.handleError(w, err, http.StatusBadRequest) {
+		return
+	}
 	query := spanstore.GetTraceParameters{
 		TraceID:   traceID,
 		StartTime: startTime,
 		EndTime:   endTime,
 	}
-	err := aH.queryService.ArchiveTrace(r.Context(), query)
+	err = aH.queryService.ArchiveTrace(r.Context(), query)
 	if errors.Is(err, spanstore.ErrTraceNotFound) {
 		aH.handleError(w, err, http.StatusNotFound)
 		return

diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go
@@ -404,6 +404,30 @@ func TestGetTraceBadTraceID(t *testing.T) {
 	require.Error(t, err)
 }
 
+func TestGetTraceBadTimeWindow(t *testing.T) {
+	testCases := []struct {
+		name  string
+		query string
+	}{
+		{
+			name:  "Bad start time",
+			query: "start=a",
+		},
+		{
+			name:  "Bad end time",
+			query: "end=b",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ts := initializeTestServer(t)
+			var response structuredResponse
+			err := getJSON(ts.server.URL+`/api/traces/123456?`+tc.query, &response)
+			require.Error(t, err)
+		})
+	}
+}
+
 func TestSearchSuccess(t *testing.T) {
 	ts := initializeTestServer(t)
 	ts.spanReader.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")).
@@ -445,6 +469,30 @@ func TestSearchByTraceIDWithTimeWindowSuccess(t *testing.T) {
 	assert.Len(t, response.Data, 1)
 }
 
+func TestSearchTraceBadTimeWindow(t *testing.T) {
+	testCases := []struct {
+		name  string
+		query string
+	}{
+		{
+			name:  "Bad start time",
+			query: "start=a",
+		},
+		{
+			name:  "Bad end time",
+			query: "end=b",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ts := initializeTestServer(t)
+			var response structuredResponse
+			err := getJSON(ts.server.URL+`/api/traces?traceID=1&`+tc.query, &response)
+			require.Error(t, err)
+		})
+	}
+}
+
 func TestSearchByTraceIDSuccessWithArchive(t *testing.T) {
 	archiveReadMock := &spanstoremocks.Reader{}
 	ts := initializeTestServerWithOptions(t, &tenancy.Manager{}, querysvc.QueryServiceOptions{