Alerting: Return better error for invalid time range on alert queries (#85611)

* Return better error for invalid time range

* drop comment

pkg/services/ngalert/api/api_provisioning.go

@@ -488,10 +488,10 @@ func (srv *ProvisioningSrv) RoutePutAlertRuleGroup(c *contextmodel.ReqContext, a
 	if errors.Is(err, alerting_models.ErrAlertRuleFailedValidation) {
 		return ErrResp(http.StatusBadRequest, err, "")
 	}
+	if errors.Is(err, store.ErrOptimisticLock) {
+		return ErrResp(http.StatusConflict, err, "")
+	}
 	if err != nil {
-		if errors.Is(err, store.ErrOptimisticLock) {
-			return ErrResp(http.StatusConflict, err, "")
-		}
 		return response.ErrOrFallback(http.StatusInternalServerError, "", err)
 	}
 	return response.JSON(http.StatusOK, ag)

pkg/services/ngalert/models/alert_query.go

@@ -303,7 +303,7 @@ func (aq *AlertQuery) PreSave() error {
 	}
 
 	if ok := isExpression || aq.RelativeTimeRange.isValid(); !ok {
-		return fmt.Errorf("invalid relative time range: %+v", aq.RelativeTimeRange)
+		return ErrInvalidRelativeTimeRange(aq.RefID, aq.RelativeTimeRange)
 	}
 	return nil
 }

pkg/services/ngalert/models/alert_query_test.go

@@ -326,6 +326,53 @@ func TestAlertQueryMarshalling(t *testing.T) {
 	}
 }
 
+func TestAlertQuery_PreSave(t *testing.T) {
+	testCases := []struct {
+		desc        string
+		blob        string
+		errContains string
+	}{
+		{
+			desc: "no error when input is correct",
+			blob: `{
+				"refId": "B",
+				"relativeTimeRange": {
+					"from": 2000,
+					"to": 1000
+				},
+				"model": {}
+			}`,
+		},
+		{
+			desc: "expected error when range is incorrect",
+			blob: `{
+				"refId": "B",
+				"relativeTimeRange": {
+					"from": 1000,
+					"to": 1000
+				},
+				"model": {}
+			}`,
+			errContains: "Invalid alert rule query B: invalid relative time range [From: 16m40s, To: 16m40s]",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			var aq AlertQuery
+			err := json.Unmarshal([]byte(tc.blob), &aq)
+			require.NoError(t, err)
+
+			err = aq.PreSave()
+			if tc.errContains == "" {
+				require.NoError(t, err)
+			} else {
+				require.ErrorContains(t, err, tc.errContains)
+			}
+		})
+	}
+}
+
 func TestAlertQuery_GetQuery(t *testing.T) {
 	tc := []struct {
 		name       string

pkg/services/ngalert/models/errors.go

@@ -8,9 +8,14 @@ var (
 	errAlertRuleConflictMsg  = "conflicting alert rule found [rule_uid: '{{ .Public.RuleUID }}', title: '{{ .Public.Title }}', namespace_uid: '{{ .Public.NamespaceUID }}']: {{ .Public.Error }}"
 	ErrAlertRuleConflictBase = errutil.Conflict("alerting.alert-rule.conflict").
 					MustTemplate(errAlertRuleConflictMsg, errutil.WithPublic(errAlertRuleConflictMsg))
-	ErrAlertRuleGroupNotFound = errutil.NotFound("alerting.alert-rule.notFound")
+	ErrAlertRuleGroupNotFound       = errutil.NotFound("alerting.alert-rule.notFound")
+	ErrInvalidRelativeTimeRangeBase = errutil.BadRequest("alerting.alert-rule.invalidRelativeTime").MustTemplate("Invalid alert rule query {{ .Public.RefID }}: invalid relative time range [From: {{ .Public.From }}, To: {{ .Public.To }}]")
 )
 
 func ErrAlertRuleConflict(rule AlertRule, underlying error) error {
 	return ErrAlertRuleConflictBase.Build(errutil.TemplateData{Public: map[string]any{"RuleUID": rule.UID, "Title": rule.Title, "NamespaceUID": rule.NamespaceUID, "Error": underlying.Error()}, Error: underlying})
 }
+
+func ErrInvalidRelativeTimeRange(refID string, rtr RelativeTimeRange) error {
+	return ErrInvalidRelativeTimeRangeBase.Build(errutil.TemplateData{Public: map[string]any{"RefID": refID, "From": rtr.From, "To": rtr.To}})
+}