match item type in groups selectors info filter (#4255)

adds item type comparisons to the info filter during groups selector reduction.  This ensures cross-
contamination of item types on shared info properties does not occur.

---

#### Does this PR need a docs update or release note?

- [x] ⛔ No

#### Type of change

- [x] 🐛 Bugfix

#### Issue(s)

* #3988 

#### Test Plan

- [x] ⚡ Unit test
- [x] 💚 E2E

src/internal/m365/backup.go

@@ -19,7 +19,6 @@ import (
 	bupMD "github.com/alcionai/corso/src/pkg/backup/metadata"
 	"github.com/alcionai/corso/src/pkg/fault"
 	"github.com/alcionai/corso/src/pkg/filters"
-	"github.com/alcionai/corso/src/pkg/logger"
 	"github.com/alcionai/corso/src/pkg/path"
 	"github.com/alcionai/corso/src/pkg/selectors"
 )
@@ -63,26 +62,6 @@ func (ctrl *Controller) ProduceBackupCollections(
 		canUsePreviousBackup bool
 	)
 
-	// All services except Exchange can make delta queries by default.
-	// Exchange can only make delta queries if the mailbox is not over quota.
-	canMakeDeltaQueries := true
-	if service == path.ExchangeService {
-		canMakeDeltaQueries, err = exchange.CanMakeDeltaQueries(
-			ctx,
-			service,
-			ctrl.AC.Users(),
-			bpc.ProtectedResource.ID())
-		if err != nil {
-			return nil, nil, false, clues.Stack(err)
-		}
-	}
-
-	if !canMakeDeltaQueries {
-		logger.Ctx(ctx).Info("delta requests not available")
-
-		bpc.Options.ToggleFeatures.DisableDelta = true
-	}
-
 	switch service {
 	case path.ExchangeService:
 		colls, ssmb, canUsePreviousBackup, err = exchange.ProduceBackupCollections(
@@ -162,7 +141,7 @@ func (ctrl *Controller) IsServiceEnabled(
 	case path.OneDriveService:
 		return onedrive.IsServiceEnabled(ctx, ctrl.AC.Users(), resourceOwner)
 	case path.SharePointService:
-		return sharepoint.IsServiceEnabled(ctx, ctrl.AC.Users().Sites(), resourceOwner)
+		return sharepoint.IsServiceEnabled(ctx, ctrl.AC.Sites(), resourceOwner)
 	case path.GroupsService:
 		return groups.IsServiceEnabled(ctx, ctrl.AC.Groups(), resourceOwner)
 	}

src/internal/m365/service/exchange/backup.go

@@ -12,6 +12,7 @@ import (
 	"github.com/alcionai/corso/src/internal/m365/support"
 	"github.com/alcionai/corso/src/internal/operations/inject"
 	"github.com/alcionai/corso/src/pkg/fault"
+	"github.com/alcionai/corso/src/pkg/logger"
 	"github.com/alcionai/corso/src/pkg/path"
 	"github.com/alcionai/corso/src/pkg/services/m365/api"
 )
@@ -38,6 +39,17 @@ func ProduceBackupCollections(
 		handlers    = exchange.BackupHandlers(ac)
 	)
 
+	canMakeDeltaQueries, err := canMakeDeltaQueries(ctx, ac.Users(), bpc.ProtectedResource.ID())
+	if err != nil {
+		return nil, nil, false, clues.Stack(err)
+	}
+
+	if !canMakeDeltaQueries {
+		logger.Ctx(ctx).Info("delta requests not available")
+
+		bpc.Options.ToggleFeatures.DisableDelta = true
+	}
+
 	// Turn on concurrency limiter middleware for exchange backups
 	// unless explicitly disabled through DisableConcurrencyLimiterFN cli flag
 	graph.InitializeConcurrencyLimiter(
@@ -96,9 +108,8 @@ func ProduceBackupCollections(
 	return collections, nil, canUsePreviousBackup, el.Failure()
 }
 
-func CanMakeDeltaQueries(
+func canMakeDeltaQueries(
 	ctx context.Context,
-	service path.ServiceType,
 	gmi getMailboxer,
 	resourceOwner string,
 ) (bool, error) {

src/internal/m365/service/exchange/enabled_test.go

@@ -64,7 +64,7 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
 		name      string
 		mock      func(context.Context) getMailInboxer
 		expect    assert.BoolAssertionFunc
-		expectErr func(*testing.T, error)
+		expectErr assert.ErrorAssertionFunc
 	}{
 		{
 			name: "ok",
@@ -74,9 +74,7 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
 				}
 			},
 			expect:    assert.True,
-			expectErr: func(t *testing.T, err error) {
+			expectErr: assert.NoError,
-				assert.NoError(t, err, clues.ToCore(err))
-			},
 		},
 		{
 			name: "user has no mailbox",
@@ -88,9 +86,7 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
 				}
 			},
 			expect:    assert.False,
-			expectErr: func(t *testing.T, err error) {
+			expectErr: assert.NoError,
-				assert.NoError(t, err, clues.ToCore(err))
-			},
 		},
 		{
 			name: "user not found",
@@ -102,9 +98,7 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
 				}
 			},
 			expect:    assert.False,
-			expectErr: func(t *testing.T, err error) {
+			expectErr: assert.Error,
-				assert.Error(t, err, clues.ToCore(err))
-			},
 		},
 		{
 			name: "overlapping resourcenotfound",
@@ -116,9 +110,7 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
 				}
 			},
 			expect:    assert.False,
