From aa66675b830e9e64c4669e757f01e8984e62eb39 Mon Sep 17 00:00:00 2001 From: Keepers Date: Mon, 9 Oct 2023 19:25:14 -0600 Subject: [PATCH] remove local bus struct (#4435) removes the local bus from fault in favor of a single Bus that operates both within a local and global instance, and can be passed downstream independent of context. Also includes some code separation in the fault package for readability. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :broom: Tech Debt/Cleanup #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/pkg/fault/alert.go | 70 +++++++ src/pkg/fault/alert_test.go | 88 +++++++++ src/pkg/fault/example_fault_test.go | 25 +++ src/pkg/fault/fault.go | 297 ++++++++++++++-------------- src/pkg/fault/fault_test.go | 19 -- src/pkg/fault/item.go | 194 +----------------- src/pkg/fault/item_test.go | 229 +++------------------ src/pkg/fault/skipped.go | 117 +++++++++++ src/pkg/fault/skipped_test.go | 146 ++++++++++++++ 9 files changed, 632 insertions(+), 553 deletions(-) create mode 100644 src/pkg/fault/alert.go create mode 100644 src/pkg/fault/alert_test.go create mode 100644 src/pkg/fault/skipped.go create mode 100644 src/pkg/fault/skipped_test.go diff --git a/src/pkg/fault/alert.go b/src/pkg/fault/alert.go new file mode 100644 index 0000000000..5d4c97cea0 --- /dev/null +++ b/src/pkg/fault/alert.go @@ -0,0 +1,70 @@ +package fault + +import ( + "github.com/alcionai/corso/src/cli/print" +) + +var _ print.Printable = &Alert{} + +// Alerts are informational-only notifications. The purpose of alerts is to +// provide a means of end-user communication about important events without +// needing to generate runtime failures or recoverable errors. When generating +// an alert, no other fault feature (failure, recoverable, skip, etc) should +// be in use. IE: Errors do not also get alerts, since the error itself is a +// form of end-user communication already. +type Alert struct { + Item Item `json:"item"` + Message string `json:"message"` +} + +// String complies with the stringer interface. +func (a *Alert) String() string { + msg := "" + + if a != nil { + msg = a.Message + } + + if len(msg) == 0 { + msg = "" + } + + return "Alert: " + msg +} + +func (a Alert) MinimumPrintable() any { + return a +} + +// Headers returns the human-readable names of properties of a skipped Item +// for printing out to a terminal. +func (a Alert) Headers() []string { + return []string{"Action", "Message", "Container", "Name", "ID"} +} + +// Values populates the printable values matching the Headers list. +func (a Alert) Values() []string { + var cn string + + acn, ok := a.Item.Additional[AddtlContainerName] + if ok { + str, ok := acn.(string) + if ok { + cn = str + } + } + + return []string{"Alert", a.Message, cn, a.Item.Name, a.Item.ID} +} + +func NewAlert(message, namespace, itemID, name string, addtl map[string]any) *Alert { + return &Alert{ + Message: message, + Item: Item{ + Namespace: namespace, + ID: itemID, + Name: name, + Additional: addtl, + }, + } +} diff --git a/src/pkg/fault/alert_test.go b/src/pkg/fault/alert_test.go new file mode 100644 index 0000000000..c45ec2e707 --- /dev/null +++ b/src/pkg/fault/alert_test.go @@ -0,0 +1,88 @@ +package fault_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" + + "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/fault" +) + +type AlertUnitSuite struct { + tester.Suite +} + +func TestAlertUnitSuite(t *testing.T) { + suite.Run(t, &AlertUnitSuite{Suite: tester.NewUnitSuite(t)}) +} + +func (suite *AlertUnitSuite) TestAlert_String() { + var ( + t = suite.T() + a fault.Alert + ) + + assert.Contains(t, a.String(), "Alert: ") + + a = fault.Alert{ + Item: fault.Item{}, + Message: "", + } + assert.Contains(t, a.String(), "Alert: ") + + a = fault.Alert{ + Item: fault.Item{ + ID: "item_id", + }, + Message: "msg", + } + assert.NotContains(t, a.String(), "item_id") + assert.Contains(t, a.String(), "Alert: msg") +} + +func (suite *AlertUnitSuite) TestNewAlert() { + t := suite.T() + addtl := map[string]any{"foo": "bar"} + a := fault.NewAlert("message-to-show", "ns", "item_id", "item_name", addtl) + + expect := fault.Alert{ + Item: fault.Item{ + Namespace: "ns", + ID: "item_id", + Name: "item_name", + Additional: addtl, + }, + Message: "message-to-show", + } + + assert.Equal(t, expect, *a) +} + +func (suite *AlertUnitSuite) TestAlert_HeadersValues() { + addtl := map[string]any{ + fault.AddtlContainerID: "cid", + fault.AddtlContainerName: "cname", + } + + table := []struct { + name string + alert *fault.Alert + expect []string + }{ + { + name: "new alert", + alert: fault.NewAlert("message-to-show", "ns", "id", "name", addtl), + expect: []string{"Alert", "message-to-show", "cname", "name", "id"}, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + assert.Equal(t, []string{"Action", "Message", "Container", "Name", "ID"}, test.alert.Headers()) + assert.Equal(t, test.expect, test.alert.Values()) + }) + } +} diff --git a/src/pkg/fault/example_fault_test.go b/src/pkg/fault/example_fault_test.go index e272655b60..90c93e0837 100644 --- a/src/pkg/fault/example_fault_test.go +++ b/src/pkg/fault/example_fault_test.go @@ -441,3 +441,28 @@ func ExampleBus_AddSkip() { // Output: skipped processing file: malware_detected } + +// ExampleBus_AddAlert showcases when to use AddAlert. +func ExampleBus_AddAlert() { + errs := fault.New(false) + + // Some events should be communicated to the end user without recording an + // error to the operation. Logs aren't sufficient because we don't promote + // log messages to the terminal. But errors and skips are too heavy and hacky + // to use. In these cases, we can create informational Alerts. + // + // Only the message gets shown to the user. But since we're persisting this + // data along with the backup details and other fault info, we have the option + // of packing any other contextual data that we want. + errs.AddAlert(ctx, fault.NewAlert( + "something important happened!", + "deduplication-namespace", + "file-id", + "file-name", + map[string]any{"foo": "bar"})) + + // later on, after processing, end users can scrutinize the alerts. + fmt.Println(errs.Alerts()[0].String()) + + // Alert: something important happened! +} diff --git a/src/pkg/fault/fault.go b/src/pkg/fault/fault.go index f5277f4a0d..e6ea1bcd9b 100644 --- a/src/pkg/fault/fault.go +++ b/src/pkg/fault/fault.go @@ -15,11 +15,24 @@ import ( "github.com/alcionai/corso/src/pkg/logger" ) +// temporary hack identifier +// see: https://github.com/alcionai/corso/pull/2510#discussion_r1113532530 +// TODO: https://github.com/alcionai/corso/issues/4003 +const LabelForceNoBackupCreation = "label_forces_no_backup_creations" + type Bus struct { mu *sync.Mutex + // When creating a local bus, the parent property retains a pointer + // to the root Bus. Even in the case of multiple chained creations of + // local busses, the parent reference remains the original root bus, + // and does not create a linked list of lineage. Any errors and failures + // created by a local instance will get fielded to the parent. But only + // local errors will returned by property getter funcs. + parent *Bus + // Failure probably identifies errors that were added to the bus - // or localBus via AddRecoverable, but which were promoted + // or a local Bus via AddRecoverable, but which were promoted // to the failure position due to failFast=true configuration. // Alternatively, the process controller might have set failure // by calling Fail(err). @@ -58,67 +71,61 @@ func New(failFast bool) *Bus { } } -// FailFast returns the failFast flag in the bus. -func (e *Bus) FailFast() bool { - return e.failFast -} +// Local constructs a new bus with a local reference to handle error aggregation +// in a constrained scope. This allows the caller to review recoverable errors and +// failures within only the current codespace, as opposed to the global set of errors. +// The function that spawned the local bus should always return `bus.Failure()` to +// ensure that hard failures are propagated back upstream. +func (e *Bus) Local() *Bus { + parent := e.parent -// Failure returns the primary error. If not nil, this -// indicates the operation exited prior to completion. -func (e *Bus) Failure() error { - return e.failure -} - -// Recovered returns the slice of errors that occurred in -// recoverable points of processing. This is often during -// iteration where a single failure (ex: retrieving an item), -// doesn't require the entire process to end. -func (e *Bus) Recovered() []error { - return slices.Clone(e.recoverable) -} + // only use e if it is already the root instance + if parent == nil { + parent = e + } -// Skipped returns the slice of items that were permanently -// skipped during processing. -func (e *Bus) Skipped() []Skipped { - return slices.Clone(e.skipped) + return &Bus{ + mu: &sync.Mutex{}, + parent: parent, + failFast: parent.failFast, + } } -// Alerts returns the slice of alerts generated during runtime. -func (e *Bus) Alerts() []Alert { - return slices.Clone(e.alerts) +// FailFast returns the failFast flag in the bus. +func (e *Bus) FailFast() bool { + return e.failFast } // Fail sets the non-recoverable error (ie: bus.failure) // in the bus. If a failure error is already present, // the error gets added to the recoverable slice for // purposes of tracking. -// -// TODO: Return Data, not Bus. The consumers of a failure -// should care about the state of data, not the communication -// pattern. func (e *Bus) Fail(err error) *Bus { if err == nil { return e } - e.mu.Lock() - defer e.mu.Unlock() - return e.setFailure(err) } // setErr handles setting bus.failure. Sync locking gets // handled upstream of this call. func (e *Bus) setFailure(err error) *Bus { + e.mu.Lock() + defer e.mu.Unlock() + if e.failure == nil { e.failure = err - return e + } else { + // technically not a recoverable error: we're using the + // recoverable slice as an overflow container here to + // ensure everything is tracked. + e.recoverable = append(e.recoverable, err) } - // technically not a recoverable error: we're using the - // recoverable slice as an overflow container here to - // ensure everything is tracked. - e.recoverable = append(e.recoverable, err) + if e.parent != nil { + e.parent.setFailure(err) + } return e } @@ -127,17 +134,11 @@ func (e *Bus) setFailure(err error) *Bus { // errors (ie: bus.recoverable). If failFast is true, the first // added error will get copied to bus.failure, causing the bus // to identify as non-recoverably failed. -// -// TODO: nil return, not Bus, since we don't want people to return -// from errors.AddRecoverable(). func (e *Bus) AddRecoverable(ctx context.Context, err error) { if err == nil { return } - e.mu.Lock() - defer e.mu.Unlock() - e.logAndAddRecoverable(ctx, err, 1) } @@ -158,19 +159,77 @@ func (e *Bus) logAndAddRecoverable(ctx context.Context, err error, skip int) { // gets handled upstream of this call. Returns true if the // error is a failure, false otherwise. func (e *Bus) addRecoverableErr(err error) bool { + e.mu.Lock() + defer e.mu.Unlock() + var isFail bool if e.failure == nil && e.failFast { - e.setFailure(err) + if e.failure == nil { + e.failure = err + } else { + // technically not a recoverable error: we're using the + // recoverable slice as an overflow container here to + // ensure everything is tracked. + e.recoverable = append(e.recoverable, err) + } + + if e.parent != nil { + e.parent.setFailure(err) + } isFail = true } e.recoverable = append(e.recoverable, err) + // local bus instances must promote errors to the root bus. + if e.parent != nil { + e.parent.addRecoverableErr(err) + } + return isFail } +// --------------------------------------------------------------------------- +// Non-error adders +// --------------------------------------------------------------------------- + +// AddAlert appends a record of an Alert message to the fault bus. +// Importantly, alerts are not errors, exceptions, or skipped items. +// An alert should only be generated if no other fault functionality +// is in use, but that we still want the end user to clearly and +// plainly receive a notification about a runtime event. +func (e *Bus) AddAlert(ctx context.Context, a *Alert) { + if a == nil { + return + } + + e.logAndAddAlert(ctx, a, 1) +} + +// logs the error and adds an alert. +func (e *Bus) logAndAddAlert(ctx context.Context, a *Alert, trace int) { + logger.CtxStack(ctx, trace+1). + With("alert", a). + Info("alert: " + a.Message) + e.addAlert(a) +} + +func (e *Bus) addAlert(a *Alert) *Bus { + e.mu.Lock() + defer e.mu.Unlock() + + e.alerts = append(e.alerts, *a) + + // local bus instances must promote alerts to the root bus. + if e.parent != nil { + e.parent.addAlert(a) + } + + return e +} + // AddSkip appends a record of a Skipped item to the fault bus. // Importantly, skipped items are not the same as recoverable // errors. An item should only be skipped under the following @@ -186,9 +245,6 @@ func (e *Bus) AddSkip(ctx context.Context, s *Skipped) { return } - e.mu.Lock() - defer e.mu.Unlock() - e.logAndAddSkip(ctx, s, 1) } @@ -196,44 +252,28 @@ func (e *Bus) AddSkip(ctx context.Context, s *Skipped) { func (e *Bus) logAndAddSkip(ctx context.Context, s *Skipped, trace int) { logger.CtxStack(ctx, trace+1). With("skipped", s). - Info("skipped item") + Info("skipped an item") e.addSkip(s) } func (e *Bus) addSkip(s *Skipped) *Bus { - e.skipped = append(e.skipped, *s) - return e -} - -// AddAlert appends a record of an Alert message to the fault bus. -// Importantly, alerts are not errors, exceptions, or skipped items. -// An alert should only be generated if no other fault functionality -// is in use, but that we still want the end user to clearly and -// plainly receive a notification about a runtime event. -func (e *Bus) AddAlert(ctx context.Context, a *Alert) { - if a == nil { - return - } - e.mu.Lock() defer e.mu.Unlock() - e.logAndAddAlert(ctx, a, 1) -} + e.skipped = append(e.skipped, *s) -// logs the error and adds an alert. -func (e *Bus) logAndAddAlert(ctx context.Context, a *Alert, trace int) { - logger.CtxStack(ctx, trace+1). - With("alert", a). - Info("alert: " + a.Message) - e.addAlert(a) -} + // local bus instances must promote skipped items to the root bus. + if e.parent != nil { + e.parent.addSkip(s) + } -func (e *Bus) addAlert(a *Alert) *Bus { - e.alerts = append(e.alerts, *a) return e } +// --------------------------------------------------------------------------- +// Results +// --------------------------------------------------------------------------- + // Errors returns the plain record of errors that were aggregated // within a fult Bus. func (e *Bus) Errors() *Errors { @@ -249,6 +289,39 @@ func (e *Bus) Errors() *Errors { } } +// Failure returns the primary error. If not nil, this +// indicates the operation exited prior to completion. +// If the bus is a local instance, this only returns the +// local failure, and will not return parent data. +func (e *Bus) Failure() error { + return e.failure +} + +// Recovered returns the slice of errors that occurred in +// recoverable points of processing. This is often during +// iteration where a single failure (ex: retrieving an item), +// doesn't require the entire process to end. +// If the bus is a local instance, this only returns the +// local recovered errors, and will not return parent data. +func (e *Bus) Recovered() []error { + return slices.Clone(e.recoverable) +} + +// Skipped returns the slice of items that were permanently +// skipped during processing. +// If the bus is a local instance, this only returns the +// local skipped items, and will not return parent data. +func (e *Bus) Skipped() []Skipped { + return slices.Clone(e.skipped) +} + +// Alerts returns the slice of alerts generated during runtime. +// If the bus is a local alerts, this only returns the +// local failure, and will not return parent data. +func (e *Bus) Alerts() []Alert { + return slices.Clone(e.alerts) +} + // ItemsAndRecovered returns the items that failed along with other // recoverable errors func (e *Bus) ItemsAndRecovered() ([]Item, []error) { @@ -275,10 +348,6 @@ func (e *Bus) ItemsAndRecovered() ([]Item, []error) { return maps.Values(is), non } -// --------------------------------------------------------------------------- -// Errors Data -// --------------------------------------------------------------------------- - // Errors provides the errors data alone, without sync controls // or adders/setters. Expected to get called at the end of processing, // as a way to aggregate results. @@ -360,6 +429,10 @@ func UnmarshalErrorsTo(e *Errors) func(io.ReadCloser) error { } } +// --------------------------------------------------------------------------- +// Print compatibility +// --------------------------------------------------------------------------- + // Print writes the DetailModel Entries to StdOut, in the format // requested by the caller. func (e *Errors) PrintItems( @@ -430,73 +503,3 @@ func (pec printableErrCore) Values() []string { return []string{pec.Msg} } - -// --------------------------------------------------------------------------- -// Local aggregator -// --------------------------------------------------------------------------- - -// Local constructs a new local bus to handle error aggregation in a -// constrained scope. Local busses shouldn't be passed down to other -// funcs, and the function that spawned the local bus should always -// return `local.Failure()` to ensure that hard failures are propagated -// back upstream. -func (e *Bus) Local() *LocalBus { - return &LocalBus{ - mu: &sync.Mutex{}, - bus: e, - } -} - -type LocalBus struct { - mu *sync.Mutex - bus *Bus - current error -} - -func (e *LocalBus) AddRecoverable(ctx context.Context, err error) { - if err == nil { - return - } - - e.mu.Lock() - defer e.mu.Unlock() - - if e.current == nil && e.bus.failFast { - e.current = err - } - - e.bus.logAndAddRecoverable(ctx, err, 1) -} - -// AddSkip appends a record of a Skipped item to the local bus. -// Importantly, skipped items are not the same as recoverable -// errors. An item should only be skipped under the following -// conditions. All other cases should be handled as errors. -// 1. The conditions for skipping the item are well-known and -// well-documented. End users need to be able to understand -// both the conditions and identifications of skips. -// 2. Skipping avoids a permanent and consistent failure. If -// the underlying reason is transient or otherwise recoverable, -// the item should not be skipped. -func (e *LocalBus) AddSkip(ctx context.Context, s *Skipped) { - if s == nil { - return - } - - e.mu.Lock() - defer e.mu.Unlock() - - e.bus.logAndAddSkip(ctx, s, 1) -} - -// Failure returns the failure that happened within the local bus. -// It does not return the underlying bus.Failure(), only the failure -// that was recorded within the local bus instance. This error should -// get returned by any func which created a local bus. -func (e *LocalBus) Failure() error { - return e.current -} - -// temporary hack identifier -// see: https://github.com/alcionai/corso/pull/2510#discussion_r1113532530 -const LabelForceNoBackupCreation = "label_forces_no_backup_creations" diff --git a/src/pkg/fault/fault_test.go b/src/pkg/fault/fault_test.go index c4166456b8..d7fd79f281 100644 --- a/src/pkg/fault/fault_test.go +++ b/src/pkg/fault/fault_test.go @@ -189,25 +189,6 @@ func (suite *FaultErrorsUnitSuite) TestAdd() { assert.Len(t, n.Recovered(), 2) } -func (suite *FaultErrorsUnitSuite) TestAddSkip() { - t := suite.T() - - ctx, flush := tester.NewContext(t) - defer flush() - - n := fault.New(true) - require.NotNil(t, n) - - n.Fail(assert.AnError) - assert.Len(t, n.Skipped(), 0) - - n.AddRecoverable(ctx, assert.AnError) - assert.Len(t, n.Skipped(), 0) - - n.AddSkip(ctx, fault.OwnerSkip(fault.SkipMalware, "ns", "id", "name", nil)) - assert.Len(t, n.Skipped(), 1) -} - func (suite *FaultErrorsUnitSuite) TestErrors() { t := suite.T() diff --git a/src/pkg/fault/item.go b/src/pkg/fault/item.go index 7275c24a62..e43070ebe2 100644 --- a/src/pkg/fault/item.go +++ b/src/pkg/fault/item.go @@ -11,15 +11,15 @@ const ( AddtlMalwareDesc = "malware_description" ) -type itemType string +type ItemType string const ( - FileType itemType = "file" - ContainerType itemType = "container" - ResourceOwnerType itemType = "resource_owner" + FileType ItemType = "file" + ContainerType ItemType = "container" + ResourceOwnerType ItemType = "resource_owner" ) -func (it itemType) Printable() string { +func (it ItemType) Printable() string { switch it { case FileType: return "File" @@ -62,7 +62,7 @@ type Item struct { Name string `json:"name"` // tracks the type of item represented by this entry. - Type itemType `json:"type"` + Type ItemType `json:"type"` // Error() of the causal error, or a sentinel if this is the // source of the error. In case of ID collisions, the first @@ -138,7 +138,7 @@ func OwnerErr(cause error, namespace, id, name string, addtl map[string]any) *It } // itemErr produces a Item of the provided type for tracking erroneous items. -func itemErr(t itemType, cause error, namespace, id, name string, addtl map[string]any) *Item { +func itemErr(t ItemType, cause error, namespace, id, name string, addtl map[string]any) *Item { return &Item{ Namespace: namespace, ID: id, @@ -148,183 +148,3 @@ func itemErr(t itemType, cause error, namespace, id, name string, addtl map[stri Additional: addtl, } } - -// --------------------------------------------------------------------------- -// Skipped Items -// --------------------------------------------------------------------------- - -// skipCause identifies the well-known conditions to Skip an item. It is -// important that skip cause enumerations do not overlap with general error -// handling. Skips must be well known, well documented, and consistent. -// Transient failures, undocumented or unknown conditions, and arbitrary -// handling should never produce a skipped item. Those cases should get -// handled as normal errors. -type skipCause string - -const ( - // SkipMalware identifies a malware detection case. Files that graph - // api identifies as malware cannot be downloaded or uploaded, and will - // permanently fail any attempts to backup or restore. - SkipMalware skipCause = "malware_detected" - - // SkipBigOneNote identifies that a file was skipped because it - // was big OneNote file and we can only download OneNote files which - // are less that 2GB in size. - //nolint:lll - // https://support.microsoft.com/en-us/office/restrictions-and-limitations-in-onedrive-and-sharepoint-64883a5d-228e-48f5-b3d2-eb39e07630fa#onenotenotebooks - SkipBigOneNote skipCause = "big_one_note_file" -) - -var _ print.Printable = &Skipped{} - -// Skipped items are permanently unprocessable due to well-known conditions. -// In order to skip an item, the following conditions should be met: -// 1. The conditions for skipping the item are well-known and -// well-documented. End users need to be able to understand -// both the conditions and identifications of skips. -// 2. Skipping avoids a permanent and consistent failure. If -// the underlying reason is transient or otherwise recoverable, -// the item should not be skipped. -// -// Skipped wraps Item primarily to minimize confusion when sharing the -// fault interface. Skipped items are not errors, and Item{} errors are -// not the basis for a Skip. -type Skipped struct { - Item Item `json:"item"` -} - -// String complies with the stringer interface. -func (s *Skipped) String() string { - if s == nil { - return "" - } - - return "skipped " + s.Item.Error() + ": " + s.Item.Cause -} - -// HasCause compares the underlying cause against the parameter. -func (s *Skipped) HasCause(c skipCause) bool { - if s == nil { - return false - } - - return s.Item.Cause == string(c) -} - -func (s Skipped) MinimumPrintable() any { - return s -} - -// Headers returns the human-readable names of properties of a skipped Item -// for printing out to a terminal. -func (s Skipped) Headers() []string { - return []string{"Action", "Type", "Name", "Container", "Cause"} -} - -// Values populates the printable values matching the Headers list. -func (s Skipped) Values() []string { - var cn string - - acn, ok := s.Item.Additional[AddtlContainerName] - if ok { - str, ok := acn.(string) - if ok { - cn = str - } - } - - return []string{"Skip", s.Item.Type.Printable(), s.Item.Name, cn, s.Item.Cause} -} - -// ContainerSkip produces a Container-kind Item for tracking skipped items. -func ContainerSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { - return itemSkip(ContainerType, cause, namespace, id, name, addtl) -} - -// FileSkip produces a File-kind Item for tracking skipped items. -func FileSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { - return itemSkip(FileType, cause, namespace, id, name, addtl) -} - -// OnwerSkip produces a ResourceOwner-kind Item for tracking skipped items. -func OwnerSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { - return itemSkip(ResourceOwnerType, cause, namespace, id, name, addtl) -} - -// itemSkip produces a Item of the provided type for tracking skipped items. -func itemSkip(t itemType, cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { - return &Skipped{ - Item: Item{ - Namespace: namespace, - ID: id, - Name: name, - Type: t, - Cause: string(cause), - Additional: addtl, - }, - } -} - -// --------------------------------------------------------------------------- -// Alerts -// --------------------------------------------------------------------------- - -var _ print.Printable = &Alert{} - -// Alerts are informational-only notifications. The purpose of alerts is to -// provide a means of end-user communication about important events without -// needing to generate runtime failures or recoverable errors. When generating -// an alert, no other fault feature (failure, recoverable, skip, etc) should -// be in use. IE: Errors do not also get alerts, since the error itself is a -// form of end-user communication already. -type Alert struct { - Item Item `json:"item"` - Message string `json:"message"` -} - -// String complies with the stringer interface. -func (a Alert) String() string { - msg := a.Message - if len(msg) == 0 { - msg = "" - } - - return "Alert: " + msg -} - -func (a Alert) MinimumPrintable() any { - return a -} - -// Headers returns the human-readable names of properties of a skipped Item -// for printing out to a terminal. -func (a Alert) Headers() []string { - return []string{"Action", "Message", "Container", "Name", "ID"} -} - -// Values populates the printable values matching the Headers list. -func (a Alert) Values() []string { - var cn string - - acn, ok := a.Item.Additional[AddtlContainerName] - if ok { - str, ok := acn.(string) - if ok { - cn = str - } - } - - return []string{"Alert", a.Message, cn, a.Item.Name, a.Item.ID} -} - -func NewAlert(message, namespace, itemID, name string, addtl map[string]any) *Alert { - return &Alert{ - Message: message, - Item: Item{ - Namespace: namespace, - ID: itemID, - Name: name, - Additional: addtl, - }, - } -} diff --git a/src/pkg/fault/item_test.go b/src/pkg/fault/item_test.go index db6fef84eb..bdb2ca4820 100644 --- a/src/pkg/fault/item_test.go +++ b/src/pkg/fault/item_test.go @@ -1,4 +1,4 @@ -package fault +package fault_test import ( "testing" @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/fault" ) type ItemUnitSuite struct { @@ -21,28 +22,28 @@ func TestItemUnitSuite(t *testing.T) { func (suite *ItemUnitSuite) TestItem_Error() { var ( t = suite.T() - i *Item + i *fault.Item ) assert.Contains(t, i.Error(), "nil") - i = &Item{} + i = &fault.Item{} assert.Contains(t, i.Error(), "unknown type") - i = &Item{Type: FileType} - assert.Contains(t, i.Error(), FileType) + i = &fault.Item{Type: fault.FileType} + assert.Contains(t, i.Error(), fault.FileType) } func (suite *ItemUnitSuite) TestContainerErr() { t := suite.T() addtl := map[string]any{"foo": "bar"} - i := ContainerErr(clues.New("foo"), "ns", "id", "name", addtl) + i := fault.ContainerErr(clues.New("foo"), "ns", "id", "name", addtl) - expect := Item{ + expect := fault.Item{ Namespace: "ns", ID: "id", Name: "name", - Type: ContainerType, + Type: fault.ContainerType, Cause: "foo", Additional: addtl, } @@ -53,13 +54,13 @@ func (suite *ItemUnitSuite) TestContainerErr() { func (suite *ItemUnitSuite) TestFileErr() { t := suite.T() addtl := map[string]any{"foo": "bar"} - i := FileErr(clues.New("foo"), "ns", "id", "name", addtl) + i := fault.FileErr(clues.New("foo"), "ns", "id", "name", addtl) - expect := Item{ + expect := fault.Item{ Namespace: "ns", ID: "id", Name: "name", - Type: FileType, + Type: fault.FileType, Cause: "foo", Additional: addtl, } @@ -70,13 +71,13 @@ func (suite *ItemUnitSuite) TestFileErr() { func (suite *ItemUnitSuite) TestOwnerErr() { t := suite.T() addtl := map[string]any{"foo": "bar"} - i := OwnerErr(clues.New("foo"), "ns", "id", "name", addtl) + i := fault.OwnerErr(clues.New("foo"), "ns", "id", "name", addtl) - expect := Item{ + expect := fault.Item{ Namespace: "ns", ID: "id", Name: "name", - Type: ResourceOwnerType, + Type: fault.ResourceOwnerType, Cause: "foo", Additional: addtl, } @@ -86,23 +87,23 @@ func (suite *ItemUnitSuite) TestOwnerErr() { func (suite *ItemUnitSuite) TestItemType_Printable() { table := []struct { - t itemType + t fault.ItemType expect string }{ { - t: FileType, + t: fault.FileType, expect: "File", }, { - t: ContainerType, + t: fault.ContainerType, expect: "Container", }, { - t: ResourceOwnerType, + t: fault.ResourceOwnerType, expect: "Resource Owner", }, { - t: itemType("foo"), + t: fault.ItemType("foo"), expect: "Unknown", }, } @@ -118,30 +119,30 @@ func (suite *ItemUnitSuite) TestItem_HeadersValues() { err = assert.AnError cause = err.Error() addtl = map[string]any{ - AddtlContainerID: "cid", - AddtlContainerName: "cname", + fault.AddtlContainerID: "cid", + fault.AddtlContainerName: "cname", } ) table := []struct { name string - item *Item + item *fault.Item expect []string }{ { name: "file", - item: FileErr(assert.AnError, "ns", "id", "name", addtl), - expect: []string{"Error", FileType.Printable(), "name", "cname", cause}, + item: fault.FileErr(assert.AnError, "ns", "id", "name", addtl), + expect: []string{"Error", fault.FileType.Printable(), "name", "cname", cause}, }, { name: "container", - item: ContainerErr(assert.AnError, "ns", "id", "name", addtl), - expect: []string{"Error", ContainerType.Printable(), "name", "cname", cause}, + item: fault.ContainerErr(assert.AnError, "ns", "id", "name", addtl), + expect: []string{"Error", fault.ContainerType.Printable(), "name", "cname", cause}, }, { name: "owner", - item: OwnerErr(assert.AnError, "ns", "id", "name", nil), - expect: []string{"Error", ResourceOwnerType.Printable(), "name", "", cause}, + item: fault.OwnerErr(assert.AnError, "ns", "id", "name", nil), + expect: []string{"Error", fault.ResourceOwnerType.Printable(), "name", "", cause}, }, } for _, test := range table { @@ -153,175 +154,3 @@ func (suite *ItemUnitSuite) TestItem_HeadersValues() { }) } } - -func (suite *ItemUnitSuite) TestSkipped_String() { - var ( - t = suite.T() - i *Skipped - ) - - assert.Contains(t, i.String(), "nil") - - i = &Skipped{Item{}} - assert.Contains(t, i.String(), "unknown type") - - i = &Skipped{Item{Type: FileType}} - assert.Contains(t, i.Item.Error(), FileType) -} - -func (suite *ItemUnitSuite) TestContainerSkip() { - t := suite.T() - addtl := map[string]any{"foo": "bar"} - i := ContainerSkip(SkipMalware, "ns", "id", "name", addtl) - - expect := Item{ - Namespace: "ns", - ID: "id", - Name: "name", - Type: ContainerType, - Cause: string(SkipMalware), - Additional: addtl, - } - - assert.Equal(t, Skipped{expect}, *i) -} - -func (suite *ItemUnitSuite) TestFileSkip() { - t := suite.T() - addtl := map[string]any{"foo": "bar"} - i := FileSkip(SkipMalware, "ns", "id", "name", addtl) - - expect := Item{ - Namespace: "ns", - ID: "id", - Name: "name", - Type: FileType, - Cause: string(SkipMalware), - Additional: addtl, - } - - assert.Equal(t, Skipped{expect}, *i) -} - -func (suite *ItemUnitSuite) TestOwnerSkip() { - t := suite.T() - addtl := map[string]any{"foo": "bar"} - i := OwnerSkip(SkipMalware, "ns", "id", "name", addtl) - - expect := Item{ - Namespace: "ns", - ID: "id", - Name: "name", - Type: ResourceOwnerType, - Cause: string(SkipMalware), - Additional: addtl, - } - - assert.Equal(t, Skipped{expect}, *i) -} - -func (suite *ItemUnitSuite) TestSkipped_HeadersValues() { - addtl := map[string]any{ - AddtlContainerID: "cid", - AddtlContainerName: "cname", - } - - table := []struct { - name string - skip *Skipped - expect []string - }{ - { - name: "file", - skip: FileSkip(SkipMalware, "ns", "id", "name", addtl), - expect: []string{"Skip", FileType.Printable(), "name", "cname", string(SkipMalware)}, - }, - { - name: "container", - skip: ContainerSkip(SkipMalware, "ns", "id", "name", addtl), - expect: []string{"Skip", ContainerType.Printable(), "name", "cname", string(SkipMalware)}, - }, - { - name: "owner", - skip: OwnerSkip(SkipMalware, "ns", "id", "name", nil), - expect: []string{"Skip", ResourceOwnerType.Printable(), "name", "", string(SkipMalware)}, - }, - } - for _, test := range table { - suite.Run(test.name, func() { - t := suite.T() - - assert.Equal(t, []string{"Action", "Type", "Name", "Container", "Cause"}, test.skip.Headers()) - assert.Equal(t, test.expect, test.skip.Values()) - }) - } -} - -func (suite *ItemUnitSuite) TestAlert_String() { - var ( - t = suite.T() - a Alert - ) - - assert.Contains(t, a.String(), "Alert: ") - - a = Alert{ - Item: Item{}, - Message: "", - } - assert.Contains(t, a.String(), "Alert: ") - - a = Alert{ - Item: Item{ - ID: "item_id", - }, - Message: "msg", - } - assert.NotContains(t, a.String(), "item_id") - assert.Contains(t, a.String(), "Alert: msg") -} - -func (suite *ItemUnitSuite) TestNewAlert() { - t := suite.T() - addtl := map[string]any{"foo": "bar"} - a := NewAlert("message-to-show", "ns", "item_id", "item_name", addtl) - - expect := Alert{ - Item: Item{ - Namespace: "ns", - ID: "item_id", - Name: "item_name", - Additional: addtl, - }, - Message: "message-to-show", - } - - assert.Equal(t, expect, *a) -} - -func (suite *ItemUnitSuite) TestAlert_HeadersValues() { - addtl := map[string]any{ - AddtlContainerID: "cid", - AddtlContainerName: "cname", - } - - table := []struct { - name string - alert *Alert - expect []string - }{ - { - name: "new alert", - alert: NewAlert("message-to-show", "ns", "id", "name", addtl), - expect: []string{"Alert", "message-to-show", "cname", "name", "id"}, - }, - } - for _, test := range table { - suite.Run(test.name, func() { - t := suite.T() - - assert.Equal(t, []string{"Action", "Message", "Container", "Name", "ID"}, test.alert.Headers()) - assert.Equal(t, test.expect, test.alert.Values()) - }) - } -} diff --git a/src/pkg/fault/skipped.go b/src/pkg/fault/skipped.go new file mode 100644 index 0000000000..b836fc129b --- /dev/null +++ b/src/pkg/fault/skipped.go @@ -0,0 +1,117 @@ +package fault + +import ( + "github.com/alcionai/corso/src/cli/print" +) + +// skipCause identifies the well-known conditions to Skip an item. It is +// important that skip cause enumerations do not overlap with general error +// handling. Skips must be well known, well documented, and consistent. +// Transient failures, undocumented or unknown conditions, and arbitrary +// handling should never produce a skipped item. Those cases should get +// handled as normal errors. +type skipCause string + +const ( + // SkipMalware identifies a malware detection case. Files that graph + // api identifies as malware cannot be downloaded or uploaded, and will + // permanently fail any attempts to backup or restore. + SkipMalware skipCause = "malware_detected" + + // SkipBigOneNote identifies that a file was skipped because it + // was big OneNote file and we can only download OneNote files which + // are less that 2GB in size. + //nolint:lll + // https://support.microsoft.com/en-us/office/restrictions-and-limitations-in-onedrive-and-sharepoint-64883a5d-228e-48f5-b3d2-eb39e07630fa#onenotenotebooks + SkipBigOneNote skipCause = "big_one_note_file" +) + +var _ print.Printable = &Skipped{} + +// Skipped items are permanently unprocessable due to well-known conditions. +// In order to skip an item, the following conditions should be met: +// 1. The conditions for skipping the item are well-known and +// well-documented. End users need to be able to understand +// both the conditions and identifications of skips. +// 2. Skipping avoids a permanent and consistent failure. If +// the underlying reason is transient or otherwise recoverable, +// the item should not be skipped. +// +// Skipped wraps Item primarily to minimize confusion when sharing the +// fault interface. Skipped items are not errors, and Item{} errors are +// not the basis for a Skip. +type Skipped struct { + Item Item `json:"item"` +} + +// String complies with the stringer interface. +func (s *Skipped) String() string { + if s == nil { + return "" + } + + return "skipped " + s.Item.Error() + ": " + s.Item.Cause +} + +// HasCause compares the underlying cause against the parameter. +func (s *Skipped) HasCause(c skipCause) bool { + if s == nil { + return false + } + + return s.Item.Cause == string(c) +} + +func (s Skipped) MinimumPrintable() any { + return s +} + +// Headers returns the human-readable names of properties of a skipped Item +// for printing out to a terminal. +func (s Skipped) Headers() []string { + return []string{"Action", "Type", "Name", "Container", "Cause"} +} + +// Values populates the printable values matching the Headers list. +func (s Skipped) Values() []string { + var cn string + + acn, ok := s.Item.Additional[AddtlContainerName] + if ok { + str, ok := acn.(string) + if ok { + cn = str + } + } + + return []string{"Skip", s.Item.Type.Printable(), s.Item.Name, cn, s.Item.Cause} +} + +// ContainerSkip produces a Container-kind Item for tracking skipped items. +func ContainerSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { + return itemSkip(ContainerType, cause, namespace, id, name, addtl) +} + +// FileSkip produces a File-kind Item for tracking skipped items. +func FileSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { + return itemSkip(FileType, cause, namespace, id, name, addtl) +} + +// OnwerSkip produces a ResourceOwner-kind Item for tracking skipped items. +func OwnerSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { + return itemSkip(ResourceOwnerType, cause, namespace, id, name, addtl) +} + +// itemSkip produces a Item of the provided type for tracking skipped items. +func itemSkip(t ItemType, cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped { + return &Skipped{ + Item: Item{ + Namespace: namespace, + ID: id, + Name: name, + Type: t, + Cause: string(cause), + Additional: addtl, + }, + } +} diff --git a/src/pkg/fault/skipped_test.go b/src/pkg/fault/skipped_test.go new file mode 100644 index 0000000000..22d8cddf49 --- /dev/null +++ b/src/pkg/fault/skipped_test.go @@ -0,0 +1,146 @@ +package fault_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" + + "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/fault" +) + +type SkippedUnitSuite struct { + tester.Suite +} + +func TestSkippedUnitSuite(t *testing.T) { + suite.Run(t, &SkippedUnitSuite{Suite: tester.NewUnitSuite(t)}) +} + +func (suite *SkippedUnitSuite) TestSkipped_String() { + var ( + t = suite.T() + i *fault.Skipped + ) + + assert.Contains(t, i.String(), "nil") + + i = &fault.Skipped{fault.Item{}} + assert.Contains(t, i.String(), "unknown type") + + i = &fault.Skipped{ + fault.Item{ + Type: fault.FileType, + }, + } + assert.Contains(t, i.Item.Error(), fault.FileType) +} + +func (suite *SkippedUnitSuite) TestContainerSkip() { + t := suite.T() + addtl := map[string]any{"foo": "bar"} + i := fault.ContainerSkip(fault.SkipMalware, "ns", "id", "name", addtl) + + expect := fault.Item{ + Namespace: "ns", + ID: "id", + Name: "name", + Type: fault.ContainerType, + Cause: string(fault.SkipMalware), + Additional: addtl, + } + + assert.Equal(t, fault.Skipped{expect}, *i) +} + +func (suite *SkippedUnitSuite) TestFileSkip() { + t := suite.T() + addtl := map[string]any{"foo": "bar"} + i := fault.FileSkip(fault.SkipMalware, "ns", "id", "name", addtl) + + expect := fault.Item{ + Namespace: "ns", + ID: "id", + Name: "name", + Type: fault.FileType, + Cause: string(fault.SkipMalware), + Additional: addtl, + } + + assert.Equal(t, fault.Skipped{expect}, *i) +} + +func (suite *SkippedUnitSuite) TestOwnerSkip() { + t := suite.T() + addtl := map[string]any{"foo": "bar"} + i := fault.OwnerSkip(fault.SkipMalware, "ns", "id", "name", addtl) + + expect := fault.Item{ + Namespace: "ns", + ID: "id", + Name: "name", + Type: fault.ResourceOwnerType, + Cause: string(fault.SkipMalware), + Additional: addtl, + } + + assert.Equal(t, fault.Skipped{expect}, *i) +} + +func (suite *SkippedUnitSuite) TestSkipped_HeadersValues() { + addtl := map[string]any{ + fault.AddtlContainerID: "cid", + fault.AddtlContainerName: "cname", + } + + table := []struct { + name string + skip *fault.Skipped + expect []string + }{ + { + name: "file", + skip: fault.FileSkip(fault.SkipMalware, "ns", "id", "name", addtl), + expect: []string{"Skip", fault.FileType.Printable(), "name", "cname", string(fault.SkipMalware)}, + }, + { + name: "container", + skip: fault.ContainerSkip(fault.SkipMalware, "ns", "id", "name", addtl), + expect: []string{"Skip", fault.ContainerType.Printable(), "name", "cname", string(fault.SkipMalware)}, + }, + { + name: "owner", + skip: fault.OwnerSkip(fault.SkipMalware, "ns", "id", "name", nil), + expect: []string{"Skip", fault.ResourceOwnerType.Printable(), "name", "", string(fault.SkipMalware)}, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + assert.Equal(t, []string{"Action", "Type", "Name", "Container", "Cause"}, test.skip.Headers()) + assert.Equal(t, test.expect, test.skip.Values()) + }) + } +} + +func (suite *SkippedUnitSuite) TestBus_AddSkip() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + n := fault.New(true) + require.NotNil(t, n) + + n.Fail(assert.AnError) + assert.Len(t, n.Skipped(), 0) + + n.AddRecoverable(ctx, assert.AnError) + assert.Len(t, n.Skipped(), 0) + + n.AddSkip(ctx, fault.OwnerSkip(fault.SkipMalware, "ns", "id", "name", nil)) + assert.Len(t, n.Skipped(), 1) +}