fix overlapping bases after fallback union (#3118)

In a case where the current manifests and the
fallback contain both distinct and overlapping
categories, the end result contains multiple
bases for the same category.  Verifydistinctresons
didn't catch this because it includes the resource owner.

---

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

- [x]  No

#### Type of change

- [x] 🐛 Bugfix

#### Issue(s)

* #2825

#### Test Plan

- [x]  Unit test
This commit is contained in:
Keepers 2023-04-14 10:29:29 -06:00 committed by GitHub
parent d40830e01e
commit 482b690e06
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 185 additions and 76 deletions

View File

@ -69,6 +69,7 @@ func (gc *GraphConnector) ProduceBackupCollections(
colls, excludes, err := exchange.DataCollections(
ctx,
sels,
sels,
metadata,
gc.credentials,
gc.UpdateStatus,
@ -95,7 +96,9 @@ func (gc *GraphConnector) ProduceBackupCollections(
case selectors.ServiceOneDrive:
colls, excludes, err := onedrive.DataCollections(
ctx,
sels, metadata,
sels,
sels,
metadata,
gc.credentials.AzureTenantID,
gc.itemClient,
gc.Service,

View File

@ -96,9 +96,12 @@ func (suite *DataCollectionIntgSuite) TestExchangeDataCollection() {
suite.Run(test.name, func() {
t := suite.T()
sel := test.getSelector(t)
collections, excludes, err := exchange.DataCollections(
ctx,
test.getSelector(t),
sel,
sel,
nil,
connector.credentials,
connector.UpdateStatus,

View File

@ -6,6 +6,7 @@ import (
"github.com/alcionai/clues"
"github.com/alcionai/corso/src/internal/common"
"github.com/alcionai/corso/src/internal/connector/exchange/api"
"github.com/alcionai/corso/src/internal/connector/graph"
"github.com/alcionai/corso/src/internal/connector/support"
@ -162,6 +163,7 @@ func parseMetadataCollections(
// Add iota to this call -> mail, contacts, calendar, etc.
func DataCollections(
ctx context.Context,
user common.IDNamer,
selector selectors.Selector,
metadata []data.RestoreCollection,
acct account.M365Config,
@ -175,7 +177,6 @@ func DataCollections(
}
var (
user = selector.DiscreteOwner
collections = []data.BackupCollection{}
el = errs.Local()
categories = map[path.CategoryType]struct{}{}
@ -214,7 +215,7 @@ func DataCollections(
baseCols, err := graph.BaseCollections(
ctx,
acct.AzureTenantID,
user,
user.ID(),
path.ExchangeService,
categories,
su,
@ -248,7 +249,7 @@ func getterByType(ac api.Client, category path.CategoryType) (addedAndRemovedIte
func createCollections(
ctx context.Context,
creds account.M365Config,
user string,
user common.IDNamer,
scope selectors.ExchangeScope,
dps DeltaPaths,
ctrlOpts control.Options,

View File

@ -239,6 +239,7 @@ func (suite *DataCollectionsIntegrationSuite) TestMailFetch() {
userID = tester.M365UserID(suite.T())
users = []string{userID}
acct, err = tester.NewM365Account(suite.T()).M365Config()
ss = selectors.Selector{}.SetDiscreteOwnerIDName(userID, userID)
)
require.NoError(suite.T(), err, clues.ToCore(err))
@ -267,7 +268,7 @@ func (suite *DataCollectionsIntegrationSuite) TestMailFetch() {
collections, err := createCollections(
ctx,
acct,
userID,
ss,
test.scope,
DeltaPaths{},
control.Options{},
@ -299,6 +300,7 @@ func (suite *DataCollectionsIntegrationSuite) TestDelta() {
userID = tester.M365UserID(suite.T())
users = []string{userID}
acct, err = tester.NewM365Account(suite.T()).M365Config()
ss = selectors.Selector{}.SetDiscreteOwnerIDName(userID, userID)
)
require.NoError(suite.T(), err, clues.ToCore(err))
@ -337,7 +339,7 @@ func (suite *DataCollectionsIntegrationSuite) TestDelta() {
collections, err := createCollections(
ctx,
acct,
userID,
ss,
test.scope,
DeltaPaths{},
control.Options{},
@ -368,7 +370,7 @@ func (suite *DataCollectionsIntegrationSuite) TestDelta() {
collections, err = createCollections(
ctx,
acct,
userID,
ss,
test.scope,
dps,
control.Options{},
@ -403,6 +405,7 @@ func (suite *DataCollectionsIntegrationSuite) TestMailSerializationRegression()
t = suite.T()
wg sync.WaitGroup
users = []string{suite.user}
ss = selectors.Selector{}.SetDiscreteOwnerIDName(suite.user, suite.user)
)
acct, err := tester.NewM365Account(t).M365Config()
@ -414,7 +417,7 @@ func (suite *DataCollectionsIntegrationSuite) TestMailSerializationRegression()
collections, err := createCollections(
ctx,
acct,
suite.user,
ss,
sel.Scopes()[0],
DeltaPaths{},
control.Options{},
@ -464,6 +467,7 @@ func (suite *DataCollectionsIntegrationSuite) TestContactSerializationRegression
require.NoError(suite.T(), err, clues.ToCore(err))
users := []string{suite.user}
ss := selectors.Selector{}.SetDiscreteOwnerIDName(suite.user, suite.user)
tests := []struct {
name string
@ -487,7 +491,7 @@ func (suite *DataCollectionsIntegrationSuite) TestContactSerializationRegression
edcs, err := createCollections(
ctx,
acct,
suite.user,
ss,
test.scope,
DeltaPaths{},
control.Options{},
@ -552,6 +556,8 @@ func (suite *DataCollectionsIntegrationSuite) TestEventsSerializationRegression(
bdayID string
)
ss := selectors.Selector{}.SetDiscreteOwnerIDName(suite.user, suite.user)
fn := func(gcf graph.CacheFolder) error {
if ptr.Val(gcf.GetDisplayName()) == DefaultCalendar {
calID = ptr.Val(gcf.GetId())
@ -599,7 +605,7 @@ func (suite *DataCollectionsIntegrationSuite) TestEventsSerializationRegression(
collections, err := createCollections(
ctx,
acct,
suite.user,
ss,
test.scope,
DeltaPaths{},
control.Options{},

View File

@ -52,7 +52,7 @@ func PopulateExchangeContainerResolver(
case path.EmailCategory:
acm := ac.Mail()
res = &mailFolderCache{
userID: qp.ResourceOwner,
userID: qp.ResourceOwner.ID(),
getter: acm,
enumer: acm,
}
@ -61,7 +61,7 @@ func PopulateExchangeContainerResolver(
case path.ContactsCategory:
acc := ac.Contacts()
res = &contactFolderCache{
userID: qp.ResourceOwner,
userID: qp.ResourceOwner.ID(),
getter: acc,
enumer: acc,
}
@ -70,7 +70,7 @@ func PopulateExchangeContainerResolver(
case path.EventsCategory:
ecc := ac.Events()
res = &eventCalendarCache{
userID: qp.ResourceOwner,
userID: qp.ResourceOwner.ID(),
getter: ecc,
enumer: ecc,
}
@ -113,7 +113,7 @@ func includeContainer(
dirPath, err := pb.ToDataLayerExchangePathForCategory(
qp.Credentials.AzureTenantID,
qp.ResourceOwner,
qp.ResourceOwner.ID(),
category,
false)
// Containers without a path (e.g. Root mail folder) always err here.
@ -126,7 +126,7 @@ func includeContainer(
if loc != nil {
locPath, err = loc.ToDataLayerExchangePathForCategory(
qp.Credentials.AzureTenantID,
qp.ResourceOwner,
qp.ResourceOwner.ID(),
category,
false)
// Containers without a path (e.g. Root mail folder) always err here.

View File

@ -108,7 +108,7 @@ func filterContainersAndFillCollections(
ictx = clues.Add(ictx, "previous_path", prevPath)
added, removed, newDelta, err := getter.GetAddedAndRemovedItemIDs(ictx, qp.ResourceOwner, cID, prevDelta)
added, removed, newDelta, err := getter.GetAddedAndRemovedItemIDs(ictx, qp.ResourceOwner.ID(), cID, prevDelta)
if err != nil {
if !graph.IsErrDeletedInFlight(err) {
el.AddRecoverable(clues.Stack(err).Label(fault.LabelForceNoBackupCreation))
@ -130,7 +130,7 @@ func filterContainersAndFillCollections(
}
edc := NewCollection(
qp.ResourceOwner,
qp.ResourceOwner.ID(),
currPath,
prevPath,
locPath,
@ -189,7 +189,7 @@ func filterContainersAndFillCollections(
}
edc := NewCollection(
qp.ResourceOwner,
qp.ResourceOwner.ID(),
nil, // marks the collection as deleted
prevPath,
nil, // tombstones don't need a location
@ -208,7 +208,7 @@ func filterContainersAndFillCollections(
col, err := graph.MakeMetadataCollection(
qp.Credentials.AzureTenantID,
qp.ResourceOwner,
qp.ResourceOwner.ID(),
path.ExchangeService,
qp.Category,
[]graph.MetadataCollectionEntry{

View File

@ -116,11 +116,12 @@ func (suite *ServiceIteratorsSuite) SetupSuite() {
}
func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections() {
ss := selectors.Selector{}.SetDiscreteOwnerIDName("user_id", "user_id")
var (
userID = "user_id"
qp = graph.QueryParams{
qp = graph.QueryParams{
Category: path.EmailCategory, // doesn't matter which one we use.
ResourceOwner: userID,
ResourceOwner: ss,
Credentials: suite.creds,
}
statusUpdater = func(*support.ConnectorOperationStatus) {}
@ -435,11 +436,12 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_repea
ctx, flush := tester.NewContext()
defer flush()
ss := selectors.Selector{}.SetDiscreteOwnerIDName("user_id", "user_id")
var (
userID = "user_id"
qp = graph.QueryParams{
qp = graph.QueryParams{
Category: path.EmailCategory, // doesn't matter which one we use.
ResourceOwner: userID,
ResourceOwner: ss,
Credentials: suite.creds,
}
statusUpdater = func(*support.ConnectorOperationStatus) {}
@ -454,6 +456,9 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_repea
resolver = newMockResolver(container1)
)
require.Equal(t, "user_id", qp.ResourceOwner.ID(), qp.ResourceOwner)
require.Equal(t, "user_id", qp.ResourceOwner.Name(), qp.ResourceOwner)
collections := map[string]data.BackupCollection{}
err := filterContainersAndFillCollections(
@ -514,13 +519,15 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_repea
}
func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_incrementals() {
ss := selectors.Selector{}.SetDiscreteOwnerIDName("user_id", "user_id")
var (
userID = "user_id"
tenantID = suite.creds.AzureTenantID
cat = path.EmailCategory // doesn't matter which one we use,
qp = graph.QueryParams{
Category: cat,
ResourceOwner: userID,
ResourceOwner: ss,
Credentials: suite.creds,
}
statusUpdater = func(*support.ConnectorOperationStatus) {}

View File

@ -78,8 +78,7 @@ func MakeMetadataCollection(
resourceOwner,
service,
cat,
false,
)
false)
if err != nil {
return nil, clues.Wrap(err, "making metadata path")
}

View File

@ -12,6 +12,7 @@ import (
msgraphsdkgo "github.com/microsoftgraph/msgraph-sdk-go"
msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core"
"github.com/alcionai/corso/src/internal/common"
"github.com/alcionai/corso/src/pkg/account"
"github.com/alcionai/corso/src/pkg/path"
)
@ -38,7 +39,7 @@ func AllMetadataFileNames() []string {
type QueryParams struct {
Category path.CategoryType
ResourceOwner string
ResourceOwner common.IDNamer
Credentials account.M365Config
}

View File

@ -7,6 +7,7 @@ import (
"github.com/alcionai/clues"
"golang.org/x/exp/maps"
"github.com/alcionai/corso/src/internal/common"
"github.com/alcionai/corso/src/internal/connector/graph"
"github.com/alcionai/corso/src/internal/connector/support"
"github.com/alcionai/corso/src/internal/data"
@ -34,6 +35,7 @@ func (fm odFolderMatcher) Matches(dir string) bool {
func DataCollections(
ctx context.Context,
selector selectors.Selector,
user common.IDNamer,
metadata []data.RestoreCollection,
tenant string,
itemClient *http.Client,
@ -49,7 +51,6 @@ func DataCollections(
var (
el = errs.Local()
user = selector.DiscreteOwner
categories = map[path.CategoryType]struct{}{}
collections = []data.BackupCollection{}
allExcludes = map[string]map[string]struct{}{}
@ -66,7 +67,7 @@ func DataCollections(
nc := NewCollections(
itemClient,
tenant,
user,
user.ID(),
OneDriveSource,
odFolderMatcher{scope},
service,
@ -95,7 +96,7 @@ func DataCollections(
baseCols, err := graph.BaseCollections(
ctx,
tenant,
user,
user.ID(),
path.OneDriveService,
categories,
su,

View File

@ -844,9 +844,11 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_exchangeIncrementals() {
// verify test data was populated, and track it for comparisons
for category, gen := range dataset {
ss := selectors.Selector{}.SetDiscreteOwnerIDName(suite.user, suite.user)
qp := graph.QueryParams{
Category: category,
ResourceOwner: suite.user,
ResourceOwner: ss,
Credentials: m365,
}
cr, err := exchange.PopulateExchangeContainerResolver(ctx, qp, fault.New(true))
@ -958,9 +960,11 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_exchangeIncrementals() {
version.Backup,
gen.dbf)
ss := selectors.Selector{}.SetDiscreteOwnerIDName(suite.user, suite.user)
qp := graph.QueryParams{
Category: category,
ResourceOwner: suite.user,
ResourceOwner: ss,
Credentials: m365,
}
cr, err := exchange.PopulateExchangeContainerResolver(ctx, qp, fault.New(true))

View File

@ -93,20 +93,6 @@ func produceManifestsAndMetadata(
return ms, nil, false, nil
}
// We only need to check that we have 1:1 reason:base if we're doing an
// incremental with associated metadata. This ensures that we're only sourcing
// data from a single Point-In-Time (base) for each incremental backup.
//
// TODO(ashmrtn): This may need updating if we start sourcing item backup
// details from previous snapshots when using kopia-assisted incrementals.
if err := verifyDistinctBases(ctx, ms); err != nil {
logger.Ctx(ctx).With("error", err).Infow(
"unioned snapshot collision, falling back to full backup",
clues.In(ctx).Slice()...)
return ms, nil, false, nil
}
for _, man := range ms {
if len(man.IncompleteReason) > 0 {
continue
@ -234,6 +220,8 @@ func unionManifests(
// backfill from the fallback where necessary
for _, m := range fallback {
useReasons := []kopia.Reason{}
for _, r := range m.Reasons {
k := r.Service.String() + r.Category.String()
t := tups[k]
@ -245,6 +233,8 @@ func unionManifests(
continue
}
useReasons = append(useReasons, r)
if len(m.IncompleteReason) > 0 && t.incomplete == nil {
t.incomplete = m
} else if len(m.IncompleteReason) == 0 {
@ -253,6 +243,10 @@ func unionManifests(
tups[k] = t
}
if len(m.IncompleteReason) == 0 && len(useReasons) > 0 {
m.Reasons = useReasons
}
}
// collect the results into a single slice of manifests

View File

@ -757,18 +757,20 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
table := []struct {
name string
main []testInput
man []testInput
fallback []testInput
reasons []kopia.Reason
fallbackReasons []kopia.Reason
categories []path.CategoryType
manCategories []path.CategoryType
fbCategories []path.CategoryType
assertErr assert.ErrorAssertionFunc
expectManIDs []string
expectNilMans bool
expectReasons map[string][]path.CategoryType
}{
{
name: "only mans, no fallbacks",
main: []testInput{
man: []testInput{
{
id: manComplete,
},
@ -777,8 +779,13 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
incomplete: true,
},
},
categories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete, manIncomplete},
manCategories: []path.CategoryType{path.EmailCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete, manIncomplete},
expectReasons: map[string][]path.CategoryType{
manComplete: {path.EmailCategory},
manIncomplete: {path.EmailCategory},
},
},
{
name: "no mans, only fallbacks",
@ -791,12 +798,17 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
incomplete: true,
},
},
categories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{fbComplete, fbIncomplete},
manCategories: []path.CategoryType{path.EmailCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{fbComplete, fbIncomplete},
expectReasons: map[string][]path.CategoryType{
fbComplete: {path.EmailCategory},
fbIncomplete: {path.EmailCategory},
},
},
{
name: "complete mans and fallbacks",
main: []testInput{
man: []testInput{
{
id: manComplete,
},
@ -806,12 +818,16 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
id: fbComplete,
},
},
categories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete},
manCategories: []path.CategoryType{path.EmailCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete},
expectReasons: map[string][]path.CategoryType{
manComplete: {path.EmailCategory},
},
},
{
name: "incomplete mans and fallbacks",
main: []testInput{
man: []testInput{
{
id: manIncomplete,
incomplete: true,
@ -823,12 +839,16 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
incomplete: true,
},
},
categories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manIncomplete},
manCategories: []path.CategoryType{path.EmailCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manIncomplete},
expectReasons: map[string][]path.CategoryType{
manIncomplete: {path.EmailCategory},
},
},
{
name: "complete and incomplete mans and fallbacks",
main: []testInput{
man: []testInput{
{
id: manComplete,
},
@ -846,12 +866,17 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
incomplete: true,
},
},
categories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete, manIncomplete},
manCategories: []path.CategoryType{path.EmailCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete, manIncomplete},
expectReasons: map[string][]path.CategoryType{
manComplete: {path.EmailCategory},
manIncomplete: {path.EmailCategory},
},
},
{
name: "incomplete mans, complete fallbacks",
main: []testInput{
man: []testInput{
{
id: manIncomplete,
incomplete: true,
@ -862,12 +887,17 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
id: fbComplete,
},
},
categories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{fbComplete, manIncomplete},
manCategories: []path.CategoryType{path.EmailCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{fbComplete, manIncomplete},
expectReasons: map[string][]path.CategoryType{
fbComplete: {path.EmailCategory},
manIncomplete: {path.EmailCategory},
},
},
{
name: "complete mans, incomplete fallbacks",
main: []testInput{
man: []testInput{
{
id: manComplete,
},
@ -878,12 +908,16 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
incomplete: true,
},
},
categories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete},
manCategories: []path.CategoryType{path.EmailCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete},
expectReasons: map[string][]path.CategoryType{
manComplete: {path.EmailCategory},
},
},
{
name: "complete mans, complete fallbacks, multiple reasons",
main: []testInput{
man: []testInput{
{
id: manComplete,
},
@ -893,8 +927,52 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
id: fbComplete,
},
},
categories: []path.CategoryType{path.EmailCategory, path.ContactsCategory},
expectManIDs: []string{manComplete},
manCategories: []path.CategoryType{path.EmailCategory, path.ContactsCategory},
fbCategories: []path.CategoryType{path.EmailCategory, path.ContactsCategory},
expectManIDs: []string{manComplete},
expectReasons: map[string][]path.CategoryType{
manComplete: {path.EmailCategory, path.ContactsCategory},
},
},
{
name: "complete mans, complete fallbacks, distinct reasons",
man: []testInput{
{
id: manComplete,
},
},
fallback: []testInput{
{
id: fbComplete,
},
},
manCategories: []path.CategoryType{path.ContactsCategory},
fbCategories: []path.CategoryType{path.EmailCategory},
expectManIDs: []string{manComplete, fbComplete},
expectReasons: map[string][]path.CategoryType{
manComplete: {path.ContactsCategory},
fbComplete: {path.EmailCategory},
},
},
{
name: "fb has superset of mans reasons",
man: []testInput{
{
id: manComplete,
},
},
fallback: []testInput{
{
id: fbComplete,
},
},
manCategories: []path.CategoryType{path.ContactsCategory},
fbCategories: []path.CategoryType{path.EmailCategory, path.ContactsCategory, path.EventsCategory},
expectManIDs: []string{manComplete, fbComplete},
expectReasons: map[string][]path.CategoryType{
manComplete: {path.ContactsCategory},
fbComplete: {path.EmailCategory, path.EventsCategory},
},
},
}
for _, test := range table {
@ -907,7 +985,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
mainReasons := []kopia.Reason{}
fbReasons := []kopia.Reason{}
for _, cat := range test.categories {
for _, cat := range test.manCategories {
mainReasons = append(
mainReasons,
kopia.Reason{
@ -915,7 +993,9 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
Service: path.ExchangeService,
Category: cat,
})
}
for _, cat := range test.fbCategories {
fbReasons = append(
fbReasons,
kopia.Reason{
@ -927,7 +1007,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
mans := []*kopia.ManifestEntry{}
for _, m := range test.main {
for _, m := range test.man {
incomplete := ""
if m.incomplete {
incomplete = "ir"
@ -959,8 +1039,18 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
assert.False(t, b, "no-metadata is forced for this test")
manIDs := []string{}
for _, m := range mans {
manIDs = append(manIDs, string(m.ID))
reasons := test.expectReasons[string(m.ID)]
mrs := []path.CategoryType{}
for _, r := range m.Reasons {
mrs = append(mrs, r.Category)
}
assert.ElementsMatch(t, reasons, mrs)
}
assert.ElementsMatch(t, test.expectManIDs, manIDs)