-			expectErr: func(t *testing.T, err error) {
+			expectErr: assert.Error,
-				assert.Error(t, err, clues.ToCore(err))
-			},
 		},
 		{
 			name: "arbitrary error",
@@ -130,9 +122,7 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
 				}
 			},
 			expect:    assert.False,
-			expectErr: func(t *testing.T, err error) {
+			expectErr: assert.Error,
-				assert.Error(t, err, clues.ToCore(err))
-			},
 		},
 	}
 	for _, test := range table {
@@ -146,7 +136,7 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
 
 			ok, err := IsServiceEnabled(ctx, gmi, "resource_id")
 			test.expect(t, ok, "has mailbox flag")
-			test.expectErr(t, err)
+			test.expectErr(t, err, clues.ToCore(err))
 		})
 	}
 }

src/internal/m365/service/groups/backup.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/alcionai/clues"
 	"github.com/kopia/kopia/repo/manifest"
-	"golang.org/x/exp/slices"
 
 	"github.com/alcionai/corso/src/internal/common/idname"
 	"github.com/alcionai/corso/src/internal/common/prefixmatcher"
@@ -56,26 +55,12 @@ func ProduceBackupCollections(
 		"group_id", clues.Hide(bpc.ProtectedResource.ID()),
 		"group_name", clues.Hide(bpc.ProtectedResource.Name()))
 
-	resp, err := ac.Groups().GetByID(ctx, bpc.ProtectedResource.ID())
+	group, err := ac.Groups().GetByID(ctx, bpc.ProtectedResource.ID())
 	if err != nil {
 		return nil, nil, false, clues.Wrap(err, "getting group").WithClues(ctx)
 	}
 
