Validate filter during snapshot construction

filters/tags_filter.go

@@ -256,3 +256,20 @@ func (f *tagsFilter) Matches(id []byte) bool {
 
 	return currIdx == len(f.tagFilters)
 }
+
+// ValidateTagsFilter validates whether a given string is a valid tags filter,
+// returning the filter values if the string is a valid tags filter expression,
+// and the error otherwise.
+func ValidateTagsFilter(str string) (TagFilterValueMap, error) {
+	filterValues, err := ParseTagFilterValueMap(str)
+	if err != nil {
+		return nil, fmt.Errorf("tags filter %s is malformed: %v", str, err)
+	}
+	for name, value := range filterValues {
+		// Validating the filter value by actually constructing the filter.
+		if _, err := NewFilterFromFilterValue(value); err != nil {
+			return nil, fmt.Errorf("tags filter %s contains invalid filter pattern %s for tag %s: %v", str, value.Pattern, name, err)
+		}
+	}
+	return filterValues, nil
+}

filters/tags_filter_test.go

@@ -23,6 +23,7 @@ package filters
 import (
 	"bytes"
 	"errors"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -187,6 +188,76 @@ func TestTagsFilterStringWithNameTag(t *testing.T) {
 	require.Equal(t, `name:Equals("foo") || tagName1:Equals("tagValue1") || tagName2:Equals("tagValue2")`, f.String())
 }
 
+func TestValidateTagsFilter(t *testing.T) {
+	inputs := []struct {
+		str      string
+		expected TagFilterValueMap
+	}{
+		{
+			str: "tagName1:tagValue1",
+			expected: TagFilterValueMap{
+				"tagName1": FilterValue{Pattern: "tagValue1", Negate: false},
+			},
+		},
+		{
+			str: "tagName1:tagValue1 tagName2:tagValue2*tagValue3",
+			expected: TagFilterValueMap{
+				"tagName1": FilterValue{Pattern: "tagValue1", Negate: false},
+				"tagName2": FilterValue{Pattern: "tagValue2*tagValue3", Negate: false},
+			},
+		},
+		{
+			str: "  tagName1:tagValue1?[0-9][!a-z]9    tagName2:{tagValue21,tagValue22}*   tagName3:tagValue3  tagName4:tagValue4",
+			expected: TagFilterValueMap{
+				"tagName1": FilterValue{Pattern: "tagValue1?[0-9][!a-z]9", Negate: false},
+				"tagName2": FilterValue{Pattern: "{tagValue21,tagValue22}*", Negate: false},
+				"tagName3": FilterValue{Pattern: "tagValue3", Negate: false},
+				"tagName4": FilterValue{Pattern: "tagValue4", Negate: false},
+			},
+		},
+	}
+
+	for _, input := range inputs {
+		res, err := ValidateTagsFilter(input.str)
+		require.NoError(t, err)
+		require.Equal(t, input.expected, res)
+	}
+}
+
+func TestValidateTagsFilterError(t *testing.T) {
+	inputs := []struct {
+		str string
+		err string
+	}{
+		{
+			str: "tagName1=tagValue1",
+			err: "tags filter tagName1=tagValue1 is malformed",
+		},
+		{
+			str: "tagName1:tagValue1 tagName2~=tagValue2",
+			err: "tags filter tagName1:tagValue1 tagName2~=tagValue2 is malformed",
+		},
+		{
+			str: "tagName1:tagValue1  tagName2:tagValue2 tagName1:tagValue3",
+			err: "tags filter tagName1:tagValue1  tagName2:tagValue2 tagName1:tagValue3 is malformed",
+		},
+		{
+			str: "tagName1:*too*many*",
+			err: "tags filter tagName1:*too*many* contains invalid filter pattern *too*many* for tag tagName1",
+		},
+		{
+			str: "tagName1:abcsdf tagName2:*con[tT]ains*",
+			err: "tags filter tagName1:abcsdf tagName2:*con[tT]ains* contains invalid filter pattern *con[tT]ains* for tag tagName2",
+		},
+	}
+
+	for _, input := range inputs {
+		_, err := ValidateTagsFilter(input.str)
+		require.Error(t, err)
+		require.True(t, strings.Contains(err.Error(), input.err))
+	}
+}
+
 func testTagsFilterOptions() TagsFilterOptions {
 	return TagsFilterOptions{
 		NameTagKey:          []byte("name"),

rules/mapping.go

@@ -70,7 +70,7 @@ func newMappingRuleSnapshot(
 		return nil, err
 	}
 
-	return newMappingRuleSnapshotFromFields(
+	return newMappingRuleSnapshotFromFieldsInternal(
 		r.Name,
 		r.Tombstoned,
 		r.CutoverNanos,
@@ -91,6 +91,33 @@ func newMappingRuleSnapshotFromFields(
 	filter filters.Filter,
 	lastUpdatedAtNanos int64,
 	lastUpdatedBy string,
+) (*mappingRuleSnapshot, error) {
+	if _, err := filters.ValidateTagsFilter(rawFilter); err != nil {
+		return nil, err
+	}
+	return newMappingRuleSnapshotFromFieldsInternal(
+		name,
+		tombstoned,
+		cutoverNanos,
+		rawFilter,
+		policies,
+		filter,
+		lastUpdatedAtNanos,
+		lastUpdatedBy,
+	), nil
+}
+
+// newMappingRuleSnapshotFromFieldsInternal creates a new mapping rule snapshot
+// from various given fields assuming the filter has already been validated.
+func newMappingRuleSnapshotFromFieldsInternal(
+	name string,
+	tombstoned bool,
+	cutoverNanos int64,
+	rawFilter string,
+	policies []policy.Policy,
+	filter filters.Filter,
+	lastUpdatedAtNanos int64,
+	lastUpdatedBy string,
 ) *mappingRuleSnapshot {
 	return &mappingRuleSnapshot{
 		name:               name,
@@ -251,7 +278,7 @@ func (mc *mappingRule) addSnapshot(
 	policies []policy.Policy,
 	meta UpdateMetadata,
 ) error {
-	snapshot := newMappingRuleSnapshotFromFields(
+	snapshot, err := newMappingRuleSnapshotFromFields(
 		name,
 		false,
 		meta.cutoverNanos,
@@ -261,6 +288,9 @@ func (mc *mappingRule) addSnapshot(
 		meta.updatedAtNanos,
 		meta.updatedBy,
 	)
+	if err != nil {
+		return err
+	}
 	mc.snapshots = append(mc.snapshots, snapshot)
 	return nil
 }

rules/rollup.go

@@ -191,7 +191,7 @@ func newRollupRuleSnapshot(
 		return nil, err
 	}
 
-	return newRollupRuleSnapshotFromFields(
+	return newRollupRuleSnapshotFromFieldsInternal(
 		r.Name,
 		r.Tombstoned,
 		r.CutoverNanos,
@@ -207,7 +207,34 @@ func newRollupRuleSnapshotFromFields(
 	name string,
 	tombstoned bool,
 	cutoverNanos int64,
-	rawfilter string,
+	rawFilter string,
+	targets []RollupTarget,
+	filter filters.Filter,
+	lastUpdatedAtNanos int64,
+	lastUpdatedBy string,
+) (*rollupRuleSnapshot, error) {
+	if _, err := filters.ValidateTagsFilter(rawFilter); err != nil {
+		return nil, err
+	}
+	return newRollupRuleSnapshotFromFieldsInternal(
+		name,
+		tombstoned,
+		cutoverNanos,
+		rawFilter,
+		targets,
+		filter,
+		lastUpdatedAtNanos,
+		lastUpdatedBy,
+	), nil
+}
+
+// newRollupRuleSnapshotFromFieldsInternal creates a new rollup rule snapshot
+// from various given fields assuming the filter has already been validated.
+func newRollupRuleSnapshotFromFieldsInternal(
+	name string,
+	tombstoned bool,
+	cutoverNanos int64,
+	rawFilter string,
 	targets []RollupTarget,
 	filter filters.Filter,
 	lastUpdatedAtNanos int64,
@@ -219,7 +246,7 @@ func newRollupRuleSnapshotFromFields(
 		cutoverNanos:       cutoverNanos,
 		filter:             filter,
 		targets:            targets,
-		rawFilter:          rawfilter,
+		rawFilter:          rawFilter,
 		lastUpdatedAtNanos: lastUpdatedAtNanos,
 		lastUpdatedBy:      lastUpdatedBy,
 	}
@@ -410,7 +437,7 @@ func (rc *rollupRule) addSnapshot(
 	rollupTargets []RollupTarget,
 	meta UpdateMetadata,
 ) error {
-	snapshot := newRollupRuleSnapshotFromFields(
+	snapshot, err := newRollupRuleSnapshotFromFields(
 		name,
 		false,
 		meta.cutoverNanos,
@@ -420,6 +447,9 @@ func (rc *rollupRule) addSnapshot(
 		meta.updatedAtNanos,
 		meta.updatedBy,
 	)
+	if err != nil {
+		return err
+	}
 	rc.snapshots = append(rc.snapshots, snapshot)
 	return nil
 }

rules/validator/validator.go

@@ -171,20 +171,15 @@ func (v *validator) validateRollupRules(rrv map[string]*rules.RollupRuleView) er
 }
 
 func (v *validator) validateFilter(ruleName string, f string) (filters.TagFilterValueMap, error) {
-	filterValues, err := filters.ParseTagFilterValueMap(f)
+	filterValues, err := filters.ValidateTagsFilter(f)
 	if err != nil {
 		return nil, fmt.Errorf("rule %s has invalid rule filter %s: %v", ruleName, f, err)
 	}
-	for tag, filterValue := range filterValues {
+	for tag := range filterValues {
 		// Validating the filter tag name does not contain invalid chars.
 		if err := v.opts.CheckInvalidCharactersForTagName(tag); err != nil {
 			return nil, fmt.Errorf("rule %s has invalid rule filter %s: tag name %s contains invalid character, err: %v", ruleName, f, tag, err)
 		}
-
-		// Validating the filter expression by actually constructing the filter.
-		if _, err := filters.NewFilterFromFilterValue(filterValue); err != nil {
-			return nil, fmt.Errorf("rule %s has invalid rule filter %s: filter pattern for tag %s is invalid, err: %v", ruleName, f, tag, err)
-		}
 	}
 	return filterValues, nil
 }