From 6af9c3050faf15615f10fc43547c96145dae42f0 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sun, 26 Jul 2026 16:44:18 -0700 Subject: [PATCH] refactor(errors): consolidate domain validation errors onto a shared type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit internal/core/errors was dead code — zero importers — while six domain packages each reimplemented the same ValidationError with a slightly different message format. Because those were distinct Go types with identical shapes, a handler could not ask "is this a validation failure?" in one place: it had to call posts.IsValidationError, then communities.IsValidationError, then aggregators.IsValidationError, and any domain it forgot fell through to a 500. The six packages now alias the shared type rather than redefining it: type ValidationError = coreerrors.ValidationError A Go type alias is the *same* type, not a similar one, so a single errors.As at the API boundary matches validation failures from every aliasing domain while each package keeps its own constructors and predicates. Existing call sites compile unchanged. Two latent bugs surfaced and are fixed: - timeline.IsValidationError and discover.IsValidationError used a bare type assertion rather than errors.As, so a validation error wrapped anywhere in the stack with %w stopped being recognised and returned 500 instead of 400. - posts.IsNotFound compared with == rather than errors.Is, which breaks the moment any layer adds context. Changes: - Rewrite internal/core/errors as the canonical ValidationError, NotFoundError, and ConflictError, with Is methods bridging the latter two to shared sentinels - Alias the shared types in posts, communities, aggregators, communityFeeds, discover, and timeline - Switch timeline/discover predicates to errors.As so they unwrap - Switch posts.IsNotFound to errors.Is - Replace a hand-rolled contains/anySubstring reimplementation of strings.Contains in posts with the stdlib - Add errors_test.go asserting the cross-domain property the whole change exists for: reverting any alias to a local struct still compiles and still passes each domain's own tests, so only this test catches it Note for clients: ValidationError.Error() is now uniformly "field: message". Previously posts returned "validation error (field): msg", aggregators "validation error: field - msg", and timeline/discover the bare message with no field prefix. These strings reach clients as the XRPC error *message*; the error *code* is unchanged, so the lexicon contract holds. Co-Authored-By: Claude Opus 5 (1M context) --- internal/core/aggregators/errors.go | 27 ++- internal/core/communities/errors.go | 25 +-- internal/core/communityFeeds/errors.go | 25 +-- internal/core/discover/types.go | 29 ++-- internal/core/errors/errors.go | 156 +++++++++++++---- internal/core/errors/errors_test.go | 225 +++++++++++++++++++++++++ internal/core/posts/errors.go | 100 ++++------- internal/core/timeline/types.go | 29 ++-- 8 files changed, 435 insertions(+), 181 deletions(-) create mode 100644 internal/core/errors/errors_test.go diff --git a/internal/core/aggregators/errors.go b/internal/core/aggregators/errors.go index 11150df..de12dbe 100644 --- a/internal/core/aggregators/errors.go +++ b/internal/core/aggregators/errors.go @@ -2,7 +2,8 @@ package aggregators import ( "errors" - "fmt" + + coreerrors "Coves/internal/core/errors" ) // Domain errors @@ -26,22 +27,15 @@ var ( ErrOAuthSessionMismatch = errors.New("OAuth session DID does not match aggregator DID") ) -// ValidationError represents a validation error with field details -type ValidationError struct { - Field string - Message string -} - -func (e *ValidationError) Error() string { - return fmt.Sprintf("validation error: %s - %s", e.Field, e.Message) -} +// ValidationError is the shared validation error type. It is aliased rather +// than redefined so that one errors.As at the API boundary matches validation +// failures from every domain package, instead of each handler needing to know +// which domains it might hear from. +type ValidationError = coreerrors.ValidationError // NewValidationError creates a new validation error func NewValidationError(field, message string) error { - return &ValidationError{ - Field: field, - Message: message, - } + return coreerrors.NewValidationError(field, message) } // Error classification helpers for handlers to map to HTTP status codes @@ -52,8 +46,9 @@ func IsNotFound(err error) bool { } func IsValidationError(err error) bool { - var validationErr *ValidationError - return errors.As(err, &validationErr) || errors.Is(err, ErrInvalidConfig) || errors.Is(err, ErrConfigSchemaValidation) + return coreerrors.IsValidationError(err) || + errors.Is(err, ErrInvalidConfig) || + errors.Is(err, ErrConfigSchemaValidation) } func IsUnauthorized(err error) bool { diff --git a/internal/core/communities/errors.go b/internal/core/communities/errors.go index 747d0dc..635eb7c 100644 --- a/internal/core/communities/errors.go +++ b/internal/core/communities/errors.go @@ -2,7 +2,8 @@ package communities import ( "errors" - "fmt" + + coreerrors "Coves/internal/core/errors" ) // Domain errors for communities @@ -47,22 +48,15 @@ var ( ErrInvalidInput = errors.New("invalid input") ) -// ValidationError wraps input validation errors with field details -type ValidationError struct { - Field string - Message string -} - -func (e *ValidationError) Error() string { - return fmt.Sprintf("%s: %s", e.Field, e.Message) -} +// ValidationError is the shared validation error type. It is aliased rather +// than redefined so that one errors.As at the API boundary matches validation +// failures from every domain package, instead of each handler needing to know +// which domains it might hear from. +type ValidationError = coreerrors.ValidationError // NewValidationError creates a new validation error func NewValidationError(field, message string) *ValidationError { - return &ValidationError{ - Field: field, - Message: message, - } + return coreerrors.NewValidationError(field, message) } // IsNotFound checks if error is a "not found" error @@ -83,6 +77,5 @@ func IsConflict(err error) bool { // IsValidationError checks if error is a validation error func IsValidationError(err error) bool { - var valErr *ValidationError - return errors.As(err, &valErr) || errors.Is(err, ErrInvalidInput) + return coreerrors.IsValidationError(err) || errors.Is(err, ErrInvalidInput) } diff --git a/internal/core/communityFeeds/errors.go b/internal/core/communityFeeds/errors.go index 7f5bb1e..f492cec 100644 --- a/internal/core/communityFeeds/errors.go +++ b/internal/core/communityFeeds/errors.go @@ -2,7 +2,8 @@ package communityFeeds import ( "errors" - "fmt" + + coreerrors "Coves/internal/core/errors" ) var ( @@ -13,26 +14,18 @@ var ( ErrInvalidCursor = errors.New("invalid pagination cursor") ) -// ValidationError represents an input validation error -type ValidationError struct { - Field string - Message string -} - -func (e *ValidationError) Error() string { - return fmt.Sprintf("validation error: %s: %s", e.Field, e.Message) -} +// ValidationError is the shared validation error type. It is aliased rather +// than redefined so that one errors.As at the API boundary matches validation +// failures from every domain package, instead of each handler needing to know +// which domains it might hear from. +type ValidationError = coreerrors.ValidationError // NewValidationError creates a new validation error func NewValidationError(field, message string) error { - return &ValidationError{ - Field: field, - Message: message, - } + return coreerrors.NewValidationError(field, message) } // IsValidationError checks if an error is a validation error func IsValidationError(err error) bool { - var ve *ValidationError - return errors.As(err, &ve) + return coreerrors.IsValidationError(err) } diff --git a/internal/core/discover/types.go b/internal/core/discover/types.go index c8ffe18..8ef4bab 100644 --- a/internal/core/discover/types.go +++ b/internal/core/discover/types.go @@ -1,6 +1,7 @@ package discover import ( + coreerrors "Coves/internal/core/errors" "Coves/internal/core/posts" "context" "errors" @@ -80,26 +81,22 @@ var ( ErrInvalidCursor = errors.New("invalid cursor") ) -// ValidationError represents a validation error with field context -type ValidationError struct { - Field string - Message string -} - -func (e *ValidationError) Error() string { - return e.Message -} +// ValidationError is the shared validation error type. It is aliased rather +// than redefined so that one errors.As at the API boundary matches validation +// failures from every domain package, instead of each handler needing to know +// which domains it might hear from. +type ValidationError = coreerrors.ValidationError // NewValidationError creates a new validation error func NewValidationError(field, message string) error { - return &ValidationError{ - Field: field, - Message: message, - } + return coreerrors.NewValidationError(field, message) } -// IsValidationError checks if an error is a validation error +// IsValidationError checks if an error is a validation error. +// +// This unwraps, unlike the bare type assertion it replaces: a validation error +// that any layer wrapped with %w for context used to stop being recognised +// here, and fell through to a 500 instead of a 400. func IsValidationError(err error) bool { - _, ok := err.(*ValidationError) - return ok + return coreerrors.IsValidationError(err) } diff --git a/internal/core/errors/errors.go b/internal/core/errors/errors.go index 0b27bdf..044d4ed 100644 --- a/internal/core/errors/errors.go +++ b/internal/core/errors/errors.go @@ -1,3 +1,26 @@ +// Package errors defines the error types shared across every Coves domain +// package. +// +// Each domain used to declare its own ValidationError, NotFoundError, and +// friends. Because those were distinct Go types with identical shapes, a +// handler could not ask "is this a validation failure?" in one place — it had +// to call posts.IsValidationError, then communities.IsValidationError, then +// aggregators.IsValidationError, and any domain a handler forgot silently fell +// through to a 500. +// +// Domain packages now alias these types: +// +// type ValidationError = coreerrors.ValidationError +// +// A Go type alias is the *same* type, not a similar one, so the ValidationError +// of every domain that aliases it is matched by a single errors.As at the +// boundary, while each package keeps its own familiar constructors and +// predicates. +// +// Six packages participate today: posts, communities, aggregators, +// communityFeeds, discover, and timeline. Domains that signal validation +// failures with sentinel errors instead — comments, for one — still need their +// own predicate at the boundary. package errors import ( @@ -5,63 +28,124 @@ import ( "fmt" ) +// Shared sentinel errors, kept to the two the typed errors below bridge to. +// Domain packages define their own more specific sentinels; speculative ones +// here would rebuild the duplicated error surface this package exists to +// remove. var ( - ErrNotFound = errors.New("resource not found") - ErrAlreadyExists = errors.New("resource already exists") - ErrInvalidInput = errors.New("invalid input") - ErrUnauthorized = errors.New("unauthorized") - ErrForbidden = errors.New("forbidden") - ErrInternal = errors.New("internal server error") - ErrDatabaseError = errors.New("database error") - ErrValidationFailed = errors.New("validation failed") + ErrNotFound = errors.New("resource not found") + ErrAlreadyExists = errors.New("resource already exists") ) +// ValidationError reports invalid input, with the offending field named. +// +// Error() is surfaced to clients as the human-readable message of an XRPC +// error response, so it stays short and free of internal detail. The stable +// machine-readable contract is the error *code* the handler chooses, not this +// string. type ValidationError struct { - Field string + // Field is the input that failed validation, named as the client named + // it (a lexicon property, not an internal struct field). + Field string + + // Message explains what was wrong with it. Message string + + // Err is the underlying cause, when there is one. Without it, wrapping a + // typed error to add field context would flatten it to a string and make + // the original sentinel unreachable from errors.Is. + Err error } -func (e ValidationError) Error() string { - return fmt.Sprintf("validation error on field '%s': %s", e.Field, e.Message) +func (e *ValidationError) Error() string { + return fmt.Sprintf("%s: %s", e.Field, e.Message) } -type ConflictError struct { - Resource string - Field string - Value string +// Unwrap exposes the underlying cause to errors.Is and errors.As. +func (e *ValidationError) Unwrap() error { + return e.Err +} + +// NewValidationError reports that field failed validation for the given +// reason. +func NewValidationError(field, message string) *ValidationError { + return &ValidationError{Field: field, Message: message} +} + +// NewValidationErrorFrom adds field context to an existing error while keeping +// cause matchable through errors.Is — which matters when cause is a sentinel +// from a package like internal/validation. +func NewValidationErrorFrom(field string, cause error) *ValidationError { + return &ValidationError{ + Field: field, + Message: cause.Error(), + Err: cause, + } } -func (e ConflictError) Error() string { - return fmt.Sprintf("%s with %s '%s' already exists", e.Resource, e.Field, e.Value) +// IsValidationError reports whether err is, or wraps, a ValidationError. +func IsValidationError(err error) bool { + var validationErr *ValidationError + return errors.As(err, &validationErr) } +// NotFoundError reports that a specific resource does not exist. type NotFoundError struct { - ID interface{} + // Resource is the kind of thing that was missing ("post", "community"). Resource string + + // ID is the identifier that was looked up. + ID string } -func (e NotFoundError) Error() string { - return fmt.Sprintf("%s with ID '%v' not found", e.Resource, e.ID) +func (e *NotFoundError) Error() string { + return fmt.Sprintf("%s not found: %s", e.Resource, e.ID) } -func NewValidationError(field, message string) error { - return ValidationError{ - Field: field, - Message: message, - } +// Is makes every NotFoundError match the shared ErrNotFound sentinel, so a +// caller can test for "missing" without caring which resource was missing. +func (e *NotFoundError) Is(target error) bool { + return target == ErrNotFound } -func NewConflictError(resource, field, value string) error { - return ConflictError{ - Resource: resource, - Field: field, - Value: value, - } +// NewNotFoundError reports that the identified resource does not exist. +func NewNotFoundError(resource, id string) *NotFoundError { + return &NotFoundError{Resource: resource, ID: id} } -func NewNotFoundError(resource string, id interface{}) error { - return NotFoundError{ - Resource: resource, - ID: id, - } +// IsNotFound reports whether err is, or wraps, a NotFoundError. +func IsNotFound(err error) bool { + var notFoundErr *NotFoundError + return errors.As(err, ¬FoundErr) +} + +// ConflictError reports that a resource already exists with the given value, +// typically a uniqueness violation. +type ConflictError struct { + // Resource is the kind of thing that already existed ("community"). + Resource string + + // Field and Value identify the colliding value ("handle", "!go@coves"). + Field string + Value string +} + +func (e *ConflictError) Error() string { + return fmt.Sprintf("%s with %s %q already exists", e.Resource, e.Field, e.Value) +} + +// Is makes every ConflictError match the shared ErrAlreadyExists sentinel. +func (e *ConflictError) Is(target error) bool { + return target == ErrAlreadyExists +} + +// NewConflictError reports a uniqueness violation on the given field. +func NewConflictError(resource, field, value string) *ConflictError { + return &ConflictError{Resource: resource, Field: field, Value: value} +} + +// IsConflict reports whether err is, or wraps, a ConflictError. +func IsConflict(err error) bool { + var conflictErr *ConflictError + return errors.As(err, &conflictErr) } diff --git a/internal/core/errors/errors_test.go b/internal/core/errors/errors_test.go new file mode 100644 index 0000000..e1451b8 --- /dev/null +++ b/internal/core/errors/errors_test.go @@ -0,0 +1,225 @@ +// Package errors_test is an external test package on purpose: it imports the +// domain packages that alias these types, which an in-package test could not +// do without creating an import cycle. +package errors_test + +import ( + "Coves/internal/core/aggregators" + "Coves/internal/core/communities" + "Coves/internal/core/communityFeeds" + "Coves/internal/core/discover" + "Coves/internal/core/posts" + "Coves/internal/core/timeline" + "errors" + "fmt" + "testing" + + coreerrors "Coves/internal/core/errors" +) + +// domainValidationErrors returns one validation error per domain package that +// aliases coreerrors.ValidationError. +func domainValidationErrors() map[string]error { + return map[string]error{ + "posts": posts.NewValidationError("uris", "must not be empty"), + "communities": communities.NewValidationError("name", "must not be empty"), + "aggregators": aggregators.NewValidationError("aggregatorDid", "is required"), + "communityFeeds": communityFeeds.NewValidationError("limit", "out of range"), + "discover": discover.NewValidationError("cursor", "is malformed"), + "timeline": timeline.NewValidationError("cursor", "is malformed"), + } +} + +// This is the property the whole consolidation exists for. Without it, a +// handler has to enumerate every domain it might hear from, and the one it +// forgets returns 500 instead of 400. +// +// Converting any alias back to a locally-defined struct still compiles and +// still passes that domain's own tests — only this test notices. +func TestSingleErrorsAsMatchesEveryAliasingDomain(t *testing.T) { + for domain, err := range domainValidationErrors() { + t.Run(domain, func(t *testing.T) { + var validationErr *coreerrors.ValidationError + if !errors.As(err, &validationErr) { + t.Fatalf("%s validation error is not a *coreerrors.ValidationError; "+ + "the type alias has been broken", domain) + } + if validationErr.Field == "" { + t.Error("Field did not survive the conversion") + } + }) + } +} + +// Service layers add context with %w on the way up. A predicate that stops +// matching once wrapped silently downgrades a 400 to a 500. +func TestValidationErrorSurvivesWrapping(t *testing.T) { + for domain, err := range domainValidationErrors() { + t.Run(domain, func(t *testing.T) { + wrapped := fmt.Errorf("creating record: %w", fmt.Errorf("validating input: %w", err)) + if !coreerrors.IsValidationError(wrapped) { + t.Errorf("%s validation error stopped matching after two wraps", domain) + } + }) + } +} + +// Each domain's own predicate must agree with the shared one, in both +// directions — that agreement is what lets a boundary mapper use either. +func TestDomainPredicatesAgreeWithShared(t *testing.T) { + predicates := map[string]func(error) bool{ + "posts": posts.IsValidationError, + "communities": communities.IsValidationError, + "aggregators": aggregators.IsValidationError, + "communityFeeds": communityFeeds.IsValidationError, + "discover": discover.IsValidationError, + "timeline": timeline.IsValidationError, + } + + for domain, err := range domainValidationErrors() { + for predicateDomain, matches := range predicates { + t.Run(domain+"/"+predicateDomain, func(t *testing.T) { + if !matches(err) { + t.Errorf("%s.IsValidationError rejected a %s validation error; "+ + "cross-domain matching is the point of the shared type", + predicateDomain, domain) + } + }) + } + } + + // And the negative direction: an unrelated error must not be swept up. + for domain, matches := range predicates { + t.Run(domain+"/negative", func(t *testing.T) { + if matches(errors.New("some unrelated failure")) { + t.Errorf("%s.IsValidationError matched an unrelated error", domain) + } + if matches(nil) { + t.Errorf("%s.IsValidationError matched nil", domain) + } + }) + } +} + +// NewValidationErrorFrom exists so a sentinel from a package like +// internal/validation stays matchable after being given field context. +func TestNewValidationErrorFromKeepsCauseMatchable(t *testing.T) { + cause := errors.New("handle contains an invalid character") + err := coreerrors.NewValidationErrorFrom("handle", cause) + + if !errors.Is(err, cause) { + t.Error("the underlying cause is not reachable via errors.Is") + } + if !coreerrors.IsValidationError(err) { + t.Error("the result is not recognised as a validation error") + } + if got, want := err.Error(), "handle: "+cause.Error(); got != want { + t.Errorf("Error() = %q, want %q", got, want) + } + + // Still true once a service layer wraps it. + wrapped := fmt.Errorf("updating profile: %w", err) + if !errors.Is(wrapped, cause) { + t.Error("the cause stopped being reachable after wrapping") + } +} + +func TestValidationErrorMessageFormat(t *testing.T) { + err := coreerrors.NewValidationError("communityDid", "is required") + if got, want := err.Error(), "communityDid: is required"; got != want { + t.Errorf("Error() = %q, want %q", got, want) + } +} + +// A nil Err must not make Unwrap misbehave — ValidationError is usually +// constructed without a cause. +func TestValidationErrorUnwrapWithoutCause(t *testing.T) { + err := coreerrors.NewValidationError("field", "message") + if unwrapped := errors.Unwrap(err); unwrapped != nil { + t.Errorf("Unwrap() = %v, want nil when there is no cause", unwrapped) + } + if errors.Is(err, errors.New("anything")) { + t.Error("a causeless ValidationError should not match arbitrary errors") + } +} + +// The Is method bridges the typed error to the shared sentinel, so callers can +// ask "is this missing?" without knowing which resource was missing. +func TestNotFoundErrorMatchesSentinel(t *testing.T) { + err := coreerrors.NewNotFoundError("post", "at://did:plc:abc/social.coves.community.post/xyz") + + if !errors.Is(err, coreerrors.ErrNotFound) { + t.Error("NotFoundError does not match ErrNotFound") + } + if !coreerrors.IsNotFound(err) { + t.Error("IsNotFound rejected a NotFoundError") + } + if !errors.Is(fmt.Errorf("loading post: %w", err), coreerrors.ErrNotFound) { + t.Error("NotFoundError stopped matching ErrNotFound after wrapping") + } + if errors.Is(err, coreerrors.ErrAlreadyExists) { + t.Error("NotFoundError must not match unrelated sentinels") + } + if got, want := err.Error(), + "post not found: at://did:plc:abc/social.coves.community.post/xyz"; got != want { + t.Errorf("Error() = %q, want %q", got, want) + } +} + +func TestConflictErrorMatchesSentinel(t *testing.T) { + err := coreerrors.NewConflictError("community", "handle", "!golang@coves.social") + + if !errors.Is(err, coreerrors.ErrAlreadyExists) { + t.Error("ConflictError does not match ErrAlreadyExists") + } + if !coreerrors.IsConflict(err) { + t.Error("IsConflict rejected a ConflictError") + } + if errors.Is(err, coreerrors.ErrNotFound) { + t.Error("ConflictError must not match unrelated sentinels") + } +} + +// posts keeps its own ErrNotFound sentinel alongside the shared typed error, +// so its predicate has to bridge both. This pins that it does — an +// unbridged predicate would map a typed not-found to a 500. +func TestPostsIsNotFoundBridgesBothRepresentations(t *testing.T) { + cases := map[string]error{ + "shared typed error": posts.NewNotFoundError("post", "at://example"), + "package sentinel": posts.ErrNotFound, + "community sentinel": posts.ErrCommunityNotFound, + "wrapped sentinel": fmt.Errorf("fetching: %w", posts.ErrNotFound), + "wrapped typed": fmt.Errorf("fetching: %w", posts.NewNotFoundError("post", "at://example")), + } + for name, err := range cases { + t.Run(name, func(t *testing.T) { + if !posts.IsNotFound(err) { + t.Errorf("posts.IsNotFound rejected %s", name) + } + }) + } + + if posts.IsNotFound(errors.New("unrelated")) { + t.Error("posts.IsNotFound matched an unrelated error") + } +} + +// The predicates are called on error paths where nil is possible; none of them +// may panic. +func TestPredicatesHandleNil(t *testing.T) { + if coreerrors.IsValidationError(nil) { + t.Error("IsValidationError(nil) should be false") + } + if coreerrors.IsNotFound(nil) { + t.Error("IsNotFound(nil) should be false") + } + if coreerrors.IsConflict(nil) { + t.Error("IsConflict(nil) should be false") + } + if posts.IsNotFound(nil) { + t.Error("posts.IsNotFound(nil) should be false") + } + if posts.IsConflict(nil) { + t.Error("posts.IsConflict(nil) should be false") + } +} diff --git a/internal/core/posts/errors.go b/internal/core/posts/errors.go index 603cd70..8d2dffd 100644 --- a/internal/core/posts/errors.go +++ b/internal/core/posts/errors.go @@ -3,6 +3,9 @@ package posts import ( "errors" "fmt" + "strings" + + coreerrors "Coves/internal/core/errors" ) // Sentinel errors for common post operations @@ -33,49 +36,26 @@ var ( ErrActorNotFound = errors.New("actor not found") ) -// ValidationError represents a validation error with field context -type ValidationError struct { - Field string - Message string - // Err is the underlying cause, when there is one. It keeps sentinels from - // packages like internal/validation matchable with errors.Is after the - // error has been given field context — without it, wrapping a typed error - // here would flatten it to a string and the sentinel would be unreachable - // from any caller. - Err error -} - -func (e *ValidationError) Error() string { - return fmt.Sprintf("validation error (%s): %s", e.Field, e.Message) -} - -// Unwrap exposes the underlying cause to errors.Is/errors.As. -func (e *ValidationError) Unwrap() error { - return e.Err -} +// ValidationError is the shared validation error type. It is aliased rather +// than redefined so that one errors.As at the API boundary matches validation +// failures from every domain package, instead of each handler needing to know +// which domains it might hear from. +type ValidationError = coreerrors.ValidationError // NewValidationError creates a new validation error func NewValidationError(field, message string) error { - return &ValidationError{ - Field: field, - Message: message, - } + return coreerrors.NewValidationError(field, message) } // NewValidationErrorFrom creates a validation error that keeps cause matchable // via errors.Is while presenting a field-scoped message to the client. func NewValidationErrorFrom(field string, cause error) error { - return &ValidationError{ - Field: field, - Message: cause.Error(), - Err: cause, - } + return coreerrors.NewValidationErrorFrom(field, cause) } // IsValidationError checks if error is a validation error func IsValidationError(err error) bool { - var valErr *ValidationError - return errors.As(err, &valErr) + return coreerrors.IsValidationError(err) } // ContentRuleViolation represents a violation of community content rules @@ -103,51 +83,41 @@ func IsContentRuleViolation(err error) bool { return errors.As(err, &violation) } -// NotFoundError represents a resource not found error -type NotFoundError struct { - Resource string // e.g., "post", "community" - ID string // Resource identifier -} - -func (e *NotFoundError) Error() string { - return fmt.Sprintf("%s not found: %s", e.Resource, e.ID) -} +// NotFoundError is the shared not-found error type, aliased for the same +// reason as ValidationError above. +type NotFoundError = coreerrors.NotFoundError // NewNotFoundError creates a new not found error func NewNotFoundError(resource, id string) error { - return &NotFoundError{ - Resource: resource, - ID: id, - } + return coreerrors.NewNotFoundError(resource, id) } -// IsNotFound checks if error is a not found error +// IsNotFound checks if error is a not found error. +// +// errors.Is rather than == : these sentinels travel up through service layers +// that wrap them with %w for context, and an == comparison stops matching the +// moment anyone adds that context — silently turning a 404 into a 500. func IsNotFound(err error) bool { - var notFoundErr *NotFoundError - return errors.As(err, ¬FoundErr) || err == ErrCommunityNotFound || err == ErrNotFound + return coreerrors.IsNotFound(err) || + errors.Is(err, ErrCommunityNotFound) || + errors.Is(err, ErrNotFound) } -// IsConflict checks if error is due to duplicate/conflict +// IsConflict checks if error is due to duplicate/conflict. +// +// This inspects the message because the conflict usually originates in the +// PostgreSQL driver as a unique-violation string rather than as a typed error +// the repository translates. Typed conflicts (coreerrors.ConflictError) are +// checked first so callers that do return one are matched exactly. func IsConflict(err error) bool { if err == nil { return false } - // Check for common conflict indicators in error message - errStr := err.Error() - return contains(errStr, "already indexed") || - contains(errStr, "duplicate key") || - contains(errStr, "already exists") -} - -func contains(s, substr string) bool { - return len(s) >= len(substr) && anySubstring(s, substr) -} - -func anySubstring(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } + if coreerrors.IsConflict(err) { + return true } - return false + message := err.Error() + return strings.Contains(message, "already indexed") || + strings.Contains(message, "duplicate key") || + strings.Contains(message, "already exists") } diff --git a/internal/core/timeline/types.go b/internal/core/timeline/types.go index 95e53d1..6b710c6 100644 --- a/internal/core/timeline/types.go +++ b/internal/core/timeline/types.go @@ -1,6 +1,7 @@ package timeline import ( + coreerrors "Coves/internal/core/errors" "Coves/internal/core/posts" "context" "errors" @@ -85,26 +86,22 @@ var ( ErrUnauthorized = errors.New("unauthorized") ) -// ValidationError represents a validation error with field context -type ValidationError struct { - Field string - Message string -} - -func (e *ValidationError) Error() string { - return e.Message -} +// ValidationError is the shared validation error type. It is aliased rather +// than redefined so that one errors.As at the API boundary matches validation +// failures from every domain package, instead of each handler needing to know +// which domains it might hear from. +type ValidationError = coreerrors.ValidationError // NewValidationError creates a new validation error func NewValidationError(field, message string) error { - return &ValidationError{ - Field: field, - Message: message, - } + return coreerrors.NewValidationError(field, message) } -// IsValidationError checks if an error is a validation error +// IsValidationError checks if an error is a validation error. +// +// This unwraps, unlike the bare type assertion it replaces: a validation error +// that any layer wrapped with %w for context used to stop being recognised +// here, and fell through to a 500 instead of a 400. func IsValidationError(err error) bool { - _, ok := err.(*ValidationError) - return ok + return coreerrors.IsValidationError(err) } -- 2.51.2