-	// Not all groups will have associated SharePoint
+	isTeam := api.IsTeam(ctx, group)
-	// sites. Distribution channels and Security groups will not
-	// have one. This check is to skip those groups.
-	groupTypes := resp.GetGroupTypes()
-	hasSharePoint := slices.Contains(groupTypes, "Unified")
-
-	// If we don't have SharePoint site, there is nothing here to
-	// backup as of now.
-	if !hasSharePoint {
-		logger.Ctx(ctx).
-			With("group_id", bpc.ProtectedResource.ID()).
-			Infof("No SharePoint site found for group")
-
-		return nil, nil, false, clues.Stack(graph.ErrServiceNotEnabled, err).WithClues(ctx)
-	}
 
 	for _, scope := range b.Scopes() {
 		if el.Failure() != nil {
@@ -143,6 +128,10 @@ func ProduceBackupCollections(
 			}
 
 		case path.ChannelMessagesCategory:
+			if !isTeam {
+				continue
+			}
+
 			dbcs, canUsePreviousBackup, err = groups.CreateCollections(
 				ctx,
 				bpc,

src/internal/m365/service/groups/enabled.go

@@ -3,7 +3,10 @@ package groups
 import (
 	"context"
 
+	"github.com/alcionai/clues"
 	"github.com/microsoftgraph/msgraph-sdk-go/models"
+
+	"github.com/alcionai/corso/src/pkg/filters"
 )
 
 type getByIDer interface {
@@ -15,7 +18,20 @@ func IsServiceEnabled(
 	gbi getByIDer,
 	resource string,
 ) (bool, error) {
-	// TODO(meain): check for error message in case groups are
+	resp, err := gbi.GetByID(ctx, resource)
-	// not enabled at all similar to sharepoint
+	if err != nil {
-	return true, nil
+		return false, clues.Wrap(err, "getting group").WithClues(ctx)
+	}
+
+	// according to graph api docs: https://learn.microsoft.com/en-us/graph/api/resources/group?view=graph-rest-1.0
+	// "If the collection contains Unified, the group is a Microsoft 365 group;
+	// otherwise, it's either a security group or distribution group."
+	//
+	// Basically, if it's "unified", then we actually have data to back up.
+	// If it's not unified, then its purely a mailing list, and has no backing data.
+	isUnified := filters.
+		Equal(resp.GetGroupTypes()).
+		Compare("unified")
+
+	return isUnified, nil
 }

src/internal/m365/service/groups/enabled_test.go

@@ -0,0 +1,118 @@
+package groups
+
+import (
+	"context"
+	"testing"
+
+	"github.com/alcionai/clues"
+	"github.com/microsoftgraph/msgraph-sdk-go/models"
+	"github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/suite"
+
+	"github.com/alcionai/corso/src/internal/m365/graph"
+	"github.com/alcionai/corso/src/internal/tester"
+)
+
+type EnabledUnitSuite struct {
+	tester.Suite
+}
+
+func TestEnabledUnitSuite(t *testing.T) {
+	suite.Run(t, &EnabledUnitSuite{Suite: tester.NewUnitSuite(t)})
+}
+
+var _ getByIDer = mockGBI{}
+
+type mockGBI struct {
+	group models.Groupable
+	err   error
+}
+
+func (m mockGBI) GetByID(ctx context.Context, identifier string) (models.Groupable, error) {
+	return m.group, m.err
+}
+
+// TODO(pandeyabs): Duplicate of graph/errors_test.go. Remove
+// this and identical funcs in od/sp and use the one in graph/errors_test.go
+// instead.
+func odErrMsg(code, message string) *odataerrors.ODataError {
+	odErr := odataerrors.NewODataError()
+	merr := odataerrors.NewMainError()
+	merr.SetCode(&code)
+	merr.SetMessage(&message)
+	odErr.SetErrorEscaped(merr)
+
+	return odErr
+}
+
+func (suite *EnabledUnitSuite) TestIsServiceEnabled() {
+	var (
+		unified    = models.NewGroup()
+		nonUnified = models.NewGroup()
+	)
+
+	unified.SetGroupTypes([]string{"unified"})
+
+	table := []struct {
+		name      string
+		mock      func(context.Context) getByIDer
+		expect    assert.BoolAssertionFunc
+		expectErr assert.ErrorAssertionFunc
+	}{
+		{
+			name: "ok",
+			mock: func(ctx context.Context) getByIDer {
+				return mockGBI{
+					group: unified,
+				}
+			},
+			expect:    assert.True,
+			expectErr: assert.NoError,
+		},
+		{
+			name: "non-unified group",
+			mock: func(ctx context.Context) getByIDer {
+				return mockGBI{
+					group: nonUnified,
+				}
+			},
+			expect:    assert.False,
+			expectErr: assert.NoError,
+		},
+		{
+			name: "group not found",
+			mock: func(ctx context.Context) getByIDer {
+				return mockGBI{
+					err: graph.Stack(ctx, odErrMsg(string(graph.RequestResourceNotFound), "message")),
+				}
+			},
+			expect:    assert.False,
+			expectErr: assert.Error,
+		},
+		{
+			name: "arbitrary error",
+			mock: func(ctx context.Context) getByIDer {
+				return mockGBI{
+					err: assert.AnError,
+				}
+			},
+			expect:    assert.False,
+			expectErr: assert.Error,
+		},
+	}
+	for _, test := range table {
+		suite.Run(test.name, func() {
+			t := suite.T()
+
+			ctx, flush := tester.NewContext(t)
+			defer flush()
+
+			gmi := test.mock(ctx)
+
+			ok, err := IsServiceEnabled(ctx, gmi, "resource_id")
+			test.expect(t, ok, "has mailbox flag")
+			test.expectErr(t, err, clues.ToCore(err))
+		})
+	}
+}

src/pkg/selectors/groups.go

@@ -770,6 +770,15 @@ func (s GroupsScope) matchesInfo(dii details.ItemInfo) bool {
 		return false
 	}
 
+	acceptableItemType := -1
+
+	switch infoCat.leafCat() {
+	case GroupsLibraryItem:
+		acceptableItemType = int(details.SharePointLibrary)
+	case GroupsChannelMessage:
+		acceptableItemType = int(details.GroupsChannelMessage)
+	}
+
 	switch infoCat {
 	case GroupsInfoSiteLibraryDrive:
 		ds := []string{}
@@ -795,5 +804,5 @@ func (s GroupsScope) matchesInfo(dii details.ItemInfo) bool {
 		i = dttm.Format(info.LastReplyAt)
 	}
 
-	return s.Matches(infoCat, i)
+	return s.Matches(infoCat, i) && int(info.ItemType) == acceptableItemType
 }

src/pkg/selectors/groups_test.go

@@ -364,10 +364,10 @@ func (suite *GroupsSelectorSuite) TestGroupsScope_MatchesInfo() {
 		// url          = host + pth
 		epoch  = time.Time{}
 		now    = time.Now()
-		modification = now.Add(15 * time.Minute)
+		mod    = now.Add(15 * time.Minute)
 		future = now.Add(45 * time.Minute)
-		dtch         = details.GroupsChannelMessage
+		dgcm   = details.GroupsChannelMessage
-		dtsl         = details.SharePointLibrary
+		dspl   = details.SharePointLibrary
 	)
 
 	table := []struct {
@@ -377,39 +377,44 @@ func (suite *GroupsSelectorSuite) TestGroupsScope_MatchesInfo() {
 		scope    []GroupsScope
 		expect   assert.BoolAssertionFunc
 	}{
-		{"file create after the epoch", dtsl, user, sel.CreatedAfter(dttm.Format(epoch)), assert.True},
+		{"file create after the epoch", dspl, user, sel.CreatedAfter(dttm.Format(epoch)), assert.True},
-		{"file create after now", dtsl, user, sel.CreatedAfter(dttm.Format(now)), assert.False},
+		{"file create after the epoch wrong type", dgcm, user, sel.CreatedAfter(dttm.Format(epoch)), assert.False},
-		{"file create after later", dtsl, user, sel.CreatedAfter(dttm.Format(future)), assert.False},
+		{"file create after now", dspl, user, sel.CreatedAfter(dttm.Format(now)), assert.False},
-		{"file create before future", dtsl, user, sel.CreatedBefore(dttm.Format(future)), assert.True},
+		{"file create after later", dspl, user, sel.CreatedAfter(dttm.Format(future)), assert.False},
-		{"file create before now", dtsl, user, sel.CreatedBefore(dttm.Format(now)), assert.False},
+		{"file create before future", dspl, user, sel.CreatedBefore(dttm.Format(future)), assert.True},
-		{"file create before modification", dtsl, user, sel.CreatedBefore(dttm.Format(modification)), assert.True},
+		{"file create before future wrong type", dgcm, user, sel.CreatedBefore(dttm.Format(future)), assert.False},
-		{"file create before epoch", dtsl, user, sel.CreatedBefore(dttm.Format(now)), assert.False},
+		{"file create before now", dspl, user, sel.CreatedBefore(dttm.Format(now)), assert.False},
-		{"file modified after the epoch", dtsl, user, sel.ModifiedAfter(dttm.Format(epoch)), assert.True},
+		{"file create before modification", dspl, user, sel.CreatedBefore(dttm.Format(mod)), assert.True},
-		{"file modified after now", dtsl, user, sel.ModifiedAfter(dttm.Format(now)), assert.True},
+		{"file create before epoch", dspl, user, sel.CreatedBefore(dttm.Format(now)), assert.False},
-		{"file modified after later", dtsl, user, sel.ModifiedAfter(dttm.Format(future)), assert.False},
+		{"file modified after the epoch", dspl, user, sel.ModifiedAfter(dttm.Format(epoch)), assert.True},
-		{"file modified before future", dtsl, user, sel.ModifiedBefore(dttm.Format(future)), assert.True},
+		{"file modified after now", dspl, user, sel.ModifiedAfter(dttm.Format(now)), assert.True},
-		{"file modified before now", dtsl, user, sel.ModifiedBefore(dttm.Format(now)), assert.False},
+		{"file modified after later", dspl, user, sel.ModifiedAfter(dttm.Format(future)), assert.False},
-		{"file modified before epoch", dtsl, user, sel.ModifiedBefore(dttm.Format(now)), assert.False},
+		{"file modified before future", dspl, user, sel.ModifiedBefore(dttm.Format(future)), assert.True},
-		{"in library", dtsl, user, sel.Library("included-library"), assert.True},
+		{"file modified before now", dspl, user, sel.ModifiedBefore(dttm.Format(now)), assert.False},
-		{"not in library", dtsl, user, sel.Library("not-included-library"), assert.False},
+		{"file modified before epoch", dspl, user, sel.ModifiedBefore(dttm.Format(now)), assert.False},
-		{"library id", dtsl, user, sel.Library("1234"), assert.True},
+		{"in library", dspl, user, sel.Library("included-library"), assert.True},
-		{"not library id", dtsl, user, sel.Library("abcd"), assert.False},
+		{"not in library", dspl, user, sel.Library("not-included-library"), assert.False},
+		{"library id", dspl, user, sel.Library("1234"), assert.True},
+		{"not library id", dspl, user, sel.Library("abcd"), assert.False},
 
-		{"channel message created by", dtch, user, sel.MessageCreator(user), assert.True},
+		{"channel message created by", dgcm, user, sel.MessageCreator(user), assert.True},
-		{"channel message not created by", dtch, user, sel.MessageCreator(host), assert.False},
+		{"channel message not created by", dgcm, user, sel.MessageCreator(host), assert.False},
-		{"chan msg create after the epoch", dtch, user, sel.MessageCreatedAfter(dttm.Format(epoch)), assert.True},
+		{"chan msg create after the epoch", dgcm, user, sel.MessageCreatedAfter(dttm.Format(epoch)), assert.True},
-		{"chan msg create after now", dtch, user, sel.MessageCreatedAfter(dttm.Format(now)), assert.False},
+		{"chan msg create after the epoch wrong type", dspl, user, sel.MessageCreatedAfter(dttm.Format(epoch)), assert.False},
-		{"chan msg create after later", dtch, user, sel.MessageCreatedAfter(dttm.Format(future)), assert.False},
+		{"chan msg create after now", dgcm, user, sel.MessageCreatedAfter(dttm.Format(now)), assert.False},
-		{"chan msg create before future", dtch, user, sel.MessageCreatedBefore(dttm.Format(future)), assert.True},
+		{"chan msg create after later", dgcm, user, sel.MessageCreatedAfter(dttm.Format(future)), assert.False},
-		{"chan msg create before now", dtch, user, sel.MessageCreatedBefore(dttm.Format(now)), assert.False},
+		{"chan msg create before future", dgcm, user, sel.MessageCreatedBefore(dttm.Format(future)), assert.True},
-		{"chan msg create before reply", dtch, user, sel.MessageCreatedBefore(dttm.Format(modification)), assert.True},
+		{"chan msg create before future wrong type", dspl, user, sel.MessageCreatedBefore(dttm.Format(future)), assert.False},
-		{"chan msg create before epoch", dtch, user, sel.MessageCreatedBefore(dttm.Format(now)), assert.False},
+		{"chan msg create before now", dgcm, user, sel.MessageCreatedBefore(dttm.Format(now)), assert.False},
-		{"chan msg last reply after the epoch", dtch, user, sel.MessageLastReplyAfter(dttm.Format(epoch)), assert.True},
+		{"chan msg create before reply", dgcm, user, sel.MessageCreatedBefore(dttm.Format(mod)), assert.True},
-		{"chan msg last reply after now", dtch, user, sel.MessageLastReplyAfter(dttm.Format(now)), assert.True},
+		{"chan msg create before reply wrong type", dspl, user, sel.MessageCreatedBefore(dttm.Format(mod)), assert.False},
-		{"chan msg last reply after later", dtch, user, sel.MessageLastReplyAfter(dttm.Format(future)), assert.False},
+		{"chan msg create before epoch", dgcm, user, sel.MessageCreatedBefore(dttm.Format(now)), assert.False},
-		{"chan msg last reply before future", dtch, user, sel.MessageLastReplyBefore(dttm.Format(future)), assert.True},
+		{"chan msg last reply after the epoch", dgcm, user, sel.MessageLastReplyAfter(dttm.Format(epoch)), assert.True},
-		{"chan msg last reply before now", dtch, user, sel.MessageLastReplyBefore(dttm.Format(now)), assert.False},
+		{"chan msg last reply after now", dgcm, user, sel.MessageLastReplyAfter(dttm.Format(now)), assert.True},
-		{"chan msg last reply before epoch", dtch, user, sel.MessageLastReplyBefore(dttm.Format(now)), assert.False},
+		{"chan msg last reply after later", dgcm, user, sel.MessageLastReplyAfter(dttm.Format(future)), assert.False},
+		{"chan msg last reply before future", dgcm, user, sel.MessageLastReplyBefore(dttm.Format(future)), assert.True},
+		{"chan msg last reply before now", dgcm, user, sel.MessageLastReplyBefore(dttm.Format(now)), assert.False},
+		{"chan msg last reply before epoch", dgcm, user, sel.MessageLastReplyBefore(dttm.Format(now)), assert.False},
 	}
 	for _, test := range table {
 		suite.Run(test.name, func() {
@@ -421,8 +426,8 @@ func (suite *GroupsSelectorSuite) TestGroupsScope_MatchesInfo() {
 					WebURL:         test.creator,
 					MessageCreator: test.creator,
 					Created:        now,
-					Modified:       modification,
+					Modified:       mod,
-					LastReplyAt:    modification,
+					LastReplyAt:    mod,
 					DriveName:      "included-library",
 					DriveID:        "1234",
 				},

src/pkg/services/m365/api/groups.go

@@ -30,16 +30,11 @@ func (c Client) Groups() Groups {
 	return Groups{c}
 }
 
-// On creation of each Teams team a corresponding group gets created.
-// The group acts as the protected resource, and all teams data like events,
-// drive and mail messages are owned by that group.
-
 // Groups is an interface-compliant provider of the client.
 type Groups struct {
 	Client
 }
 
-// GetAllGroups retrieves all groups.
 func (c Groups) GetAll(
 	ctx context.Context,
 	errs *fault.Bus,
@@ -100,7 +95,10 @@ func getGroups(
 
 const filterGroupByDisplayNameQueryTmpl = "displayName eq '%s'"
 
-// GetID retrieves group by groupID.
+// GetID can look up a group by either its canonical id (a uuid)
+// or by the group's display name.  If looking up the display name
+// an error will be returned if more than one group gets returned
+// in the results.
 func (c Groups) GetByID(
 	ctx context.Context,
 	identifier string,
@@ -155,7 +153,6 @@ func (c Groups) GetByID(
 	return group, nil
 }
 
-// GetRootSite retrieves the root site for the group.
 func (c Groups) GetRootSite(
 	ctx context.Context,
 	identifier string,