experimental: change time-range behavior (#1428)

* Revert "experimental: DataQuery: switch from timeRange to _timeRange (#1420)"

This reverts commit 64d18ac.

* remove time range from backend.DataQuery.JSON

* added explanation comment

Author: gabor

experimental/apis/data/v0alpha1/conversions.go

@@ -45,6 +45,36 @@ func ToDataSourceQueries(req QueryDataRequest) ([]backend.DataQuery, *DataSource
 	return queries, dsRef, nil
 }
 
+// when the time range is specified locally, per query,
+// it will appear in the JSON field of backend.DataQuery.
+// this can cause problems in certain data source plugins,
+// that expect that the `timeRange` field does not exist.
+// see https://github.com/grafana/grafana-plugin-sdk-go/issues/1419
+// for more info.
+// to improve compatibility, we remove it from the json bytes
+// NOTE: it will still be available in the `.TimeRange` field
+// of `backend.DataQuery`.
+var timeRangeKey = "timeRange"
+
+func deleteTimeRangeFromQueryJSON(data []byte) ([]byte, error) {
+	var d map[string]any
+	err := json.Unmarshal(data, &d)
+	if err != nil {
+		return nil, err
+	}
+
+	_, found := d[timeRangeKey]
+
+	if !found {
+		// we can finish here, return the original
+		return data, nil
+	}
+
+	delete(d, timeRangeKey)
+
+	return json.Marshal(d)
+}
+
 // Converts a generic query to a backend one
 func toBackendDataQuery(q DataQuery, defaultTimeRange *backend.TimeRange) (backend.DataQuery, error) {
 	var err error
@@ -65,10 +95,31 @@ func toBackendDataQuery(q DataQuery, defaultTimeRange *backend.TimeRange) (backe
 		bq.TimeRange = *defaultTimeRange
 	}
 
-	bq.JSON, err = json.Marshal(q)
+	bytes, err := json.Marshal(q)
+	if err != nil {
+		return bq, err
+	}
+
+	// we understand there is a certain inefficiency here
+	// with re-marshaling the bytes, we chose this approach
+	// for the following reasons:
+	// 1. the smallest possible change
+	// 2. the request object is small, so the performance
+	//    impact should be limited
+	// 3. we can implement faster solutions later,
+	//    as a purely internal change here, without
+	//    affecting anything.
+	//
+	// the alternative approach would be to directly
+	// serialise the v0alpha1.DataQuery structure into JSON
+	// without the timeRange field.
+	fixedBytes, err := deleteTimeRangeFromQueryJSON(bytes)
 	if err != nil {
 		return bq, err
 	}
+
+	bq.JSON = fixedBytes
+
 	if bq.MaxDataPoints == 0 {
 		bq.MaxDataPoints = 100
 	}

experimental/apis/data/v0alpha1/conversions_test.go

@@ -56,14 +56,13 @@ func TestToBackendDataQueryJSON(t *testing.T) {
 	require.Equal(t, time.UnixMilli(87654321).UTC(), bq.TimeRange.To)
 
 	jsonData := `{` +
-		`"refId":"A",` +
-		`"_timeRange":{"from":"12345678","to":"87654321"},` +
 		`"datasource":{"type":"prometheus","uid":"hello-world"},` +
-		`"queryType":"interesting",` +
-		`"maxDataPoints":42,` +
 		`"intervalMs":15,` +
 		`"key1":"value1",` +
-		`"key2":"value2"` +
+		`"key2":"value2",` +
+		`"maxDataPoints":42,` +
+		`"queryType":"interesting",` +
+		`"refId":"A"` +
 		`}`
 
 	require.Equal(t, jsonData, string(bq.JSON))
@@ -126,10 +125,48 @@ func TestToDataSourceQueriesTimeRangeHandling(t *testing.T) {
 	require.Equal(t, time.UnixMilli(1763114120000).UTC(), b.TimeRange.From)
 	require.Equal(t, time.UnixMilli(1763114130000).UTC(), b.TimeRange.To)
 	jsonB := `{` +
-		`"refId":"B",` +
-		`"_timeRange":{"from":"1763114120000","to":"1763114130000"},` +
 		`"datasource":{"type":"prometheus","uid":"prom1"},` +
-		`"expr":"222"` +
+		`"expr":"222",` +
+		`"refId":"B"` +
 		`}`
 	require.Equal(t, jsonB, string(b.JSON))
 }
+
+func TestDeleteTimeRangeFromQueryJSON(t *testing.T) {
+	tests := []struct {
+		name          string
+		data          []byte
+		expected      []byte
+		expectedError bool
+	}{
+		{
+			name:          "invalid json",
+			data:          []byte("hello world"),
+			expectedError: true,
+		},
+		{
+			name:          "with time range",
+			data:          []byte(`{"f1":{"f2":42},"timeRange":{"from":"111","to":"222"},"f3":"v3"}`),
+			expected:      []byte(`{"f1":{"f2":42},"f3":"v3"}`),
+			expectedError: false,
+		},
+		{
+			name:          "without time range",
+			data:          []byte(`{"f1":{"f2":42},"f3":"v3"}`),
+			expected:      []byte(`{"f1":{"f2":42},"f3":"v3"}`),
+			expectedError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := deleteTimeRangeFromQueryJSON(tt.data)
+			if tt.expectedError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expected, result)
+			}
+		})
+	}
+}

