Fix selector path matching (#979)

## Description

Fixes an issue with the path matching logic in selectors where if the path was a substring of what was
specified as the scope in the filter, the item would still be matched.

e.g. 
Given 2 items - `/fold/contact1` and `/folderA/folderB/contact2` and a 
selector `er.Include(er.ContactFolders("AnyUser", []string{"folderA/folderB"}))`, 
the selector would match both items because `fold` is contained in the selector scope `folderA/folderB`

The fix is to invert the comparison - we check if the selector scope is contained in the item path instead.
In the example above, the selector string `folderA/folderB` will then only match the second items path.

## Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Test
- [ ] 💻 CI/Deployment
- [ ] 🐹 Trivial/Minor


## Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x]  Unit test
- [ ] 💚 E2E
This commit is contained in:
Vaibhav Kamra 2022-09-28 17:06:26 -07:00 committed by GitHub
parent f0ff7ee982
commit e59d596152
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 194 additions and 14 deletions

View File

@ -24,6 +24,9 @@ const (
Fails
// passthrough for the target
IdentityValue
// target is a prefix of the value it is compared
// against
TargetPrefixes
)
func norm(s string) string {
@ -119,6 +122,18 @@ func Identity(id string) Filter {
return newFilter(IdentityValue, id, false)
}
// Prefix creates a filter where Compare(v) is true if
// target.Prefix(v)
func Prefix(target string) Filter {
return newFilter(TargetPrefixes, target, false)
}
// NotPrefix creates a filter where Compare(v) is true if
// !target.Prefix(v)
func NotPrefix(target string) Filter {
return newFilter(TargetPrefixes, target, true)
}
// newFilter is the standard filter constructor.
func newFilter(c comparator, target string, negate bool) Filter {
return Filter{c, target, negate}
@ -143,6 +158,8 @@ func (f Filter) Compare(input string) bool {
cmp = contains
case TargetIn:
cmp = in
case TargetPrefixes:
cmp = prefixed
case Passes:
return true
case Fails:
@ -182,6 +199,11 @@ func in(target, input string) bool {
return strings.Contains(input, target)
}
// true if target has input as a prefix.
func prefixed(target, input string) bool {
return strings.HasPrefix(target, input)
}
// ----------------------------------------------------------------------------------------------------
// Helpers
// ----------------------------------------------------------------------------------------------------
@ -193,6 +215,7 @@ var prefixString = map[comparator]string{
LessThan: "lt:",
TargetContains: "cont:",
TargetIn: "in:",
TargetPrefixes: "pr:",
}
func (f Filter) String() string {

View File

@ -159,3 +159,28 @@ func (suite *FiltersSuite) TestIn_Joined() {
})
}
}
func (suite *FiltersSuite) TestPrefixes() {
input := "folderA"
table := []struct {
name string
target string
expectF assert.BoolAssertionFunc
expectNF assert.BoolAssertionFunc
}{
{"Exact match - same case", "folderA", assert.True, assert.False},
{"Exact match - different case", "Foldera", assert.True, assert.False},
{"Prefix match - same case", "folderA/folderB", assert.True, assert.False},
{"Prefix match - different case", "Foldera/folderB", assert.True, assert.False},
{"Should not match substring", "folder1/folderA", assert.False, assert.True},
}
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")
})
}
}

View File

