From 036eb508df7eba3835509858bec10983e5c2a27e Mon Sep 17 00:00:00 2001 From: George Robinson Date: Fri, 12 Apr 2024 10:40:14 +0200 Subject: [PATCH] Fix `Route.ID()` returns conflicting IDs (#3803) * Update TestRouteID tests This commit updates the TestRouteID tests to be more simple without reducing test coverage. It also adds new cases that show a bug in the existing code where conflicting IDs can be returned. Signed-off-by: George Robinson * Fix Route.ID() returns conflicting IDs This commit fixes a bug where Route.ID() returns conflicting IDs. For example, the configuration: receiver: test routes: - matchers: - foo=bar continue: true routes: - matchers: - bar=baz - matchers: - foo=bar continue: true routes: - matchers: - bar=baz Gives the following Route IDs: {} {}/{foo="bar"}/0 {}/{foo="bar"}/{bar="baz"}/0 {}/{foo="bar"}/1 {}/{foo="bar"}/{bar="baz"}/0 When it should give these Route IDs: {} {}/{foo="bar"}/0 {}/{foo="bar"}/0/{bar="baz"}/0 {}/{foo="bar"}/1 {}/{foo="bar"}/1/{bar="baz"}/0 Signed-off-by: George Robinson --------- Signed-off-by: George Robinson --- dispatch/route.go | 22 +++++---- dispatch/route_test.go | 107 ++++++++++++++++++++--------------------- 2 files changed, 65 insertions(+), 64 deletions(-) diff --git a/dispatch/route.go b/dispatch/route.go index 5ada178dab..e174672d3f 100644 --- a/dispatch/route.go +++ b/dispatch/route.go @@ -17,6 +17,7 @@ import ( "encoding/json" "fmt" "sort" + "strconv" "strings" "time" @@ -184,22 +185,23 @@ func (r *Route) Key() string { func (r *Route) ID() string { b := strings.Builder{} - position := -1 if r.parent != nil { - // Find the position in the same level leaf. - for i, cr := range r.parent.Routes { - if cr == r { - position = i + b.WriteString(r.parent.ID()) + b.WriteRune('/') + } + + b.WriteString(r.Matchers.String()) + + if r.parent != nil { + for i := range r.parent.Routes { + if r == r.parent.Routes[i] { + b.WriteRune('/') + b.WriteString(strconv.Itoa(i)) break } } } - b.WriteString(r.Key()) - if position > -1 { - b.WriteRune('/') - b.WriteString(fmt.Sprint(position)) - } return b.String() } diff --git a/dispatch/route_test.go b/dispatch/route_test.go index 40f1c41423..6a9d7d4588 100644 --- a/dispatch/route_test.go +++ b/dispatch/route_test.go @@ -856,68 +856,67 @@ routes: func TestRouteID(t *testing.T) { in := ` -receiver: 'notify-def' - +receiver: default routes: -- matchers: ['{owner="team-A"}', '{level!="critical"}'] - receiver: 'notify-D' - group_by: [...] - continue: true -- matchers: ['{owner="team-A"}', '{level!="critical"}'] - receiver: 'notify-A' +- continue: true + matchers: + - foo=bar + receiver: test1 routes: - - matchers: ['{env="testing"}', '{baz!~".*quux"}'] - receiver: 'notify-testing' - group_by: [...] - - match: - env: "production" - receiver: 'notify-productionA' - group_wait: 1m - continue: true - - matchers: [ env=~"produ.*", job=~".*"] - receiver: 'notify-productionB' - group_wait: 30s - group_interval: 5m - repeat_interval: 1h - group_by: ['job'] -- match_re: - owner: 'team-(B|C)' - group_by: ['foo', 'bar'] - group_wait: 2m - receiver: 'notify-BC' -- matchers: [group_by="role"] - group_by: ['role'] + - matchers: + - bar=baz +- continue: true + matchers: + - foo=bar + receiver: test1 routes: - - matchers: ['{env="testing"}'] - receiver: 'notify-testing' - routes: - - matchers: [wait="long"] - group_wait: 2m + - matchers: + - bar=baz +- continue: true + matchers: + - foo=bar + receiver: test2 + routes: + - matchers: + - bar=baz +- continue: true + matchers: + - bar=baz + receiver: test3 + routes: + - matchers: + - baz=qux + - matchers: + - qux=corge +- continue: true + matchers: + - qux=~"[a-zA-Z0-9]+" +- continue: true + matchers: + - corge!~"[0-9]+" ` - - var ctree config.Route - if err := yaml.UnmarshalStrict([]byte(in), &ctree); err != nil { - t.Fatal(err) - } - tree := NewRoute(&ctree, nil) + cr := config.Route{} + require.NoError(t, yaml.Unmarshal([]byte(in), &cr)) + r := NewRoute(&cr, nil) expected := []string{ "{}", - "{}/{level!=\"critical\",owner=\"team-A\"}/0", - "{}/{level!=\"critical\",owner=\"team-A\"}/1", - "{}/{level!=\"critical\",owner=\"team-A\"}/{baz!~\".*quux\",env=\"testing\"}/0", - "{}/{level!=\"critical\",owner=\"team-A\"}/{env=\"production\"}/1", - "{}/{level!=\"critical\",owner=\"team-A\"}/{env=~\"produ.*\",job=~\".*\"}/2", - "{}/{owner=~\"^(?:team-(B|C))$\"}/2", - "{}/{group_by=\"role\"}/3", - "{}/{group_by=\"role\"}/{env=\"testing\"}/0", - "{}/{group_by=\"role\"}/{env=\"testing\"}/{wait=\"long\"}/0", + "{}/{foo=\"bar\"}/0", + "{}/{foo=\"bar\"}/0/{bar=\"baz\"}/0", + "{}/{foo=\"bar\"}/1", + "{}/{foo=\"bar\"}/1/{bar=\"baz\"}/0", + "{}/{foo=\"bar\"}/2", + "{}/{foo=\"bar\"}/2/{bar=\"baz\"}/0", + "{}/{bar=\"baz\"}/3", + "{}/{bar=\"baz\"}/3/{baz=\"qux\"}/0", + "{}/{bar=\"baz\"}/3/{qux=\"corge\"}/1", + "{}/{qux=~\"[a-zA-Z0-9]+\"}/4", + "{}/{corge!~\"[0-9]+\"}/5", } - var got []string - tree.Walk(func(r *Route) { - got = append(got, r.ID()) + var actual []string + r.Walk(func(r *Route) { + actual = append(actual, r.ID()) }) - - require.ElementsMatch(t, got, expected) + require.ElementsMatch(t, actual, expected) }