From b5fe52d6bd2dccae43dc6cdd5d2e0d998e3dd9fb Mon Sep 17 00:00:00 2001 From: Anton Nikiforov Date: Tue, 29 Aug 2023 08:41:59 +0300 Subject: [PATCH] [#150] policy: Check for redundant selectors and filters Signed-off-by: Anton Nikiforov --- netmap/policy.go | 59 ++++++++++++++++++++++++++-------- netmap/policy_decode_test.go | 61 ++++++++++++++++++++++++++++++++++++ netmap/policy_test.go | 50 ----------------------------- 3 files changed, 107 insertions(+), 63 deletions(-) create mode 100644 netmap/policy_decode_test.go diff --git a/netmap/policy.go b/netmap/policy.go index d706556..df84a8f 100644 --- a/netmap/policy.go +++ b/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()) } } diff --git a/netmap/policy_decode_test.go b/netmap/policy_decode_test.go new file mode 100644 index 0000000..d1e3cb6 --- /dev/null +++ b/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) + } +} diff --git a/netmap/policy_test.go b/netmap/policy_test.go index f954eec..05b0c98 100644 --- a/netmap/policy_test.go +++ b/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()