@ -847,6 +847,10 @@ func (suite *ExchangeSelectorSuite) TestExchangeScope_MatchesPath() {
{"no folders", es.MailFolders(Any(), None()), "", assert.False},
{"matching folder", es.MailFolders(Any(), []string{fld}), "", assert.True},
{"non-matching folder", es.MailFolders(Any(), []string{"smarf"}), "", assert.False},
// This test validates that folders that match a substring of the scope are not included (bugfix 1)
{"non-matching folder substring", es.MailFolders(Any(), []string{fld + "_suffix"}), "", assert.False},
{"matching folder prefix", es.MailFolders(Any(), []string{"mailF"}), "", assert.True},
{"matching folder substring", es.MailFolders(Any(), []string{"Folder"}), "", assert.False},
{"one of multiple folders", es.MailFolders(Any(), []string{"smarf", fld}), "", assert.True},
{"all mail", es.Mails(Any(), Any(), Any()), "", assert.True},
{"no mail", es.Mails(Any(), Any(), None()), "", assert.False},
@ -913,9 +917,10 @@ func (suite *ExchangeSelectorSuite) TestIdPath() {
func (suite *ExchangeSelectorSuite) TestExchangeRestore_Reduce() {
var (
contact = stubRepoRef(path.ExchangeService, path.ContactsCategory, "uid", "cfld", "cid")
event = stubRepoRef(path.ExchangeService, path.EventsCategory, "uid", "ecld", "eid")
mail = stubRepoRef(path.ExchangeService, path.EmailCategory, "uid", "mfld", "mid")
contact = stubRepoRef(path.ExchangeService, path.ContactsCategory, "uid", "cfld", "cid")
event = stubRepoRef(path.ExchangeService, path.EventsCategory, "uid", "ecld", "eid")
mail = stubRepoRef(path.ExchangeService, path.EmailCategory, "uid", "mfld", "mid")
contactInSubFolder = stubRepoRef(path.ExchangeService, path.ContactsCategory, "uid", "cfld1/cfld2", "cid")
)
makeDeets := func(refs ...string) *details.Details {
@ -1020,6 +1025,16 @@ func (suite *ExchangeSelectorSuite) TestExchangeRestore_Reduce() {
},
arr(contact),
},
{
"only match contactInSubFolder",
makeDeets(contactInSubFolder, contact, event, mail),
func() *ExchangeRestore {
er := NewExchangeRestore()
er.Include(er.ContactFolders([]string{"uid"}, []string{"cfld1/cfld2"}))
return er
},
arr(contactInSubFolder),
},
{
"only match event",
makeDeets(contact, event, mail),

View File

@ -379,12 +379,44 @@ func matchesPathValues[T scopeT, C categoryT](
// all parts of the scope must match
cc := c.(C)
if !isAnyTarget(sc, cc) {
notMatch := filters.NotContains(join(scopeVals...))
if c.isLeaf() && len(shortRef) > 0 {
if notMatch.Compare(pathVal) && notMatch.Compare(shortRef) {
return false
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
}
}
} else if notMatch.Compare(pathVal) {
// short circuit if we found a match
if match {
break
}
}
if !match {
// Didn't match any scope
return false
}
}

View File

@ -164,6 +164,85 @@ func (suite *SelectorReduceSuite) TestReduce() {
testdata.ExchangeEventsItems...,
),
},
{
name: "ExchangeMailByFolder",
selFunc: func() selectors.Reducer {
sel := selectors.NewExchangeRestore()
sel.Include(sel.MailFolders(
selectors.Any(),
[]string{testdata.ExchangeEmailBasePath.Folder()},
))
return sel
},
expected: []details.DetailsEntry{testdata.ExchangeEmailItems[0]},
},
{
name: "ExchangeMailByFolderRoot",
selFunc: func() selectors.Reducer {
sel := selectors.NewExchangeRestore()
sel.Include(sel.MailFolders(
selectors.Any(),
[]string{testdata.ExchangeEmailInboxPath.Folder()},
))
return sel
},
expected: testdata.ExchangeEmailItems,
},
{
name: "ExchangeContactByFolder",
selFunc: func() selectors.Reducer {
sel := selectors.NewExchangeRestore()
sel.Include(sel.ContactFolders(
selectors.Any(),
[]string{testdata.ExchangeContactsBasePath.Folder()},
))
return sel
},
expected: []details.DetailsEntry{testdata.ExchangeContactsItems[0]},
},
{
name: "ExchangeContactByFolderRoot",
selFunc: func() selectors.Reducer {
sel := selectors.NewExchangeRestore()
sel.Include(sel.ContactFolders(
selectors.Any(),
[]string{testdata.ExchangeContactsRootPath.Folder()},
))
return sel
},
expected: testdata.ExchangeContactsItems,
},
{
name: "ExchangeEventsByFolder",
selFunc: func() selectors.Reducer {
sel := selectors.NewExchangeRestore()
sel.Include(sel.EventCalendars(
selectors.Any(),
[]string{testdata.ExchangeEventsBasePath.Folder()},
))
return sel
},
expected: []details.DetailsEntry{testdata.ExchangeEventsItems[0]},
},
{
name: "ExchangeEventsByFolderRoot",
selFunc: func() selectors.Reducer {
sel := selectors.NewExchangeRestore()
sel.Include(sel.EventCalendars(
selectors.Any(),
[]string{testdata.ExchangeEventsRootPath.Folder()},
))
return sel
},
expected: testdata.ExchangeEventsItems,
},
}
for _, test := range table {

View File

@ -42,9 +42,11 @@ var (
Time1 = time.Date(2022, 9, 21, 10, 0, 0, 0, time.UTC)
Time2 = time.Date(2022, 10, 21, 10, 0, 0, 0, time.UTC)
ExchangeEmailBasePath = mustParsePath("tenant-id/exchange/user-id/email/Inbox/subfolder", false)
ExchangeEmailInboxPath = mustParsePath("tenant-id/exchange/user-id/email/Inbox", false)
ExchangeEmailBasePath = mustAppendPath(ExchangeEmailInboxPath, "subfolder", false)
ExchangeEmailBasePath2 = mustAppendPath(ExchangeEmailInboxPath, "othersubfolder", false)
ExchangeEmailItemPath1 = mustAppendPath(ExchangeEmailBasePath, ItemName1, true)
ExchangeEmailItemPath2 = mustAppendPath(ExchangeEmailBasePath, ItemName2, true)
ExchangeEmailItemPath2 = mustAppendPath(ExchangeEmailBasePath2, ItemName2, true)
ExchangeEmailItems = []details.DetailsEntry{
{
@ -75,9 +77,11 @@ var (
},
}
ExchangeContactsBasePath = mustParsePath("tenant-id/exchange/user-id/contacts/contacts", false)
ExchangeContactsRootPath = mustParsePath("tenant-id/exchange/user-id/contacts/contacts", false)
ExchangeContactsBasePath = mustAppendPath(ExchangeContactsRootPath, "contacts", false)
ExchangeContactsBasePath2 = mustAppendPath(ExchangeContactsRootPath, "morecontacts", false)
ExchangeContactsItemPath1 = mustAppendPath(ExchangeContactsBasePath, ItemName1, true)
ExchangeContactsItemPath2 = mustAppendPath(ExchangeContactsBasePath, ItemName2, true)
ExchangeContactsItemPath2 = mustAppendPath(ExchangeContactsBasePath2, ItemName2, true)
ExchangeContactsItems = []details.DetailsEntry{
{
@ -104,9 +108,11 @@ var (
},
}
ExchangeEventsBasePath = mustParsePath("tenant-id/exchange/user-id/events/holidays", false)
ExchangeEventsRootPath = mustParsePath("tenant-id/exchange/user-id/events/holidays", false)
ExchangeEventsBasePath = mustAppendPath(ExchangeEventsRootPath, "holidays", false)
ExchangeEventsBasePath2 = mustAppendPath(ExchangeEventsRootPath, "moreholidays", false)
ExchangeEventsItemPath1 = mustAppendPath(ExchangeEventsBasePath, ItemName1, true)
ExchangeEventsItemPath2 = mustAppendPath(ExchangeEventsBasePath, ItemName2, true)
ExchangeEventsItemPath2 = mustAppendPath(ExchangeEventsBasePath2, ItemName2, true)
ExchangeEventsItems = []details.DetailsEntry{
{