Repository navigation
feat(httputil)!: return translatable errors with a code, message key and parameters - #333
mushrafmim wants to merge 1 commit into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
| Layer / File(s) | Summary |
|---|---|
Message representation and interpolation httputil/message.go, httputil/message_test.go |
Message carries a translation key, default text, and optional parameters. Interpolation substitutes matching placeholders, and JSON encoding retains parameters. |
Categorized errors and response writing httputil/errors.go, httputil/errors_test.go, httputil/response.go, httputil/response_test.go, httputil/README.md, README.md |
Error adds categories, messages, details, and wrapping. WriteError writes categorized responses and logs applicable errors. The previous response type and helper functions are removed. Documentation describes the new workflow. |
Error catalog validation httputil/catalog.go, httputil/catalog_test.go |
Catalog returns default message values when declarations pass validation, and reports declaration or key conflicts. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant HTTPHandler
participant WriteError
participant CorrelationID
participant slog
participant ResponseWriter
HTTPHandler->>WriteError: pass request and error
WriteError->>CorrelationID: get request correlation ID
WriteError->>slog: log unrecognized error or wrapped context
WriteError->>ResponseWriter: write status and structured error body
Suggested reviewers: saknarajapakshe
Merge Risk: 
7647d
-
The new error API is ready to merge after normal checks. Consumers will need to adopt the documented breaking API and response changes.
Pre-merge checks: 
-
Check name Status Explanation Resolution Linked Issues check The implementation satisfies the main #332requirements: it adds keyed messages with{{name}}parameters, stable codes, centralized category status mapping, Go error compatibility, validation detail…Extend the catalog generation API or declaration model so declared FieldError.Messagekeys and English defaults are included. Add tests that verify those keys appear in the catalog and generateden.json.Docstring Coverage Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 7 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main breaking change: translatable httputil errors with stable codes, message keys, and parameters. Description check The description follows the required template and documents the purpose, breaking changes, implementation details, testing, checklist, issue reference, and known limitation. Out of Scope Changes check The changed implementation, tests, and documentation stay within #332. The new error types,WriteError, catalog validation, response changes, and removal of the old helpers implement the requested b…Full details: Out of Scope Changes check
-
Explanation
The changed implementation, tests, and documentation stay within
#332. The new error types,WriteError, catalog validation, response changes, and removal of the old helpers implement the requested breaking API replacement. The tests support those objectives, and no unrelated product behavior is introduced.
-
Full details: Linked Issues check
-
Explanation
The implementation satisfies the main
#332requirements: it adds keyed messages with{{name}}parameters, stable codes, centralized category status mapping, Go error compatibility, validation details, safeWriteError, and catalog validation. However,Catalogonly addsError.MessageandError.Description(httputil/catalog.go). It does not include keys used only byFieldError.Message, although#332requires a catalog of every declared error key and requires translatable validation errors. The PR description confirms this limitation.Resolution
Extend the catalog generation API or declaration model so declared
FieldError.Messagekeys and English defaults are included. Add tests that verify those keys appear in the catalog and generateden.json.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 7 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
sthanikan2000
left a comment
sthanikan2000
left a comment
There was a problem hiding this comment.
Thanks for this. Stable codes, the HTTP status coming from a category, and unknown errors safely falling back to ErrInternal are all real improvements, and the tests are thorough. I'm requesting changes on three points: where these types live, a few heuristics, and how much OpenNSW-specific policy ends up in httputil.
Where the types live. As written, service code is meant to return httputil.Error (see the README example). So domain packages, repositories and Temporal activities would all import an HTTP package just to report "not found". Only the status table and WriteError are actually about HTTP; the message type, error type and catalog aren't. I'd split it so imports point from the HTTP side to the domain side:
apperror/ no net/http; service code imports only this
Message, Category, Error, FieldError, Catalog
apperror/httperror/ the only package that knows both
WriteError, the category→status table, the response body type
httputil/ stays generic, no dependencies
JSON, CorrelationID, CorrelationIDFunc, plus a general status + body error writer
Names are up to you; the import direction is what matters.
Keeping httputil generic. With Error(w, r, status, msg) removed, the only way to write an error is through 8 categories. This repo's own storage (413, 415) and payment (422) handlers can't move over without changing their status codes. The code format, the SYS-5000 reservation, English defaults and i18next syntax are conventions for our services. That's another reason to keep them out of a shared HTTP helper.
Details are inline.
| } | ||
| ``` | ||
|
|
||
| Return it as an ordinary `error` from anywhere, and write it with `WriteError`: |
There was a problem hiding this comment.
This is the pattern I'm most concerned about. If services return httputil.Error, every package that wants a typed client error depends on httputil. That includes packages that never touch HTTP, like Temporal activities, taskflow and workflow. CategoryNotFound is fine as a domain concept, but declaring it shouldn't need an HTTP package. See the split suggested in the review summary.
There was a problem hiding this comment.
These are HTTP response errors by design, not domain errors. The handler maps a service's own errors (e.g. member.ErrNotFound) to an httputil.Error and writes it. Services, repositories, Temporal activities, taskflow and workflow return their own errors and never import httputil. Activities don't respond to a frontend anyway, and a Go error type wouldn't survive Temporal's serialization.
The README example was misleading: it returned ErrMemberNotFound from a service method. I've updated the README and the Error doc comment so the mapping happens in the handler; that goes up with my next push. With that boundary, a transport-neutral apperror package would have no second consumer, so I'd rather not split until one exists.
There was a problem hiding this comment.
This error pattern is specifically to send error response to frontends, if some service is not supposed to send errors to the frontend, they doesn't need to use this package.
| CategoryInternal Category = "internal" // 500 Internal Server Error | ||
| ) | ||
|
|
||
| var categoryStatus = map[Category]int{ |
There was a problem hiding this comment.
The category set is closed and the table is unexported, so any other category becomes a 500. With Error(w, r, status, msg) removed, there's no longer a way to send:
- 413 (
storage/local_content_handler.go:107) - 415 (
storage/http_handler.go:70) - 422 (
payment/handler.go:114) - 405, 502 or 504
Could we add categories for at least these (e.g. tooLarge, unsupportedMediaType, unprocessable, methodNotAllowed, badGateway, gatewayTimeout)? And could httputil keep a general writer for anything outside the table?
| // | ||
| // errors.Is(err, &ErrX) matches any error derived from ErrX, since errors | ||
| // are compared by Code. | ||
| type Error struct { |
There was a problem hiding this comment.
One type is both the declared error and the error that gets returned, and it relies on mixed receivers to do it. The builder methods take the value, while Error/Is/Unwrap take the pointer. A side effect: return &ErrX (which the doc comment recommends) and apiErr = &ErrInternal hand out a pointer to the package-level var. Anything that gets it through errors.As can then change the declaration for every later request:
var got *httputil.Error
errors.As(fmt.Errorf("x: %w", &ErrMemberNotFound), &got)
got.Message.DefaultValue = "changed" // ErrMemberNotFound is now changed everywhereEither of these would make declarations immutable and drop the mixed receivers:
- a separate definition type whose methods always return a new
*Error - unexported fields set by a constructor
| } | ||
|
|
||
| // errorBody is the JSON shape of an API error response. | ||
| type errorBody struct { |
There was a problem hiding this comment.
The response shape is the public contract, but its type is unexported; the old ErrorResponse was exported. Go callers such as remote, service-to-service clients and consumers' tests would each have to redefine it to decode an error. Could we export it?
| // from Wrap, or context added by wrapping it in another error. Server | ||
| // categories (internal, unavailable) log at error level, client categories | ||
| // at warn level. | ||
| func WriteError(w http.ResponseWriter, r *http.Request, err error) { |
There was a problem hiding this comment.
The writer now owns the logging policy: whether to log, at what level, and with a fixed message. InternalServerError let the caller pass a message and structured attrs ...any. Here the only way to add context is wrapping with fmt.Errorf, so a field like itemId ends up as text inside error. Would WriteError(w, r, err, attrs ...any), passed through to the log line, work? It keeps the old behaviour without changing the response.
| if !errors.As(err, &apiErr) || apiErr == nil { | ||
| slog.ErrorContext(r.Context(), "httputil: unhandled error", "error", err, "correlationId", correlationID) | ||
| apiErr = &ErrInternal | ||
| } else if apiErr.cause != nil || err != error(apiErr) { |
There was a problem hiding this comment.
err != error(apiErr) compares pointers as a stand-in for "the caller added context". Wrappers that add nothing all trip it, so a plain 404 gets logged at WARN:
WriteError(w, r, errors.Join(&ErrMemberNotFound)) // WARN
WriteError(w, r, fmt.Errorf("%w", &ErrMemberNotFound)) // WARN
WriteError(w, r, tracingWrapper{&ErrMemberNotFound}) // WARNWhether a 4xx gets logged then depends on how deep in the call stack it was wrapped, and a burst of 401s or 429s becomes a burst of log lines. Making Wrap the only way to opt in would be explicit:
} else if apiErr.cause != nil {| func (m Message) WithParams(params map[string]string) Message { | ||
| var used map[string]string | ||
| for name, value := range params { | ||
| if strings.Contains(m.DefaultValue, "{{"+name+"}}") { |
There was a problem hiding this comment.
Filtering params by searching the English DefaultValue has two side effects:
- The English wording decides which params other languages receive. A param that a translation needs but the English text doesn't use is dropped.
- Only the bare
{{name}}form is recognised.{{max, number}},{{ max }}and{{- max}}are all valid i18next, but each one drops the param and the client gets the raw placeholder asdefaultValue:
Message{DefaultValue: "At most {{max, number}}"}.WithParams(map[string]string{"max": "64"})
// Params: nil; defaultValue sent as "At most {{max, number}}"The filter is only needed because one WithParams call fills both Message and Description. Sending the full map, or putting params once on the error, avoids it. If filling in placeholders stays on the server, either handle those forms or document that only the bare form works.
| // client can either show the English text or translate Key with Params. | ||
| func (m Message) MarshalJSON() ([]byte, error) { | ||
| type plain Message | ||
| return json.Marshal(plain{Key: m.Key, DefaultValue: m.String(), Params: maps.Clone(m.Params)}) |
There was a problem hiding this comment.
Nit: maps.Clone isn't needed here, since json.Marshal doesn't modify the map.
| "regexp" | ||
| ) | ||
|
|
||
| var codePattern = regexp.MustCompile(`^[A-Z][A-Z0-9]*-[0-9]{4}$`) |
There was a problem hiding this comment.
The code format (^[A-Z][A-Z0-9]*-[0-9]{4}$) and the reserved SYS-5000 are OpenNSW conventions. If they stay, they belong with the error types (the apperror split in the summary) rather than in an HTTP helper. Alternatively, the consumer could pass the pattern in.
There was a problem hiding this comment.
Why this is NSW Specific? This is a generic pattern anybody can adopt based on their needs while adhering to this pattern, if we need anything specific in the future then we can make it customizable. You Aren't gonna need it (YAGNI)
| catalog[m.Key] = m.DefaultValue | ||
| } | ||
|
|
||
| for _, e := range append([]Error{ErrInternal}, errs...) { |
There was a problem hiding this comment.
Minor: Catalog adds ErrInternal on its own, and sameDeclaration exists only to tolerate that. It also compares fields by hand, so it will drift when a field is added. Letting callers list ErrInternal themselves would remove both.
On the known gap in the description: FieldErrors are written inline where they're returned and never reach Catalog. So the validation messages are exactly the ones translators won't see. It may be worth fixing that here rather than in a follow-up, e.g. by letting Catalog accept declared FieldErrors.
…and parameters
Replace the plain-string ErrorResponse with a declared Error type: a stable
code, a category that decides the HTTP status, and a message and description
carried as {key, defaultValue, params}. WriteError writes any *Error with its
category's status and sends every other error as ErrInternal, so internal
error text never reaches the client. Catalog lists every key with its English
default and catches duplicate codes and keys.
BREAKING CHANGE: ErrorResponse, Error(w, r, status, message) and
InternalServerError are removed in favour of Error, WriteError and
ErrInternal, and the error response body changes shape.
Closes OpenNSW#332
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7647da9 to
cddcb6c
Compare
| } | ||
|
|
||
| // Error is a client-facing API error. Declare each one once, as a | ||
| // package-level value, and return it as an error with &ErrX, ErrX.WithParams, |
There was a problem hiding this comment.
Blocking: a declared error is one shared value, so one request can change it for every later request.
httputil/errors.go:49 tells callers to return &ErrX. Error at httputil/errors.go:66 exports Code, Category, Message, and Description, and Error, Unwrap, and Is at httputil/errors.go:125, httputil/errors.go:134, and httputil/errors.go:140 use a pointer receiver. errors.As therefore returns the package-level variable. This writes through to every user of that declaration:
var got *httputil.Error
errors.As(fmt.Errorf("load: %w", &ErrMemberNotFound), &got)
got.Message.DefaultValue = "changed"
The same thing happens if a handler fills a parameter on &ErrX. A member id from one request then shows up in later responses. WithParams, WithDetails, and Wrap at httputil/errors.go:104, httputil/errors.go:112, and httputil/errors.go:119 are safe only because the receiver is a value, so they copy first. &ErrX skips that copy. WriteError also keeps &ErrInternal at httputil/errors.go:169.
Suggested fix: keep the package-level declaration as its own type, and make every method that returns an error return a new *Error (clone details with slices.Clone). Change httputil/errors.go:49 so the no-parameter return uses that method, for example ErrX.Err(), and use the same copy at httputil/errors.go:169.
Summary
httputil.Errorwrote{"error": "<English sentence>", "correlationId": "..."}. Clients couldn't translate it, couldn't tell errors apart without parsing text, and the status was picked by hand at every call site. Call sites also often passederr.Error()as the message, which leaked internal error text.This PR replaces that with errors that are declared once. Each has a stable code, a category that decides the HTTP status, and a message and description carried as
{key, defaultValue, params}. Placeholders use the i18next{{name}}syntax, so frontends can translate in the UI. A singleWriteErrorsends anything that isn't a declared error asErrInternal, so internal error text never reaches the client.Type of Change
Changes Made
message.go:Message{Key, DefaultValue, Params}. In the JSON body,defaultValuehas its{{name}}placeholders filled in (in a single pass, so a param value is never expanded again) andparamsis kept.WithParamsgives each message only the params its text uses.errors.go:Category, with one table mapping it to an HTTP status:invalid400,unauthenticated401,forbidden403,notFound404,conflict409,tooManyRequests429,unavailable503,internal500. An unknown category gives 500.Error, withWithParams,WithDetailsandWrap. Each returns a copy, so declared values are never changed.Errorimplementserror,Unwrap, and anIsthat compares by code, soerrors.Is(err, &ErrX)matches afterWithParams/Wrap.FieldErrorfor validation details.ErrInternal(SYS-5000).WriteError(w, r, err). It writes an*Error, found witherrors.As, with its category's status. Any other error, includingnil, is logged with the correlation ID and sent asErrInternal. An*Erroris logged only when it carries a cause or wrapping context: at error level for 5xx categories, warn level otherwise.catalog.go:Catalog(errs ...Error)returns every key with its English default, includingErrInternal's, for a test and ago generatestep that writesen.json. It reports duplicate codes, keys with conflicting defaults, malformed codes (^[A-Z][A-Z0-9]*-[0-9]{4}$), unknown categories and missing keys, all in oneerrors.Join.response.go: removedErrorResponse,Error(w, r, status, message)andInternalServerError.JSON,CorrelationIDandCorrelationIDFuncare unchanged.httputil/README.md(declaring, returning, writing, validation, catalog) and updated the root README row.Response body:
{ "code": "PB-1003", "message": { "key": "pb.member.not_found", "defaultValue": "Member not found" }, "description": { "key": "pb.member.not_found_description", "defaultValue": "No member has the ID m-42", "params": { "memberId": "m-42" } }, "details": [], "correlationId": "c0ffee" }descriptionis omitted when empty,detailsis always an array, andcorrelationIdis omitted whenCorrelationIDFuncis unset.Testing
go test -race ./...inhttputilpasses with 100% statement coverage. The tests cover:WithDetailscopies don't share sliceserrors.Is/Asthrough wrappingnilerrors never leakCatalogrejection caseChecklist
Related Issues
Closes #332
Screenshots/Demo
N/A
Additional Notes
httputil.Error(...)with a declaredhttputil.Errorreturned throughWriteError, andInternalServerError(...)with returning the wrapped error toWriteError. The error body changes shape, so frontends need updating too. No module in this repo importshttputil.Catalogonly readsErrorvalues, so keys used only inFieldErrormessages aren't in the generated catalog yet. That can be a follow-up, for example by lettingCatalogalso accept standaloneMessages.🤖 Generated with Claude Code
Summary by CodeRabbit