[#150] policy: Check for redundant selectors and filters

Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>

netmap/policy.go

@@ -551,7 +551,7 @@ func (p *PlacementPolicy) DecodeString(s string) error {
 		return errors.New("parsed nil value")
 	}
 
-	if err := validatePolicy(*p); err != nil {
+	if err := validatePolicy(*parsed); err != nil {
 		return fmt.Errorf("invalid policy: %w", err)
 	}
 
@@ -605,6 +605,13 @@ var (
 	errUnknownSelector = errors.New("policy: selector not found")
 	// errSyntaxError is returned for errors found by ANTLR parser.
 	errSyntaxError = errors.New("policy: syntax error")
+	// errRedundantSelector is returned for errors found by selectors policy validator.
+	errRedundantSelector = errors.New("policy: found redundant selector")
+	// errUnnamedSelector is returned for errors found by selectors policy validator.
+	errUnnamedSelector = errors.New("policy: unnamed selectors are useless, " +
+		"make sure to pair REP and SELECT clauses: \"REP .. IN X\" + \"SELECT ... AS X\"")
+	// errRedundantSelector is returned for errors found by filters policy validator.
+	errRedundantFilter = errors.New("policy: found redundant filter")
 )
 
 type policyVisitor struct {
@@ -846,24 +853,50 @@ func (p *policyVisitor) VisitExpr(ctx *parser.ExprContext) any {
 // being actually defined in FILTER section.
 func validatePolicy(p PlacementPolicy) error {
 	seenFilters := map[string]bool{}
-
+	expectedFilters := map[string]struct{}{}
 	for i := range p.filters {
 		seenFilters[p.filters[i].GetName()] = true
-	}
-
-	seenSelectors := map[string]bool{}
-
-	for i := range p.selectors {
-		if flt := p.selectors[i].GetFilter(); flt != mainFilterName && !seenFilters[flt] {
-			return fmt.Errorf("%w: '%s'", errUnknownFilter, flt)
+		for _, f := range p.filters[i].GetFilters() {
+			if f.GetName() != "" {
+				expectedFilters[f.GetName()] = struct{}{}
+			}
 		}
-
-		seenSelectors[p.selectors[i].GetName()] = true
 	}
 
+	seenSelectors := map[string]*netmap.Selector{}
+	for i := range p.selectors {
+		if p.selectors[i].GetName() == "" {
+			return errUnnamedSelector
+		}
+		if flt := p.selectors[i].GetFilter(); flt != mainFilterName {
+			expectedFilters[flt] = struct{}{}
+			if !seenFilters[flt] {
+				return fmt.Errorf("%w: '%s'", errUnknownFilter, flt)
+			}
+		}
+		seenSelectors[p.selectors[i].GetName()] = &p.selectors[i]
+	}
+
+	for _, f := range p.filters {
+		if _, ok := expectedFilters[f.GetName()]; !ok {
+			return fmt.Errorf("%w: '%s'", errRedundantFilter, f.GetName())
+		}
+	}
+
+	expectedSelectors := map[string]struct{}{}
 	for i := range p.replicas {
-		if sel := p.replicas[i].GetSelector(); sel != "" && !seenSelectors[sel] {
-			return fmt.Errorf("%w: '%s'", errUnknownSelector, sel)
+		selName := p.replicas[i].GetSelector()
+		if selName != "" {
+			expectedSelectors[selName] = struct{}{}
+			if seenSelectors[selName] == nil {
+				return fmt.Errorf("%w: '%s'", errUnknownSelector, selName)
+			}
+		}
+	}
+
+	for _, s := range p.selectors {
+		if _, ok := expectedSelectors[s.GetName()]; !ok {
+			return fmt.Errorf("%w: to use selector '%s' use keyword IN", errRedundantSelector, s.GetName())
 		}
 	}
 

netmap/policy_decode_test.go

@@ -0,0 +1,61 @@
+package netmap
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestDecodeString(t *testing.T) {
+	testCases := []string{
+		`REP 1 IN X
+CBF 1
+SELECT 2 IN SAME Location FROM * AS X`,
+
+		`REP 1 IN X
+REP 2 IN Y
+CBF 1
+SELECT 2 FROM * AS X
+SELECT 3 FROM * AS Y`,
+
+		`REP 1 IN X
+SELECT 2 IN City FROM Good AS X
+FILTER Country EQ RU AS FromRU
+FILTER Country EQ EN AS FromEN
+FILTER @FromRU AND @FromEN AND Rating GT 7 AS Good`,
+
+		`REP 7 IN SPB
+SELECT 1 IN City FROM SPBSSD AS SPB
+FILTER City EQ SPB AND SSD EQ true OR City EQ SPB AND Rating GE 5 AS SPBSSD`,
+
+		`REP 7 IN SPB
+SELECT 1 IN City FROM SPBSSD AS SPB
+FILTER NOT (NOT (City EQ SPB) AND SSD EQ true OR City EQ SPB AND Rating GE 5) AS SPBSSD`,
+
+		`UNIQUE
+REP 1
+REP 1`,
+	}
+
+	var p PlacementPolicy
+
+	for _, testCase := range testCases {
+		require.NoError(t, p.DecodeString(testCase), "unable parse %s", testCase)
+		var b strings.Builder
+		require.NoError(t, p.WriteStringTo(&b))
+		require.Equal(t, testCase, b.String())
+	}
+
+	invalidTestCases := map[string]error{
+		`?REP 1`:                     errSyntaxError,
+		`REP 1 trailing garbage`:     errSyntaxError,
+		`REP 1 SELECT 4 FROM * `:     errUnnamedSelector,
+		`REP 1 SELECT 4 FROM * AS X`: errRedundantSelector,
+		`REP 1 IN X REP 2 SELECT 4 FROM * AS X FILTER 'UN-LOCODE' EQ 'RU LED' AS F`: errRedundantFilter,
+	}
+
+	for i := range invalidTestCases {
+		require.ErrorIs(t, p.DecodeString(i), invalidTestCases[i], "#%s", i)
+	}
+}

netmap/policy_test.go

@@ -1,7 +1,6 @@
 package netmap_test
 
 import (
-	"strings"
 	"testing"
 
 	. "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/netmap"
@@ -9,55 +8,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestEncode(t *testing.T) {
-	testCases := []string{
-		`REP 1 IN X
-CBF 1
-SELECT 2 IN SAME Location FROM * AS X`,
-
-		`REP 1
-SELECT 2 IN City FROM Good
-FILTER Country EQ RU AS FromRU
-FILTER @FromRU AND Rating GT 7 AS Good`,
-
-		`REP 7 IN SPB
-SELECT 1 IN City FROM SPBSSD AS SPB
-FILTER City EQ SPB AND SSD EQ true OR City EQ SPB AND Rating GE 5 AS SPBSSD`,
-
-		`REP 7 IN SPB
-SELECT 1 IN City FROM SPBSSD AS SPB
-FILTER NOT (NOT (City EQ SPB) AND SSD EQ true OR City EQ SPB AND Rating GE 5) AS SPBSSD`,
-
-		`UNIQUE
-REP 1
-REP 1`,
-
-		`REP 1 IN X
-SELECT 1 FROM F AS X
-FILTER 'UN-LOCODE' EQ 'RU LED' AS F`,
-	}
-
-	var p PlacementPolicy
-
-	for _, testCase := range testCases {
-		require.NoError(t, p.DecodeString(testCase))
-
-		var b strings.Builder
-		require.NoError(t, p.WriteStringTo(&b))
-
-		require.Equal(t, testCase, b.String())
-	}
-
-	invalidTestCases := []string{
-		`?REP 1`,
-		`REP 1 trailing garbage`,
-	}
-
-	for i := range invalidTestCases {
-		require.Error(t, p.DecodeString(invalidTestCases[i]), "#%d", i)
-	}
-}
-
 func TestPlacementPolicyEncoding(t *testing.T) {
 	v := netmaptest.PlacementPolicy()