Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parses metadata collections using previous paths #4947

Merged
merged 5 commits into from
Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/internal/m365/collection/site/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func CollectLists(
su support.StatusUpdater,
counter *count.Bus,
errs *fault.Bus,
) ([]data.BackupCollection, error) {
) ([]data.BackupCollection, bool, error) {
logger.Ctx(ctx).Debug("Creating SharePoint List Collections")

var (
Expand All @@ -160,9 +160,19 @@ func CollectLists(
currPaths = map[string]string{}
)

dps, canUsePreviousBackup, err := parseMetadataCollections(ctx, path.ListsCategory, bpc.MetadataCollections)
if err != nil {
return nil, false, err
}

// [TODO] utilise deltapaths to determine list's state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We use // TODO(hiteshrepo): comment as a convention for marking todos in code.

_ = dps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You could do this in line 163 itself i.e. _, canUsePreviousBackup, err


ctx = clues.Add(ctx, "can_use_previous_backup", canUsePreviousBackup)

lists, err := bh.GetItems(ctx, acc)
if err != nil {
return nil, err
return nil, false, err
}

for _, list := range lists {
Expand Down Expand Up @@ -204,7 +214,7 @@ func CollectLists(
path.ListsCategory,
false)
if err != nil {
return nil, clues.WrapWC(ctx, err, "making metadata path prefix").
return nil, false, clues.WrapWC(ctx, err, "making metadata path prefix").
Label(count.BadPathPrefix)
}

Expand All @@ -216,12 +226,12 @@ func CollectLists(
su,
counter.Local())
if err != nil {
return nil, clues.WrapWC(ctx, err, "making metadata collection")
return nil, false, clues.WrapWC(ctx, err, "making metadata collection")
}

spcs = append(spcs, col)

return spcs, el.Failure()
return spcs, canUsePreviousBackup, el.Failure()
}

func idAnd(ss ...string) []string {
Expand Down
2 changes: 1 addition & 1 deletion src/internal/m365/collection/site/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (suite *SharePointSuite) TestCollectLists() {

bh := NewListsBackupHandler(bpc.ProtectedResource.ID(), ac.Lists())

col, err := CollectLists(
col, _, err := CollectLists(
ctx,
bh,
bpc,
Expand Down
99 changes: 99 additions & 0 deletions src/internal/m365/collection/site/metadata.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package site

import (
"context"
"encoding/json"

"github.com/alcionai/clues"

"github.com/alcionai/corso/src/internal/data"
"github.com/alcionai/corso/src/pkg/backup/metadata"
"github.com/alcionai/corso/src/pkg/fault"
"github.com/alcionai/corso/src/pkg/logger"
"github.com/alcionai/corso/src/pkg/path"
)

func parseMetadataCollections(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is missing a unit test. You can use TestParseMetadataCollections as a reference.

  2. Also, should we make this function list specific? Maybe call the file list_metadata.go as well. Reasons:

  • It can be confusing since it comes off as a generic func used for all sites categories. But it's only being used for lists.
  • It doesn't handle delta file paths, which is a list behavior. Might not be true for other sites categories. Please correct me if wrong.
  1. Could you please also file a tech debt issue to make parseMetadataCollections a generic function which works for all services? Right now this code is duplicated everywhere, but most of it is common for all services. We don't need to fix the tech debt right away, but it'd be good to track it as an issue. Let me know if you disagree :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pandeyabs

  1. Yeah sure I will write the unit test for it.
  2. I thought, when sharepoint page is being implemented, that would to take care of dividing/unifying the function. But lets do what you suggested. In that case while page is being implemented, we have to consider unifying the logic based on delta path availability for page.
  3. Yeah. I will assimilate tech-debts arising from PRs in the notion page momentarily and then move them to appropriate location (GH/linear).

ctx context.Context,
cat path.CategoryType,
colls []data.RestoreCollection,
) (metadata.DeltaPaths, bool, error) {
cdp := metadata.CatDeltaPaths{
cat: {},
}

found := map[path.CategoryType]map[string]struct{}{
cat: {},
}

errs := fault.New(true)

for _, coll := range colls {
var (
breakLoop bool
items = coll.Items(ctx, errs)
category = coll.FullPath().Category()
)

for {
select {
case <-ctx.Done():
return nil, false, clues.WrapWC(ctx, ctx.Err(), "parsing collection metadata")

case item, ok := <-items:
if !ok || errs.Failure() != nil {
breakLoop = true
break
}

var (
m = map[string]string{}
cdps, wantedCategory = cdp[category]
)

if !wantedCategory {
continue
}

err := json.NewDecoder(item.ToReader()).Decode(&m)
if err != nil {
return nil, false, clues.NewWC(ctx, "decoding metadata json")
}

if item.ID() == metadata.PreviousPathFileName {
if _, ok := found[category][metadata.PathKey]; ok {
return nil, false, clues.Wrap(clues.NewWC(ctx, category.String()), "multiple versions of path metadata")
}

for k, p := range m {
cdps.AddPath(k, p)
}

found[category][metadata.PathKey] = struct{}{}

cdp[category] = cdps
}
}

if breakLoop {
break
}
}
}

if errs.Failure() != nil {
logger.CtxErr(ctx, errs.Failure()).Info("reading metadata collection items")

return metadata.DeltaPaths{}, false, nil
}

for _, dps := range cdp {
for k, dp := range dps {
if len(dp.Path) == 0 {
delete(dps, k)
}
}
}

return cdp[cat], true, nil
}
6 changes: 1 addition & 5 deletions src/internal/m365/service/sharepoint/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func ProduceBackupCollections(
case path.ListsCategory:
bh := site.NewListsBackupHandler(bpc.ProtectedResource.ID(), ac.Lists())

spcs, err = site.CollectLists(
spcs, canUsePreviousBackup, err = site.CollectLists(
ctx,
bh,
bpc,
Expand All @@ -73,10 +73,6 @@ func ProduceBackupCollections(
continue
}

// Lists don't make use of previous metadata
// TODO: Revisit when we add support of lists
canUsePreviousBackup = true

case path.LibrariesCategory:
spcs, canUsePreviousBackup, err = site.CollectLibraries(
ctx,
Expand Down