diff --git a/platform/errs/BUILD.bazel b/platform/errs/BUILD.bazel index c7de244e9..8e5e6d32e 100644 --- a/platform/errs/BUILD.bazel +++ b/platform/errs/BUILD.bazel @@ -5,6 +5,7 @@ go_library( srcs = [ "errs.go", "failure.go", + "group.go", "processor.go", ], importpath = "github.com/uber/submitqueue/platform/errs", @@ -17,6 +18,7 @@ go_test( srcs = [ "errs_test.go", "failure_test.go", + "group_test.go", "processor_test.go", ], embed = [":go_default_library"], diff --git a/platform/errs/README.md b/platform/errs/README.md index 391054c4c..b39b5defa 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -44,13 +44,46 @@ An `ErrorProcessor` runs the per-chain pass that turns a raw chain into a wrappe Two implementations ship in this package: - **`NewClassifierProcessor(classifiers...)`** — the standard pass for primary pipeline consumers. Walks the chain twice: - 1. **Pass 1 — framework-wrap check.** A cheap type switch looks for an existing `*userError` / `*infraError` anywhere in the chain. If found, the chain is already interpretable and the processor returns `err` unchanged. **No classifier is invoked.** + 1. **Pass 1 — framework-wrap check.** Looks for an existing `*userError` / `*infraError` on the error's single-cause spine. If found, the chain is already interpretable and the processor returns `err` unchanged. **No classifier is invoked.** 2. **Pass 2 — classifier walk.** From outermost to innermost node, each registered classifier is asked for a verdict. The first non-`Unknown` verdict wins and `err` is wrapped with the matching framework constructor. If no classifier recognises anything, `err` is returned unchanged — and behaves as non-retryable infra at the helper layer. - **`AlwaysRetryableProcessor`** — unconditionally wraps every non-nil error with `NewRetryableError`, overriding any inner framework wrap. Use it for narrowly-scoped consumers — typically DLQ reconciliation — that must redeliver on any failure because there is no further dead-letter destination. Side-effect: an inner `*infraError(dependency=true)` is masked by the outer `retryable=true` wrap, since `errors.As` matches the outermost `*infraError` first. This is acceptable for the intended DLQ use case where only `IsRetryable` drives transport behaviour; do not pair this processor with a primary pipeline consumer or genuine user errors will retry forever instead of reaching their DLQ. +### Grouped errors + +`Group(errs...)` reports several failures that happened together as one error. It drops nils and returns nil when every member is nil, so a step that fans work out to independent handlers can accumulate failures in a loop and return the result directly: + +```go +var failures []error +for _, h := range handlers { + if err := h.Handle(ctx, event); err != nil { + failures = append(failures, fmt.Errorf("%s: %w", h.Name(), err)) + } +} +return errs.Group(failures...) +``` + +Pass 2 descends into the members of a group, as well as down ordinary single-cause wraps. Without that, `errors.Unwrap` returns nil for a group, so a walk built on it alone sees the group node and nothing beneath it, and every member goes unclassified. + +**Grouping is opt-in, and `Group` is how you opt in.** The processor recognizes a group only by the type `Group` returns, not by the `Unwrap() []error` method. That method alone does not mean "independent failures": `fmt.Errorf("%w: %w", ErrNotFound, err)` uses it for two facets of a single failure, and `errors.Join` uses it for whatever a caller happened to bundle, often a real failure beside a cleanup error on a shutdown path. Ranking those would let an incidental sibling decide retryability for a failure nobody meant to group — a transient error joined to the failure that caused it would make the whole thing look retryable. So the deliberate spelling is required, and every other multi-cause error is an opaque node. + +This costs less than it appears, because it only governs verdicts a **classifier** has to derive. `errors.Is` and `errors.As` traverse multiple causes on their own, so a member that already carries a framework wrap is honored wherever it sits: `IsRetryable(errors.Join(cleanupErr, NewRetryableError(cause)))` is true with no group involved. + +A group and a wrap chain combine differently, because they mean different things: + +- **Down a wrap chain**, the outermost verdict wins. A wrapper saw the error it wrapped and classified anyway, so it speaks with more knowledge than its cause. +- **Across the members of a group**, nothing shadows anything. The members are independent failures reported together, and their order is the order the caller ran them in, not a precedence. They combine by rank, so the result cannot depend on which member failed first. + +The rank puts retryable above non-retryable, because the two mistakes cost differently: a wrong "retryable" spends a bounded retry budget and then dead-letters anyway, while a wrong "non-retryable" throws away a failure that would have cleared on its own. Within a retryability tier, the verdict that implicates this service outranks the one pointing elsewhere, so a partly-local failure is not reported as a pure dependency or user problem — that ordering only moves attribution, since every non-retryable verdict produces the same transport outcome. See `verdictRank` for the table. + +A framework wrap classifies the subtree beneath it and no further. Above a group it covers the whole group and Pass 1 returns the error verbatim, so no member is consulted. *Inside* a member it is one member's account of one failure, with no standing to classify the failures beside it — so it contributes its own verdict to the rank like any other member. That is what keeps a sibling's transient failure from being discarded by a member that happened to arrive pre-classified, and it also removes an ordering artifact: two wrapped members of differing retryability used to resolve by whichever one `errors.As` reached first. + +The losing member keeps its wrap in the chain, so `IsUserError` and `IsRetryable` can both report true for the same grouped error — one from a member, one from the outer wrap. Only the outer wrap drives the retry decision, the same way it does under `AlwaysRetryableProcessor`. + +One operational consequence worth knowing before relying on any of this: **retrying a group re-runs everything.** The retry redelivers to every child, including the ones that succeeded, so children must be idempotent, and a child that fails persistently with a retryable-looking error (a decommissioned service returning connection-refused, say) will spend the whole retry budget on every message. Drop such a child rather than absorbing it. + ### Choosing a processor - **Primary pipeline consumer** → `NewClassifierProcessor(...)`. Controllers' explicit `NewUserError` / `NewDependencyError` wraps must survive so user errors don't get retried, and unclassified backend errors must be inspected by the registered classifiers. @@ -131,7 +164,7 @@ if err != nil { Two practical rules fall out of the short-circuit semantics: - **Wrap with a framework constructor as soon as the controller knows the right verdict.** Any wrap added later in the chain still wins, but wrapping early keeps the intent close to the decision. -- **A wrap anywhere in the chain blocks all classifiers — including for nodes deeper than the wrap.** If you want a classifier to still get a look at the cause, do not wrap above it. (In practice this is rare: controllers wrap because they have the final answer.) +- **A wrap blocks all classifiers beneath it, including for nodes deeper than the wrap.** If you want a classifier to still get a look at the cause, do not wrap above it. (In practice this is rare: controllers wrap because they have the final answer.) It does not block the sibling members of a group, which are classified and ranked independently. ### When *not* to classify in a controller diff --git a/platform/errs/processor.go b/platform/errs/processor.go index 6fd806f02..8fd12b367 100644 --- a/platform/errs/processor.go +++ b/platform/errs/processor.go @@ -50,10 +50,9 @@ type ErrorProcessor interface { // Semantics of Process on the returned processor: // // - nil in, nil out. -// - If err's chain already carries a framework classification (*userError -// or *infraError anywhere in the chain), returns err unchanged — the chain -// is already interpretable by IsUserError / IsRetryable / -// IsDependencyError. +// - If err carries a framework classification (*userError or *infraError) on +// its single-cause spine, returns err unchanged — the chain is already +// interpretable by IsUserError / IsRetryable / IsDependencyError. // - Otherwise, walks the chain from outermost to innermost, asking each // classifier per node. The FIRST non-Unknown verdict wins; the outermost // such node determines the wrap. err is wrapped with the framework @@ -61,15 +60,21 @@ type ErrorProcessor interface { // -> NewRetryableError, etc.) and the wrapped error is returned. // - Verdict Infra means "non-retryable infra" — which is already the default // behavior for an unwrapped chain, so no wrap is added. -// - If no classifier recognises anything, err is returned unchanged. +// - If no classifier recognizes anything, err is returned unchanged. // -// Implementation: two passes over the chain. Pass 1 is a cheap type check -// looking for an existing framework wrap and short-circuits if one is found — -// no classifier is invoked. Pass 2 runs the configured classifiers per node. -// Walking the chain is cheap relative to a classifier call, so this avoids -// running classifiers whenever the chain is already classified deeper down. +// Implementation: two passes over the chain. Pass 1 looks for an existing +// framework wrap and short-circuits if one is found — no classifier is invoked. +// Pass 2 runs the configured classifiers per node. Walking the chain is cheap +// relative to a classifier call, so this avoids running classifiers whenever +// the chain is already classified deeper down. // -// Passing no classifiers is valid — the processor will still honour any +// Grouping is opt-in. Pass 2 descends into the members of a Group, where a wrap +// classifies the subtree beneath it and no more, so a wrapped member is weighed +// against its siblings instead of answering for them; classify documents how. +// Any other multi-cause error is an opaque node, so a caller that wants its +// failures weighed together says so with Group. +// +// Passing no classifiers is valid — the processor will still honor any // framework wrap already in the chain and otherwise return err unchanged. // // NOTE: this central classifier model cannot disambiguate errors of the same @@ -90,29 +95,20 @@ func (p classifierProcessor) Process(err error) error { return nil } - // Pass 1 — cheap framework-wrap check. If any node already carries a - // framework type, the chain is interpretable as-is and classifiers are - // never invoked. + // Pass 1 — framework-wrap check, along the single-cause spine only. A wrap + // found here sits above everything else in err, so it classifies the whole + // error and is returned verbatim. The walk stops at a group because + // errors.Unwrap yields nothing for one, which is the behavior we want: a + // wrap inside one member speaks only for that member, and classify weighs + // it against its siblings rather than letting it silently answer for them. for cur := err; cur != nil; cur = errors.Unwrap(cur) { - switch cur.(type) { - case *userError, *infraError: + if wrapVerdict(cur) != Unknown { return err } } - // Pass 2 — run classifiers per node from outermost to innermost. Stop at - // the first non-Unknown verdict. - var verdict Verdict - for cur := err; cur != nil && verdict == Unknown; cur = errors.Unwrap(cur) { - for _, c := range p.classifiers { - if v := c.Classify(cur); v != Unknown { - verdict = v - break - } - } - } - - switch verdict { + // Pass 2 — classify the chain and wrap with the verdict it reaches. + switch p.classify(err) { case User: return NewUserError(err) case InfraRetryable: @@ -127,6 +123,121 @@ func (p classifierProcessor) Process(err error) error { return err } +// classify returns the verdict for err — from a framework wrap if it carries +// one, otherwise from the configured classifiers — or Unknown when nothing +// recognizes any node in it. +// +// The walk treats the two ways an error can contain another differently, +// because they mean different things: +// +// - Down a wrap chain (Unwrap() error) the outermost verdict wins. A wrapper +// saw the error it wrapped and chose to add context on top of it, so it +// speaks with more knowledge than its cause. +// - Across the members of a Group nothing shadows anything. The members are +// independent failures that happened to be reported together, and their +// order is the order the caller ran them in, not a precedence. They combine +// by verdictRank so that the result cannot depend on which member failed +// first. +// +// The second rule is why a framework wrap does not get the short-circuit it +// gets on the spine: within a group it is one member's account of one failure, +// with no standing to classify the failures beside it. +// +// Only the group Group builds is walked that way, because Unwrap() []error on +// its own does not mean "independent failures". fmt.Errorf("%w: %w", ...) +// produces one for two facets of a single failure, and errors.Join produces one +// for whatever a caller happened to bundle. Ranking those would let an +// incidental sibling decide retryability for a failure nobody meant to group, +// so the walk asks for the deliberate spelling and treats every other +// multi-cause error as an opaque node. +func (p classifierProcessor) classify(err error) Verdict { + for cur := err; cur != nil; { + // A wrap is authoritative for everything it contains, so it answers for + // this subtree without consulting a classifier. + if v := wrapVerdict(cur); v != Unknown { + return v + } + + for _, c := range p.classifiers { + if v := c.Classify(cur); v != Unknown { + return v + } + } + + if group, ok := cur.(*groupedError); ok { + verdict := Unknown + for _, member := range group.Unwrap() { + if v := p.classify(member); verdictRank(v) > verdictRank(verdict) { + verdict = v + } + } + return verdict + } + + cur = errors.Unwrap(cur) + } + return Unknown +} + +// wrapVerdict returns the verdict the framework wrap err carries, or Unknown if +// err is not a wrap. It inspects the single node it is given, never the chain. +// Every wrap maps to a real verdict, which is what makes Unknown usable as the +// "not a wrap" answer. +func wrapVerdict(err error) Verdict { + switch e := err.(type) { + case *userError: + return User + case *infraError: + switch { + case e.retryable && e.dependency: + return InfraDependencyRetryable + case e.retryable: + return InfraRetryable + case e.dependency: + return InfraDependency + default: + return Infra + } + } + return Unknown +} + +// verdictRank orders verdicts for combining the independent members of a +// grouped error. The highest-ranked member verdict becomes the verdict for the +// group as a whole. +// +// It must be an explicit table rather than a comparison on Verdict itself, +// whose constants are declaration-ordered and not severity-ordered: +// InfraDependency numerically exceeds InfraRetryable, so ranking by value would +// let a non-retryable member discard a retryable sibling. +// +// Two principles set the order. Retryable outranks non-retryable because the +// two mistakes cost differently — a wrong "retryable" spends a bounded retry +// budget and then dead-letters anyway, while a wrong "non-retryable" throws +// away a failure that would have cleared on its own and leaves it for someone +// to find and replay by hand. Within a retryability tier the verdict that +// implicates this service outranks the one that points elsewhere, so a failure +// that is partly ours is not reported as a pure dependency or user problem and +// routed away from the people who can fix it. That second ordering only moves +// attribution: every verdict in the non-retryable tier yields the same transport +// outcome. +func verdictRank(v Verdict) int { + switch v { + case InfraRetryable: + return 5 + case InfraDependencyRetryable: + return 4 + case Infra: + return 3 + case User: + return 2 + case InfraDependency: + return 1 + default: // Unknown + return 0 + } +} + // AlwaysRetryableProcessor classifies every non-nil error as InfraRetryable by // wrapping it with NewRetryableError. The wrap is unconditional: an inner // *userError or non-retryable *infraError is overridden because errors.As diff --git a/platform/errs/processor_test.go b/platform/errs/processor_test.go index d81d5ed75..02a7e4037 100644 --- a/platform/errs/processor_test.go +++ b/platform/errs/processor_test.go @@ -30,6 +30,21 @@ type stubClassifier struct{ verdict Verdict } func (s stubClassifier) Classify(error) Verdict { return s.verdict } +// verdictByError recognizes only the exact nodes it was built with, the way a +// real backend classifier recognizes only its own driver's errors. Everything +// else classifies Unknown. +type verdictByError map[error]Verdict + +func (m verdictByError) Classify(err error) Verdict { return m[err] } + +// singleWrap is a classifiable node with exactly one cause. fmt.Errorf cannot +// stand in for it: its wrapper node is opaque to a classifier, and two %w verbs +// produce a group rather than a chain. +type singleWrap struct{ cause error } + +func (w singleWrap) Error() string { return "wrapped: " + w.cause.Error() } +func (w singleWrap) Unwrap() error { return w.cause } + func TestNewClassifierProcessor_NilIn(t *testing.T) { p := NewClassifierProcessor() assert.NoError(t, p.Process(nil)) @@ -69,6 +84,250 @@ func TestNewClassifierProcessor_NoClassifiersReturnsUnchanged(t *testing.T) { assert.False(t, IsUserError(out)) } +// TestNewClassifierProcessor_GroupedMembers covers the reason Group exists: a +// caller that fans work out to several children reports their failures as one +// error, and errors.Unwrap cannot see into one, so classifiers used to never be +// offered any member. +func TestNewClassifierProcessor_GroupedMembers(t *testing.T) { + transient := errors.New("deadlock") + permanent := errors.New("schema mismatch") + badInput := errors.New("malformed payload") + upstreamBlip := errors.New("upstream 503") + upstreamGone := errors.New("upstream decommissioned") + + p := NewClassifierProcessor(verdictByError{ + transient: InfraRetryable, + permanent: Infra, + badInput: User, + upstreamBlip: InfraDependencyRetryable, + upstreamGone: InfraDependency, + }) + + tests := []struct { + name string + err error + wantRetryable bool + wantUser bool + wantDependency bool + }{ + { + // A group of one is still a group, so a lone failing child is just + // as opaque to errors.Unwrap as several. + name: "single member", + err: Group(fmt.Errorf("child a: %w", transient)), + wantRetryable: true, + }, + { + name: "retryable member last", + err: Group(fmt.Errorf("child a: %w", permanent), fmt.Errorf("child b: %w", transient)), + wantRetryable: true, + }, + { + // Same members reversed: the verdict must come from rank, not from + // the order the children happened to run in. + name: "retryable member first", + err: Group(fmt.Errorf("child a: %w", transient), fmt.Errorf("child b: %w", permanent)), + wantRetryable: true, + }, + { + name: "retryable outranks user", + err: Group(badInput, transient), + wantRetryable: true, + }, + { + name: "group nested below a wrap", + err: fmt.Errorf("dispatch: %w", Group(permanent, fmt.Errorf("child b: %w", transient))), + wantRetryable: true, + }, + { + name: "group nested inside another group", + err: Group(permanent, Group(badInput, transient)), + wantRetryable: true, + }, + { + // Both members are retryable, so only attribution is in question: + // a failure that is partly local is not blamed on the dependency. + name: "local retryable outranks dependency retryable", + err: Group(upstreamBlip, transient), + wantRetryable: true, + }, + { + name: "dependency retryable alone keeps its provenance", + err: Group(permanent, upstreamBlip), + wantRetryable: true, + wantDependency: true, + }, + { + name: "user outranks non-retryable dependency", + err: Group(upstreamGone, badInput), + wantUser: true, + }, + { + name: "no member recognized", + err: Group(errors.New("who knows"), errors.New("nor this")), + }, + { + // "nobody recognized this" is weaker evidence than any verdict, so + // an unrecognized member must not drown out a classified sibling. + name: "unrecognized member does not outrank a classified user sibling", + err: Group(errors.New("who knows"), badInput), + wantUser: true, + }, + { + name: "unrecognized member does not outrank a classified dependency sibling", + err: Group(errors.New("who knows"), upstreamGone), + wantDependency: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := p.Process(tt.err) + require.Error(t, out) + assert.Equal(t, tt.wantRetryable, IsRetryable(out)) + assert.Equal(t, tt.wantUser, IsUserError(out)) + assert.Equal(t, tt.wantDependency, IsDependencyError(out)) + }) + } +} + +// TestNewClassifierProcessor_MultiCauseErrorsOutsideGroupAreOpaque pins the +// distinction Group exists to draw. errors.Join and a two-verb fmt.Errorf also +// expose Unwrap() []error, but neither says its causes are independent failures +// meant to be weighed against each other, so a classifier verdict on one of +// them must not decide the retryability of the whole. +func TestNewClassifierProcessor_MultiCauseErrorsOutsideGroupAreOpaque(t *testing.T) { + transient := errors.New("deadlock") + notFound := errors.New("record not found") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + tests := []struct { + name string + err error + }{ + {name: "errors.Join", err: errors.Join(notFound, transient)}, + {name: "two %w verbs", err: fmt.Errorf("%w: %w", notFound, transient)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := p.Process(tt.err) + require.Error(t, out) + assert.False(t, IsRetryable(out)) + assert.True(t, errors.Is(out, transient), "the causes stay reachable either way") + assert.True(t, errors.Is(out, notFound)) + }) + } +} + +// A framework wrap needs no opt-in: errors.As walks a multi-cause error, so a +// member that already carries a classification is honored wherever it sits. +// Only a verdict a classifier has to derive depends on Group. +func TestNewClassifierProcessor_FrameworkWrapVisibleInsideAnyMultiCauseError(t *testing.T) { + p := NewClassifierProcessor() + + out := p.Process(errors.Join(errors.New("cleanup failed"), NewRetryableError(errors.New("dropped connection")))) + + require.Error(t, out) + assert.True(t, IsRetryable(out)) +} + +func TestNewClassifierProcessor_GroupedMembersKeepEveryCause(t *testing.T) { + transient := errors.New("deadlock") + permanent := errors.New("schema mismatch") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + out := p.Process(Group(fmt.Errorf("child a: %w", permanent), fmt.Errorf("child b: %w", transient))) + + require.True(t, IsRetryable(out)) + assert.True(t, errors.Is(out, transient)) + assert.True(t, errors.Is(out, permanent), "the member that lost the rank must stay in the chain for diagnostics") +} + +// TestNewClassifierProcessor_WrappedMembersAreWeighed covers members that +// arrive already classified. A wrap speaks for the subtree beneath it, so it +// contributes a verdict to the group like any other member rather than deciding +// for its siblings — which is what makes the outcome independent of the order +// the members were reported in. +func TestNewClassifierProcessor_WrappedMembersAreWeighed(t *testing.T) { + transient := errors.New("deadlock") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + tests := []struct { + name string + err error + wantRetryable bool + wantUser bool + }{ + { + // IsUserError stays true alongside it: the losing member keeps its + // wrap in the chain, and only the outer one drives the retry. + name: "wrapped user error does not suppress a classifiable sibling", + err: Group(NewUserError(errors.New("malformed payload")), transient), + wantRetryable: true, + wantUser: true, + }, + { + name: "retryable wrap ranked ahead of a non-retryable one", + err: Group(NewDependencyError(errors.New("upstream 503")), NewRetryableError(errors.New("blip"))), + wantRetryable: true, + }, + { + // The same two wraps reversed. Before wraps were weighed, this pair + // resolved by whichever member errors.As reached first. + name: "retryable wrap ranked ahead of a non-retryable one, reversed", + err: Group(NewRetryableError(errors.New("blip")), NewDependencyError(errors.New("upstream 503"))), + wantRetryable: true, + }, + { + name: "sole wrapped member still classifies the group", + err: Group(NewUserError(errors.New("malformed payload")), errors.New("unrecognized")), + wantUser: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := p.Process(tt.err) + require.Error(t, out) + assert.Equal(t, tt.wantRetryable, IsRetryable(out)) + assert.Equal(t, tt.wantUser, IsUserError(out)) + }) + } +} + +// TestNewClassifierProcessor_SpineWrapStillShortCircuits pins the other half of +// the rule: a wrap above a group covers the whole group, so it is returned +// verbatim and no member is consulted. +func TestNewClassifierProcessor_SpineWrapStillShortCircuits(t *testing.T) { + transient := errors.New("deadlock") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + wrapped := NewUserError(Group(errors.New("child a"), transient)) + out := p.Process(wrapped) + + assert.Same(t, wrapped, out) + assert.True(t, IsUserError(out)) + assert.False(t, IsRetryable(out)) +} + +// TestNewClassifierProcessor_WrapChainKeepsOutermostVerdict guards the +// asymmetry between the two walks: rank decides between the members of a group, +// but down a wrap chain the outer node still wins outright, because it saw its +// cause and classified anyway. Without this the group rule would leak into +// ordinary chains and let a retryable cause override the verdict a caller +// deliberately put on top of it. +func TestNewClassifierProcessor_WrapChainKeepsOutermostVerdict(t *testing.T) { + inner := errors.New("deadlock") + outer := singleWrap{cause: inner} + p := NewClassifierProcessor(verdictByError{outer: User, inner: InfraRetryable}) + + out := p.Process(outer) + + assert.True(t, IsUserError(out)) + assert.False(t, IsRetryable(out)) +} + func TestAlwaysRetryableProcessor_NilIn(t *testing.T) { assert.NoError(t, AlwaysRetryableProcessor.Process(nil)) } diff --git a/submitqueue/extension/validator/composite/BUILD.bazel b/submitqueue/extension/validator/composite/BUILD.bazel index 55a9cf716..a1b45bf02 100644 --- a/submitqueue/extension/validator/composite/BUILD.bazel +++ b/submitqueue/extension/validator/composite/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/uber/submitqueue/submitqueue/extension/validator/composite", visibility = ["//visibility:public"], deps = [ + "//platform/errs:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/validator:go_default_library", ], diff --git a/submitqueue/extension/validator/composite/validator.go b/submitqueue/extension/validator/composite/validator.go index b6f38eff9..6668fd6d8 100644 --- a/submitqueue/extension/validator/composite/validator.go +++ b/submitqueue/extension/validator/composite/validator.go @@ -16,13 +16,13 @@ package composite import ( "context" - "errors" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/validator" ) -// compositeValidator runs all validators and joins their errors. +// compositeValidator runs all validators and groups their errors. type compositeValidator struct { // cfg is the per-queue identity this validator was built for. cfg validator.Config @@ -31,17 +31,20 @@ type compositeValidator struct { } // New creates a Validator bound to the queue named in cfg that evaluates all -// child validators and joins their errors. +// child validators and groups their errors. func New(cfg validator.Config, validators []validator.Validator) validator.Validator { return &compositeValidator{cfg: cfg, validators: validators} } +// Validate runs every child even after one fails, and reports the failures as a +// group so each is classified on its own merits: one child failing on bad input +// must not make a sibling's transient failure permanent. func (c *compositeValidator) Validate(ctx context.Context, request entity.Request) error { - var errs []error + var failures []error for _, v := range c.validators { if err := v.Validate(ctx, request); err != nil { - errs = append(errs, err) + failures = append(failures, err) } } - return errors.Join(errs...) + return errs.Group(failures...) }