Repository: VictoriaMetrics/VictoriaMetrics
Pull request #4955 (2023-09-05 10:05:35)

vmalert: correctly add duplicated params to the query
Fix the bug when Group's `params` fields with multiple values were overriding each other instead of adding up.
The bug was introduced in this commit https://github.com/VictoriaMetrics/VictoriaMetrics/commit/eccecdf177115297fa1dc4d42d38e23de9a9f2cb
 starting from v1.91.1 https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.91.1

 https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4908


-----------------

Once merged, should be backported to LTS releases.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4955?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) Report
Patch coverage: **`100.00%`** and no project coverage change.
> Comparison is base [(`9ddb2d8`)](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/commit/9ddb2d8010c373e5d0c79ab5b2499b0a05f44cac?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) 60.60% compared to head [(`d645246`)](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4955?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) 60.61%.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##           master    #4955   +/-   ##
=======================================
  Coverage   60.60%   60.61%           
=======================================
  Files         384      384           
  Lines       72377    72384    +7     
=======================================
+ Hits        43861    43872   +11     
+ Misses      26121    26120    -1     
+ Partials     2395     2392    -3     
```


| [Files Changed](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4955?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) | Coverage Δ | |
|---|---|---|
| [app/vmalert/datasource/vm.go](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4955?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-YXBwL3ZtYWxlcnQvZGF0YXNvdXJjZS92bS5nbw==) | `81.41% <100.00%> (+0.87%)` | :arrow_up: |

... and [5 files with indirect coverage changes](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4955/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4955?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).


Patch:
diff --git a/app/vmalert/datasource/vm.go b/app/vmalert/datasource/vm.go
index eb4c3fa49..ee21ff029 100644
--- a/app/vmalert/datasource/vm.go
+++ b/app/vmalert/datasource/vm.go
@@ -101,8 +101,15 @@ func (s *VMStorage) ApplyParams(params QuerierParams) *VMStorage {
 			s.extraParams = url.Values{}
 		}
 		for k, vl := range params.QueryParams {
-			for _, v := range vl { // custom query params are prior to default ones
-				s.extraParams.Set(k, v)
+			// custom query params are prior to default ones
+			if s.extraParams.Has(k) {
+				s.extraParams.Del(k)
+			}
+			for _, v := range vl {
+				// don't use .Set() instead of Del/Add since it is allowed
+				// for GET params to be duplicated
+				// see https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4908
+				s.extraParams.Add(k, v)
 			}
 		}
 	}
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 289deb142..c750dd196 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -51,6 +51,7 @@ ssue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4825) and [these
 * BUGFIX: [vminsert enterprise](https://docs.victoriametrics.com/enterprise.html): properly parse `/insert/multitenant/*` urls, which have been broken since [v1.93.2](#v1932). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4947).
 * BUGFIX: properly build production armv5 binaries for `GOARCH=arm`. This has been broken after the upgrading of Go builder to Go1.21.0. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4965).
 * BUGFIX: [vmselect](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html): return `503 Service Unavailable` status code when [partial responses](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html#cluster-availability) are denied and some of `vmstorage` nodes are temporarily unavailable. Previously `422 Unprocessable Entiry` status code was mistakenly returned in this case, which could prevent from automatic recovery by re-sending the request to healthy cluster replica in another availability zone.
+* BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert.html): fix the bug when Group's `params` fields with multiple values were overriding each other instead of adding up. The bug was introduced in [this commit](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/eccecdf177115297fa1dc4d42d38e23de9a9f2cb) starting from [v1.91.1](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.91.1). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4908).
 
 ## [v1.93.3](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.93.3)
 


Test patch:
diff --git a/app/vmalert/datasource/vm_test.go b/app/vmalert/datasource/vm_test.go
index 7b06f42db..aa2e79598 100644
--- a/app/vmalert/datasource/vm_test.go
+++ b/app/vmalert/datasource/vm_test.go
@@ -596,6 +596,17 @@ func TestRequestParams(t *testing.T) {
 				checkEqualString(t, exp.Encode(), r.URL.RawQuery)
 			},
 		},
+		{
+			"allow duplicates in query params",
+			false,
+			storage.Clone().ApplyParams(QuerierParams{
+				QueryParams: url.Values{"extra_labels": {"env=dev", "foo=bar"}},
+			}),
+			func(t *testing.T, r *http.Request) {
+				exp := url.Values{"query": {query}, "round_digits": {"10"}, "extra_labels": {"env=dev", "foo=bar"}, "time": {timestamp.Format(time.RFC3339)}}
+				checkEqualString(t, exp.Encode(), r.URL.RawQuery)
+			},
+		},
 		{
 			"graphite extra params",
 			false,