experimental/apis/data/v0alpha1/query.go

@@ -269,7 +269,7 @@ func writeQuery(g *DataQuery, stream *j.Stream) {
 
 	if q.TimeRange != nil {
 		stream.WriteMore()
-		stream.WriteObjectField("_timeRange")
+		stream.WriteObjectField("timeRange")
 		stream.WriteVal(g.TimeRange)
 	}
 
@@ -347,9 +347,7 @@ func (g *CommonQueryProperties) readQuery(iter *jsoniter.Iterator,
 			g.RefID, err = iter.ReadString()
 		case "resultAssertions":
 			err = iter.ReadVal(&g.ResultAssertions)
-		case "timeRange": // temporarily kept for backward compatibility
-			err = iter.ReadVal(&g.TimeRange)
-		case "_timeRange":
+		case "timeRange":
 			err = iter.ReadVal(&g.TimeRange)
 		case "datasource":
 			// Old datasource values may just be a string

experimental/apis/data/v0alpha1/query_test.go

@@ -56,18 +56,6 @@ func TestParseQueriesIntoQueryDataRequest(t *testing.T) {
 					"from": "100",
 					"to": "200"
 				}
-			},
-			{
-				"refId": "Q",
-				"datasource": {
-					"type": "prometheus",
-					"uid": "u-i-d"
-				},
-				"expr": "42",
-				"_timeRange": {
-					"from": "10",
-					"to": "20"
-				}
 			}
 		],
 		"from": "1692624667389",
@@ -79,7 +67,7 @@ func TestParseQueriesIntoQueryDataRequest(t *testing.T) {
 	require.NoError(t, err)
 
 	t.Run("verify raw unmarshal", func(t *testing.T) {
-		require.Len(t, req.Queries, 3)
+		require.Len(t, req.Queries, 2)
 		require.Equal(t, "b1808c48-9fc9-4045-82d7-081781f8a553", req.Queries[0].Datasource.UID)
 		require.Equal(t, "spreadsheetID", req.Queries[0].GetString("spreadsheet"))
 
@@ -96,42 +84,21 @@ func TestParseQueriesIntoQueryDataRequest(t *testing.T) {
 		require.Equal(t, int64(10), req.Queries[1].MaxDataPoints) // input was a string
 
 		// The second query has an explicit time range, and legacy datasource name
-		require.NotNil(t, req.Queries[1].TimeRange)
-		require.Equal(t, "100", req.Queries[1].TimeRange.From)
-		require.Equal(t, "200", req.Queries[1].TimeRange.To)
 		out, err = json.MarshalIndent(req.Queries[1], "", "  ")
 		require.NoError(t, err)
+		// fmt.Printf("%s\n", string(out))
 		require.JSONEq(t, `{
 			"datasource": {
 			  "type": "", ` /* NOTE! this implies legacy naming */ +`
 			  "uid": "old"
 			},
 			"maxDataPoints": 10,
 			"refId": "Z",
-			"_timeRange": {
+			"timeRange": {
 			  "from": "100",
 			  "to": "200"
 			}
 		  }`, string(out))
-
-		// The third query has a time-range with an underscore prefix
-		require.NotNil(t, req.Queries[2].TimeRange)
-		require.Equal(t, "10", req.Queries[2].TimeRange.From)
-		require.Equal(t, "20", req.Queries[2].TimeRange.To)
-		out, err = json.MarshalIndent(req.Queries[2], "", "  ")
-		require.NoError(t, err)
-		require.JSONEq(t, `{
-			"datasource": {
-			  "type": "prometheus",
-			  "uid": "u-i-d"
-			},
-			"expr": "42",
-			"refId": "Q",
-			"_timeRange": {
-			  "from": "10",
-			  "to": "20"
-			}
-		  }`, string(out))
 	})
 
 	t.Run("verify deep copy", func(t *testing.T) {