fix(notificationrules): Correct logic on matching notification rules (#…

…16328)

* fix(notificationrules): Correct logic on matching notification rules

* feat(nrs): Add more tests

http/swagger.yml

@@ -5580,7 +5580,7 @@ paths:
             type: string
         - in: query
           name: tag
-          description: Only show notification rules that match a tag pair. Uses `AND` to specify multiple tags.
+          description: Only return notification rules that "would match" statuses which contain the tag key value pairs provided.
           schema:
             type: string
             pattern: ^[a-zA-Z0-9_]+:[a-zA-Z0-9_]+$

kv/notification_rule.go

@@ -440,10 +440,8 @@ func filterNotificationRulesFn(
 	filter influxdb.NotificationRuleFilter) func(nr influxdb.NotificationRule) bool {
 	if filter.OrgID != nil {
 		return func(nr influxdb.NotificationRule) bool {
-			for _, ft := range filter.Tags {
-				if !nr.HasTag(ft.Key, ft.Value) {
-					return false
-				}
+			if !nr.MatchesTags(filter.Tags) {
+				return false
 			}
 
 			_, ok := idMap[nr.GetID()]
@@ -452,10 +450,8 @@ func filterNotificationRulesFn(
 	}
 
 	return func(nr influxdb.NotificationRule) bool {
-		for _, ft := range filter.Tags {
-			if !nr.HasTag(ft.Key, ft.Value) {
-				return false
-			}
+		if !nr.MatchesTags(filter.Tags) {
+			return false
 		}
 
 		_, ok := idMap[nr.GetID()]

notification.go

@@ -30,7 +30,7 @@ type NotificationRule interface {
 	GetEndpointID() ID
 	GetLimit() *Limit
 	GenerateFlux(NotificationEndpoint) (string, error)
-	HasTag(key, value string) bool
+	MatchesTags(tags []Tag) bool
 }
 
 // NotificationRuleStore represents a service for managing notification rule.

notification/rule/rule.go

@@ -335,15 +335,34 @@ func (b *Base) ClearPrivateData() {
 	b.TaskID = 0
 }
 
-// HasTag returns true if the Rule has a matching tagRule
-func (b *Base) HasTag(key, value string) bool {
-	for _, tr := range b.TagRules {
-		if tr.Operator == influxdb.Equal && tr.Key == key && tr.Value == value {
-			return true
+// MatchesTags returns true if the Rule matches all of the tags
+func (b *Base) MatchesTags(tags []influxdb.Tag) bool {
+
+	// for each tag in NR
+	// if there exists
+	// a key value match with operator == equal in tags
+	// or
+	// a key match with a value mismatch with operator == notequal in tags
+	// then true
+
+	for _, NRtag := range b.TagRules {
+		isNRTagInFilterTags := false
+
+		for _, filterTag := range tags {
+			if NRtag.Key == filterTag.Key {
+				if NRtag.Operator == influxdb.Equal && NRtag.Value == filterTag.Value {
+					isNRTagInFilterTags = true
+				}
+				if NRtag.Operator == influxdb.NotEqual && NRtag.Value != filterTag.Value {
+					isNRTagInFilterTags = true
+				}
+			}
+		}
+		if !isNRTagInFilterTags {
+			return false
 		}
 	}
-
-	return false
+	return true
 }
 
 // GetOwnerID returns the owner id.

notification/rule/rule_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/influxdata/influxdb/mock"
+	"github.com/influxdata/influxdb/pkg/testing/assert"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/influxdata/influxdb/notification"
@@ -337,3 +338,156 @@ func TestJSON(t *testing.T) {
 		}
 	}
 }
+
+func TestMatchingRules(t *testing.T) {
+	cases := []struct {
+		name       string
+		tagRules   []notification.TagRule
+		filterTags []influxdb.Tag
+		exp        bool
+	}{
+		{
+			name: "Matches when tagrules and filterTags are the same. ",
+			tagRules: []notification.TagRule{
+				{
+					Tag: influxdb.Tag{
+						Key:   "a",
+						Value: "b",
+					},
+					Operator: influxdb.Equal,
+				},
+				{
+					Tag: influxdb.Tag{
+						Key:   "c",
+						Value: "d",
+					},
+					Operator: influxdb.Equal,
+				},
+			},
+			filterTags: []influxdb.Tag{
+				{Key: "a", Value: "b"},
+				{Key: "c", Value: "d"},
+			},
+			exp: true,
+		},
+		{
+			name: "Matches when tagrules are subset of filterTags. ",
+			tagRules: []notification.TagRule{
+				{
+					Tag: influxdb.Tag{
+						Key:   "a",
+						Value: "b",
+					},
+					Operator: influxdb.Equal,
+				},
+				{
+					Tag: influxdb.Tag{
+						Key:   "c",
+						Value: "d",
+					},
+					Operator: influxdb.Equal,
+				},
+			},
+			filterTags: []influxdb.Tag{
+				{Key: "a", Value: "b"},
+				{Key: "c", Value: "d"},
+				{Key: "e", Value: "f"},
+			},
+			exp: true,
+		},
+		{
+			name: "Does not match when filterTags are missing tags that are in tag rules.",
+			tagRules: []notification.TagRule{
+				{
+					Tag: influxdb.Tag{
+						Key:   "a",
+						Value: "b",
+					},
+					Operator: influxdb.Equal,
+				},
+				{
+					Tag: influxdb.Tag{
+						Key:   "c",
+						Value: "d",
+					},
+					Operator: influxdb.Equal,
+				},
+			},
+			filterTags: []influxdb.Tag{
+				{Key: "a", Value: "b"},
+			},
+			exp: false,
+		},
+		{
+			name: "Does not match when tagrule has key value pair that does not match value of same key in filterTags",
+			tagRules: []notification.TagRule{
+				{
+					Tag: influxdb.Tag{
+						Key:   "a",
+						Value: "b",
+					},
+					Operator: influxdb.Equal,
+				},
+				{
+					Tag: influxdb.Tag{
+						Key:   "c",
+						Value: "d",
+					},
+					Operator: influxdb.Equal,
+				},
+			},
+			filterTags: []influxdb.Tag{
+				{Key: "a", Value: "b"},
+				{Key: "c", Value: "X"},
+			},
+			exp: false,
+		},
+		{
+			name: "Match when tagrule has key value pair that does not match value of same key in filterTags, if tagrule has notEqual operator",
+			tagRules: []notification.TagRule{
+				{
+					Tag: influxdb.Tag{
+						Key:   "a",
+						Value: "b",
+					},
+					Operator: influxdb.Equal,
+				},
+				{
+					Tag: influxdb.Tag{
+						Key:   "c",
+						Value: "d",
+					},
+					Operator: influxdb.NotEqual,
+				},
+			},
+			filterTags: []influxdb.Tag{
+				{Key: "a", Value: "b"},
+				{Key: "c", Value: "X"},
+			},
+			exp: true,
+		},
+		{
+			name:     "Empty tag rule matches filterTags",
+			tagRules: []notification.TagRule{},
+			filterTags: []influxdb.Tag{
+				{Key: "a", Value: "b"},
+				{Key: "c", Value: "X"},
+			},
+			exp: true,
+		},
+		{
+			name:       "Empty tag rule matches empty filter tags",
+			tagRules:   []notification.TagRule{},
+			filterTags: []influxdb.Tag{},
+			exp:        true,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+
+			r := rule.Base{TagRules: c.tagRules}
+
+			assert.Equal(t, r.MatchesTags(c.filterTags), c.exp, "expected NR tags to be subset of filterTags")
+		})
+	}
+}