From e5243404a8c438809db0093646870641e3c8d72a Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 13 Oct 2022 16:58:18 -0600 Subject: [PATCH] add prefix option to scopes (#1141) ## Description adds extensible options to folder-level scopes that allows the caller to specify whether they want a prefix-comparison matcher or a contains-comparison matcher. Also corrects the behavior of the prefix filter so that it accurately follows the "target is prefix of input" specification, rather than the reverse. ## Type of change - [x] :sunflower: Feature ## Issue(s) * #1133 ## Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/pkg/filters/filters.go | 17 +++- src/pkg/filters/filters_test.go | 33 +++++-- src/pkg/selectors/exchange.go | 50 ++-------- src/pkg/selectors/exchange_test.go | 49 ---------- src/pkg/selectors/onedrive.go | 4 +- src/pkg/selectors/scopes.go | 108 +++++++++++---------- src/pkg/selectors/scopes_test.go | 41 ++++++-- src/pkg/selectors/selectors.go | 44 ++++++--- src/pkg/selectors/selectors_reduce_test.go | 17 ++++ src/pkg/selectors/selectors_test.go | 19 ++-- 10 files changed, 191 insertions(+), 191 deletions(-) diff --git a/src/pkg/filters/filters.go b/src/pkg/filters/filters.go index e716d6cda..97361c2a9 100644 --- a/src/pkg/filters/filters.go +++ b/src/pkg/filters/filters.go @@ -24,8 +24,7 @@ const ( Fails // passthrough for the target IdentityValue - // target is a prefix of the value it is compared - // against + // "foo" is a prefix of "foo/bar/baz" TargetPrefixes ) @@ -143,6 +142,18 @@ func newFilter(c comparator, target string, negate bool) Filter { // Comparisons // ---------------------------------------------------------------------------------------------------- +// CompareAny checks whether any one of all the provided +// inputs passes the filter. +func (f Filter) CompareAny(inputs ...string) bool { + for _, in := range inputs { + if f.Compare(in) { + return true + } + } + + return false +} + // Compare checks whether the input passes the filter. func (f Filter) Compare(input string) bool { var cmp func(string, string) bool @@ -201,7 +212,7 @@ func in(target, input string) bool { // true if target has input as a prefix. func prefixed(target, input string) bool { - return strings.HasPrefix(target, input) + return strings.HasPrefix(input, target) } // ---------------------------------------------------------------------------------------------------- diff --git a/src/pkg/filters/filters_test.go b/src/pkg/filters/filters_test.go index bfa7237fa..d8b501185 100644 --- a/src/pkg/filters/filters_test.go +++ b/src/pkg/filters/filters_test.go @@ -37,6 +37,27 @@ func (suite *FiltersSuite) TestEquals() { } } +func (suite *FiltersSuite) TestEquals_any() { + f := filters.Equal("foo") + nf := filters.NotEqual("foo") + + table := []struct { + name string + input []string + expectF assert.BoolAssertionFunc + expectNF assert.BoolAssertionFunc + }{ + {"includes target", []string{"foo", "bar"}, assert.True, assert.True}, + {"not includes target", []string{"baz", "qux"}, assert.False, assert.True}, + } + for _, test := range table { + suite.T().Run(test.name, func(t *testing.T) { + test.expectF(t, f.CompareAny(test.input...), "filter") + test.expectNF(t, nf.CompareAny(test.input...), "negated filter") + }) + } +} + func (suite *FiltersSuite) TestGreater() { f := filters.Greater("5") nf := filters.NotGreater("5") @@ -161,11 +182,13 @@ func (suite *FiltersSuite) TestIn_Joined() { } func (suite *FiltersSuite) TestPrefixes() { - input := "folderA" + target := "folderA" + f := filters.Prefix(target) + nf := filters.NotPrefix(target) table := []struct { name string - target string + input string expectF assert.BoolAssertionFunc expectNF assert.BoolAssertionFunc }{ @@ -177,10 +200,8 @@ func (suite *FiltersSuite) TestPrefixes() { } for _, test := range table { suite.T().Run(test.name, func(t *testing.T) { - f := filters.Prefix(test.target) - nf := filters.NotPrefix(test.target) - test.expectF(t, f.Compare(input), "filter") - test.expectNF(t, nf.Compare(input), "negated filter") + test.expectF(t, f.Compare(test.input), "filter") + test.expectNF(t, nf.Compare(test.input), "negated filter") }) } } diff --git a/src/pkg/selectors/exchange.go b/src/pkg/selectors/exchange.go index a4e41dedf..027cc277b 100644 --- a/src/pkg/selectors/exchange.go +++ b/src/pkg/selectors/exchange.go @@ -189,12 +189,12 @@ func (s *exchange) Contacts(users, folders, contacts []string) []ExchangeScope { // If any slice contains selectors.Any, that slice is reduced to [selectors.Any] // If any slice contains selectors.None, that slice is reduced to [selectors.None] // If any slice is empty, it defaults to [selectors.None] -func (s *exchange) ContactFolders(users, folders []string) []ExchangeScope { +func (s *exchange) ContactFolders(users, folders []string, opts ...option) []ExchangeScope { scopes := []ExchangeScope{} scopes = append( scopes, - makeScope[ExchangeScope](ExchangeContactFolder, users, folders), + makeScope[ExchangeScope](ExchangeContactFolder, users, folders, opts...), ) return scopes @@ -221,12 +221,12 @@ func (s *exchange) Events(users, calendars, events []string) []ExchangeScope { // If any slice contains selectors.Any, that slice is reduced to [selectors.Any] // If any slice contains selectors.None, that slice is reduced to [selectors.None] // If any slice is empty, it defaults to [selectors.None] -func (s *exchange) EventCalendars(users, events []string) []ExchangeScope { +func (s *exchange) EventCalendars(users, events []string, opts ...option) []ExchangeScope { scopes := []ExchangeScope{} scopes = append( scopes, - makeScope[ExchangeScope](ExchangeEventCalendar, users, events), + makeScope[ExchangeScope](ExchangeEventCalendar, users, events, opts...), ) return scopes @@ -252,12 +252,12 @@ func (s *exchange) Mails(users, folders, mails []string) []ExchangeScope { // If any slice contains selectors.Any, that slice is reduced to [selectors.Any] // If any slice contains selectors.None, that slice is reduced to [selectors.None] // If any slice is empty, it defaults to [selectors.None] -func (s *exchange) MailFolders(users, folders []string) []ExchangeScope { +func (s *exchange) MailFolders(users, folders []string, opts ...option) []ExchangeScope { scopes := []ExchangeScope{} scopes = append( scopes, - makeScope[ExchangeScope](ExchangeMailFolder, users, folders), + makeScope[ExchangeScope](ExchangeMailFolder, users, folders, opts...), ) return scopes @@ -429,44 +429,6 @@ func (sr *ExchangeRestore) MailSubject(subject string) []ExchangeScope { } } -// --------------------------------------------------------------------------- -// Destination -// --------------------------------------------------------------------------- - -type ExchangeDestination Destination - -func NewExchangeDestination() ExchangeDestination { - return ExchangeDestination{} -} - -// GetOrDefault gets the destination of the provided category. If no -// destination is set, returns the current value. -func (d ExchangeDestination) GetOrDefault(cat exchangeCategory, current string) string { - dest, ok := d[cat.String()] - if !ok { - return current - } - - return dest.Target -} - -// Sets the destination value of the provided category. Returns an error -// if a destination is already declared for that category. -func (d ExchangeDestination) Set(cat exchangeCategory, dest string) error { - if len(dest) == 0 { - return nil - } - - cs := cat.String() - if curr, ok := d[cs]; ok { - return existingDestinationErr(cs, curr.Target) - } - - d[cs] = filterize(dest) - - return nil -} - // --------------------------------------------------------------------------- // Categories // --------------------------------------------------------------------------- diff --git a/src/pkg/selectors/exchange_test.go b/src/pkg/selectors/exchange_test.go index 0524e1d84..ac483f089 100644 --- a/src/pkg/selectors/exchange_test.go +++ b/src/pkg/selectors/exchange_test.go @@ -479,55 +479,6 @@ func (suite *ExchangeSelectorSuite) TestExchangeSelector_Include_Users() { } } -func (suite *ExchangeSelectorSuite) TestNewExchangeDestination() { - t := suite.T() - dest := NewExchangeDestination() - assert.Len(t, dest, 0) -} - -func (suite *ExchangeSelectorSuite) TestExchangeDestination_Set() { - dest := NewExchangeDestination() - - table := []exchangeCategory{ - ExchangeCategoryUnknown, - ExchangeContact, - ExchangeContactFolder, - ExchangeEvent, - ExchangeMail, - ExchangeMailFolder, - ExchangeUser, - } - for _, test := range table { - suite.T().Run(test.String(), func(t *testing.T) { - assert.NoError(t, dest.Set(test, "foo")) - assert.Error(t, dest.Set(test, "foo")) - }) - } - - assert.NoError(suite.T(), dest.Set(ExchangeUser, "")) -} - -func (suite *ExchangeSelectorSuite) TestExchangeDestination_GetOrDefault() { - dest := NewExchangeDestination() - - table := []exchangeCategory{ - ExchangeCategoryUnknown, - ExchangeContact, - ExchangeContactFolder, - ExchangeEvent, - ExchangeMail, - ExchangeMailFolder, - ExchangeUser, - } - for _, test := range table { - suite.T().Run(test.String(), func(t *testing.T) { - assert.Equal(t, "bar", dest.GetOrDefault(test, "bar")) - assert.NoError(t, dest.Set(test, "foo")) - assert.Equal(t, "foo", dest.GetOrDefault(test, "bar")) - }) - } -} - func (suite *ExchangeSelectorSuite) TestExchangeBackup_Scopes() { eb := NewExchangeBackup() eb.Include(eb.Users(Any())) diff --git a/src/pkg/selectors/onedrive.go b/src/pkg/selectors/onedrive.go index d5d77ccc5..136857696 100644 --- a/src/pkg/selectors/onedrive.go +++ b/src/pkg/selectors/onedrive.go @@ -177,12 +177,12 @@ func (s *oneDrive) Users(users []string) []OneDriveScope { // If any slice contains selectors.Any, that slice is reduced to [selectors.Any] // If any slice contains selectors.None, that slice is reduced to [selectors.None] // If any slice is empty, it defaults to [selectors.None] -func (s *oneDrive) Folders(users, folders []string) []OneDriveScope { +func (s *oneDrive) Folders(users, folders []string, opts ...option) []OneDriveScope { scopes := []OneDriveScope{} scopes = append( scopes, - makeScope[OneDriveScope](OneDriveFolder, users, folders), + makeScope[OneDriveScope](OneDriveFolder, users, folders, opts...), ) return scopes diff --git a/src/pkg/selectors/scopes.go b/src/pkg/selectors/scopes.go index 554634411..5aaed5184 100644 --- a/src/pkg/selectors/scopes.go +++ b/src/pkg/selectors/scopes.go @@ -119,12 +119,19 @@ type ( func makeScope[T scopeT]( cat categorizer, resources, vs []string, + opts ...option, ) T { + sc := scopeConfig{} + + for _, opt := range opts { + opt(&sc) + } + s := T{ scopeKeyCategory: filters.Identity(cat.String()), scopeKeyDataType: filters.Identity(cat.leafCat().String()), - cat.String(): filterize(vs...), - cat.rootCat().String(): filterize(resources...), + cat.String(): filterize(sc, vs...), + cat.rootCat().String(): filterize(scopeConfig{}, resources...), } return s @@ -189,10 +196,20 @@ func getCatValue[T scopeT](s T, cat categorizer) []string { // set sets a value by category to the scope. Only intended for internal // use, not for exporting to callers. func set[T scopeT](s T, cat categorizer, v []string) T { - s[cat.String()] = filterize(v...) + s[cat.String()] = filterize(scopeConfig{}, v...) return s } +// returns true if the category is included in the scope's category type, +// and the value is set to None(). +func isNoneTarget[T scopeT, C categoryT](s T, cat C) bool { + if !typeAndCategoryMatches(cat, s.categorizer()) { + return false + } + + return s[cat.String()].Target == NoneTgt +} + // returns true if the category is included in the scope's category type, // and the value is set to Any(). func isAnyTarget[T scopeT, C categoryT](s T, cat C) bool { @@ -361,67 +378,54 @@ func matchesPathValues[T scopeT, C categoryT]( shortRef string, ) bool { for _, c := range cat.pathKeys() { - scopeVals := getCatValue(sc, c) - - // the scope must define the targets to match on - if len(scopeVals) == 0 { - return false - } - - // None() fails all matches - if scopeVals[0] == NoneTgt { - return false - } - // the pathValues must have an entry for the given categorizer pathVal, ok := pathValues[c] if !ok { return false } - // all parts of the scope must match cc := c.(C) - if !isAnyTarget(sc, cc) { - var ( - match = false - // Used to check if the path contains the value specified in scopeVals - pathHas = filters.Contains(pathVal) - // Used to check if the path has the value specified in scopeVal as a prefix - pathPrefix = filters.Prefix(pathVal) - // Used to check if the shortRef equals the value specified in scopeVals - shortRefEq = filters.Equal(shortRef) - ) - for _, scopeVal := range scopeVals { - switch { - case c.isLeaf() && len(shortRef) > 0: - // Leaf category - we do a "contains" match for path or equality match on - // the shortRef - if pathHas.Compare(scopeVal) || shortRefEq.Compare(scopeVal) { - match = true - } - case !c.isLeaf() && c != c.rootCat(): - // Folder category - we check if the scope is a prefix - // TODO: If the scopeVal is not a "path" - then we'll want to check - // if any of the path elements match the scopeVal exactly - if pathPrefix.Compare(scopeVal) { - match = true - } - default: - if pathHas.Compare(scopeVal) { - match = true - } - } - // short circuit if we found a match - if match { + if isNoneTarget(sc, cc) { + return false + } + + if isAnyTarget(sc, cc) { + // continue, not return: all path keys must match the entry to succeed + continue + } + + var ( + match bool + isLeaf = c.isLeaf() + isRoot = c == c.rootCat() + ) + + switch { + // Leaf category - the scope can match either the path value (the item ID itself), + // or the shortRef hash representing the item. + case isLeaf && len(shortRef) > 0: + match = matches(sc, cc, pathVal) || matches(sc, cc, shortRef) + + // Folder category - checks if any target folder is a prefix of the path folders. + // Assumes (correctly) that we need to split the targets and re-compose them into + // individual prefix matchers. + // TODO: assumes all folders require prefix matchers. Users can now specify whether + // the folder filter is a prefix match or not. We should respect that configuration. + case !isLeaf && !isRoot: + for _, tgt := range getCatValue(sc, c) { + if filters.Prefix(tgt).Compare(pathVal) { + match = true break } } - if !match { - // Didn't match any scope - return false - } + default: + match = matches(sc, cc, pathVal) + } + + if !match { + return false } } diff --git a/src/pkg/selectors/scopes_test.go b/src/pkg/selectors/scopes_test.go index ce1b55c14..4670e7ce8 100644 --- a/src/pkg/selectors/scopes_test.go +++ b/src/pkg/selectors/scopes_test.go @@ -65,7 +65,7 @@ func (suite *SelectorScopesSuite) TestContains() { name: "blank target", scope: func() mockScope { stub := stubScope("") - stub[rootCatStub.String()] = filterize("fnords") + stub[rootCatStub.String()] = filterize(scopeConfig{}, "fnords") return stub }, check: "", @@ -75,7 +75,7 @@ func (suite *SelectorScopesSuite) TestContains() { name: "matching target", scope: func() mockScope { stub := stubScope("") - stub[rootCatStub.String()] = filterize(rootCatStub.String()) + stub[rootCatStub.String()] = filterize(scopeConfig{}, rootCatStub.String()) return stub }, check: rootCatStub.String(), @@ -85,7 +85,7 @@ func (suite *SelectorScopesSuite) TestContains() { name: "non-matching target", scope: func() mockScope { stub := stubScope("") - stub[rootCatStub.String()] = filterize(rootCatStub.String()) + stub[rootCatStub.String()] = filterize(scopeConfig{}, rootCatStub.String()) return stub }, check: "smarf", @@ -105,7 +105,7 @@ func (suite *SelectorScopesSuite) TestGetCatValue() { t := suite.T() stub := stubScope("") - stub[rootCatStub.String()] = filterize(rootCatStub.String()) + stub[rootCatStub.String()] = filterize(scopeConfig{}, rootCatStub.String()) assert.Equal(t, []string{rootCatStub.String()}, @@ -265,7 +265,7 @@ func (suite *SelectorScopesSuite) TestScopesByCategory() { t := suite.T() s1 := stubScope("") s2 := stubScope("") - s2[scopeKeyCategory] = filterize(unknownCatStub.String()) + s2[scopeKeyCategory] = filterize(scopeConfig{}, unknownCatStub.String()) result := scopesByCategory[mockScope]( []scope{scope(s1), scope(s2)}, map[path.CategoryType]mockCategorizer{ @@ -368,8 +368,8 @@ func (suite *SelectorScopesSuite) TestMatchesPathValues() { for _, test := range table { suite.T().Run(test.name, func(t *testing.T) { sc := stubScope("") - sc[rootCatStub.String()] = filterize(test.rootVal) - sc[leafCatStub.String()] = filterize(test.leafVal) + sc[rootCatStub.String()] = filterize(scopeConfig{}, test.rootVal) + sc[leafCatStub.String()] = filterize(scopeConfig{}, test.leafVal) test.expect(t, matchesPathValues(sc, cat, pvs, test.shortRef)) }) @@ -486,3 +486,30 @@ func (suite *SelectorScopesSuite) TestWrapFilter() { }) } } + +func (suite *SelectorScopesSuite) TestScopeConfig() { + input := "input" + + table := []struct { + name string + config scopeConfig + expect int + }{ + { + name: "no configs set", + config: scopeConfig{}, + expect: int(filters.EqualTo), + }, + { + name: "force prefix", + config: scopeConfig{usePrefixFilter: true}, + expect: int(filters.TargetPrefixes), + }, + } + for _, test := range table { + suite.T().Run(test.name, func(t *testing.T) { + result := filterize(test.config, input) + assert.Equal(t, test.expect, int(result.Comparator)) + }) + } +} diff --git a/src/pkg/selectors/selectors.go b/src/pkg/selectors/selectors.go index 88bca941f..5729d7539 100644 --- a/src/pkg/selectors/selectors.go +++ b/src/pkg/selectors/selectors.go @@ -309,12 +309,23 @@ func addToSet(set []string, v []string) []string { } // --------------------------------------------------------------------------- -// Destination +// helpers // --------------------------------------------------------------------------- -type Destination scope +type scopeConfig struct { + usePrefixFilter bool +} -var ErrorDestinationAlreadySet = errors.New("destination is already declared") +type option func(*scopeConfig) + +// PrefixMatch ensures the selector uses a Prefix comparator, instead +// of contains or equals. Will not override a default Any() or None() +// comparator. +func PrefixMatch() option { + return func(sc *scopeConfig) { + sc.usePrefixFilter = true + } +} // --------------------------------------------------------------------------- // helpers @@ -324,10 +335,6 @@ func badCastErr(cast, is service) error { return errors.Wrapf(ErrorBadSelectorCast, "%s service is not %s", cast, is) } -func existingDestinationErr(category, is string) error { - return errors.Wrapf(ErrorDestinationAlreadySet, "%s destination already set to %s", category, is) -} - func join(s ...string) string { return strings.Join(s, delimiter) } @@ -362,20 +369,25 @@ func clean(s []string) []string { // filterize turns the slice into a filter. // if the input is Any(), returns a passAny filter. // if the input is None(), returns a failAny filter. +// if the scopeConfig specifies a filter, use that filter. // if the input is len(1), returns an Equals filter. // otherwise returns a Contains filter. -func filterize(s ...string) filters.Filter { +func filterize(sc scopeConfig, s ...string) filters.Filter { s = clean(s) + if len(s) == 0 || s[0] == NoneTgt { + return failAny + } + + if s[0] == AnyTgt { + return passAny + } + + if sc.usePrefixFilter { + return filters.Prefix(join(s...)) + } + if len(s) == 1 { - if s[0] == AnyTgt { - return passAny - } - - if s[0] == NoneTgt { - return failAny - } - return filters.Equal(s[0]) } diff --git a/src/pkg/selectors/selectors_reduce_test.go b/src/pkg/selectors/selectors_reduce_test.go index 7aae1bc32..e3ecbd736 100644 --- a/src/pkg/selectors/selectors_reduce_test.go +++ b/src/pkg/selectors/selectors_reduce_test.go @@ -192,6 +192,23 @@ func (suite *SelectorReduceSuite) TestReduce() { }, expected: []details.DetailsEntry{testdata.ExchangeEmailItems[0]}, }, + // TODO (keepers): all folders are treated as prefix-matches at this time. + // so this test actually does nothing different. In the future, we'll + // need to amend the non-prefix folder tests to expect non-prefix matches. + { + name: "ExchangeMailByFolderPrefix", + selFunc: func() selectors.Reducer { + sel := selectors.NewExchangeRestore() + sel.Include(sel.MailFolders( + selectors.Any(), + []string{testdata.ExchangeEmailBasePath.Folder()}, + selectors.PrefixMatch(), // force prefix matching + )) + + return sel + }, + expected: []details.DetailsEntry{testdata.ExchangeEmailItems[0]}, + }, { name: "ExchangeMailByFolderRoot", selFunc: func() selectors.Reducer { diff --git a/src/pkg/selectors/selectors_test.go b/src/pkg/selectors/selectors_test.go index e1c25e3e6..fe977ed29 100644 --- a/src/pkg/selectors/selectors_test.go +++ b/src/pkg/selectors/selectors_test.go @@ -29,11 +29,6 @@ func (suite *SelectorSuite) TestBadCastErr() { assert.Error(suite.T(), err) } -func (suite *SelectorSuite) TestExistingDestinationErr() { - err := existingDestinationErr("foo", "bar") - assert.Error(suite.T(), err) -} - func (suite *SelectorSuite) TestPrintable() { t := suite.T() @@ -57,7 +52,7 @@ func (suite *SelectorSuite) TestPrintable_IncludedResources() { stubWithResource := func(resource string) scope { ss := stubScope("") - ss[rootCatStub.String()] = filterize(resource) + ss[rootCatStub.String()] = filterize(scopeConfig{}, resource) return scope(ss) } @@ -102,8 +97,8 @@ func (suite *SelectorSuite) TestToResourceTypeMap() { input: []scope{ scope(stubScope("")), { - rootCatStub.String(): filterize("smarf"), - scopeKeyDataType: filterize(unknownCatStub.String()), + rootCatStub.String(): filterize(scopeConfig{}, "smarf"), + scopeKeyDataType: filterize(scopeConfig{}, unknownCatStub.String()), }, }, expect: map[string][]string{ @@ -116,8 +111,8 @@ func (suite *SelectorSuite) TestToResourceTypeMap() { input: []scope{ scope(stubScope("")), { - rootCatStub.String(): filterize(AnyTgt), - scopeKeyDataType: filterize("other"), + rootCatStub.String(): filterize(scopeConfig{}, AnyTgt), + scopeKeyDataType: filterize(scopeConfig{}, "other"), }, }, expect: map[string][]string{ @@ -138,9 +133,9 @@ func (suite *SelectorSuite) TestContains() { key := rootCatStub target := "fnords" does := stubScope("") - does[key.String()] = filterize(target) + does[key.String()] = filterize(scopeConfig{}, target) doesNot := stubScope("") - doesNot[key.String()] = filterize("smarf") + doesNot[key.String()] = filterize(scopeConfig{}, "smarf") assert.True(t, matches(does, key, target), "does contain") assert.False(t, matches(doesNot, key, target), "does not contain")