From ce8d7efb675a15840b5ee274d11625d920827147 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 19 Oct 2026 12:53:17 +0200 Subject: [PATCH] fix(cabana): translate hook ValidationError phrase keys in the request locale - One shared localizer for ValidationError and ForbiddenError, tested in writeCRUDError's order; replaces the ForbiddenError-only one. - Applied on the CRUD and relation service transactions, the detached bulk branch, runAction, the record read (ShowRecord), and the reorder, singleton and relation-options routes, which localized neither type. - Only strings containing "::" are looked up, so translated text passes through unchanged and logs no missing-key warning. - Copy-on-write: the plugin's error value is never modified. - README and backend docs describe the localized ValidationError. --- docs/backend/admin-controllers.md | 8 +- docs/backend/lists-and-filters.md | 2 +- docs/backend/partials-and-widgets.md | 2 +- modules/cabana/README.md | 6 +- modules/cabana/actions.go | 14 +- modules/cabana/crud.go | 125 +++++++++----- modules/cabana/field_type.go | 12 +- modules/cabana/localize_error_test.go | 189 ++++++++++++++++++++++ modules/cabana/phase121_fixture_test.go | 11 +- modules/cabana/phase121_forbidden_test.go | 21 +++ modules/cabana/relation.go | 5 +- modules/cabana/relation_field.go | 3 +- modules/cabana/reorder.go | 6 +- modules/cabana/reorder_hook_test.go | 36 +++++ modules/cabana/singleton.go | 3 +- 15 files changed, 376 insertions(+), 67 deletions(-) create mode 100644 modules/cabana/localize_error_test.go diff --git a/docs/backend/admin-controllers.md b/docs/backend/admin-controllers.md index b844e6f7..d7519fe8 100644 --- a/docs/backend/admin-controllers.md +++ b/docs/backend/admin-controllers.md @@ -269,13 +269,15 @@ A hook or an action stops a write by returning an error. Which error decides wha | Error | Answer | |-------|--------| -| `cabana.ValidationError` | 422 `validation_failed`, with its `Details` as messages per field. | +| `cabana.ValidationError` | 422 `validation_failed`, with its `Details` as messages per field; each message that is a translation key is translated in the request locale. | | `cabana.ForbiddenError` | 403 `forbidden`, with its `Message` and its `Details`. | | any other error | The opaque 500. The error is logged on the server and its text never reaches the client. | Return a `cabana.ForbiddenError` when the signed-in administrator may open the screen but may not make this particular change, for example editing a record that needs a higher permission. `Message` is a translation key or text; cabana translates it, and every `Details` message, in the request locale. `Details` maps a field name to a list of messages, as a `cabana.ValidationError` does, and may be left out. With an empty `Message` the admin shows its own text. -Every one of these errors rolls the write's transaction back, so a refused write changes nothing: a bulk action that refuses on its third record leaves the first two untouched. The admin SPA shows a refused save as a banner above the form and keeps what the administrator typed; a refused delete or action is a toast. A `cabana.ForbiddenError` works the same from the form hooks, the relation hooks, and bulk, record, toolbar and widget actions. +A `cabana.ValidationError` message may be a translation key too. A hook that returns `acme.blog::lang.errors.slug_taken` on the `slug` field shows the administrator the sentence in their language; text that is not a key, such as an already translated message, is sent as written. cabana translates a copy, so the error can be a shared package variable. + +Every one of these errors rolls the write's transaction back, so a refused write changes nothing: a bulk action that refuses on its third record leaves the first two untouched. The admin SPA shows a refused save as a banner above the form and keeps what the administrator typed; a refused delete or action is a toast. Both errors work the same, translated in the request locale, from the form hooks, the relation hooks, bulk, record, toolbar and widget actions, the reorder hook (`pact.ReorderAfterSave`) and the singleton record (`pact.AdminSingleton`). ## Toolbar actions @@ -441,7 +443,7 @@ The admin SPA shows the declared actions in a "Bulk actions" menu next to the se - The ids are resolved and row-locked through the controller's `pact.ListExtendQuery` scope inside one transaction. `Run` receives the loaded records in `pact.AdminBulkActionInput`, never the ids, so an id outside the scope cannot reach plugin code. - A selection that matches no row in the scope answers `affected: 0` without calling `Run`. A selection of which only a part is in the scope answers 409 `conflict` and changes nothing. -- `Run` writes through `cabana.TxFromContext(ctx)`. Any error rolls every row back; a `cabana.ValidationError` is a 422 and any other error the opaque 500. +- `Run` writes through `cabana.TxFromContext(ctx)`. Any error rolls every row back; a `cabana.ValidationError` is a 422 with its messages translated in the request locale, and any other error the opaque 500. - The action's `Permissions` are checked on top of the controller's. An administrator who may not run an action does not get it in the list schema, and posting it answers 403. - `pact.AdminBulkActionResult` carries an optional `Message` (a phrase key or text) and `Affected`, the number of records the action changed. The answer is a `cabana.BulkActionResult`. - An action with `Detached: true` runs outside the framework transaction. Its records are loaded through the same scope, with the same empty and 409 rules, but without row locks, and `cabana.TxFromContext` reports no transaction. `Run` opens the short transactions it needs (`lagoon.Transaction`). Set it on an action that calls an external service, so that no transaction or row lock is held across the call. What the action saved before a later failure stays saved. diff --git a/docs/backend/lists-and-filters.md b/docs/backend/lists-and-filters.md index a4198a8d..eda909ae 100644 --- a/docs/backend/lists-and-filters.md +++ b/docs/backend/lists-and-filters.md @@ -284,7 +284,7 @@ A save posts `{partition, nodes: [{id, parent_id, sort_order}]}` to `POST .../re - A duplicate id, a parent outside the set, a cycle and a node deeper than `maxDepth` are 422 `validation_failed` on `nodes`; a partitioned page without `partition` is a 422 on `partition`. - Otherwise the parent and sort columns of every node are written, without running the model's save hooks, and the answer is the new tree. -The model's save hooks do not run on a reorder, and neither do the controller's `pact.FormAfterUpdate` hooks. A controller that has to react to a new order implements `pact.ReorderAfterSave`: `ReorderAfterSave(ctx, partition)` runs after every node was written, inside the same transaction (`cabana.TxFromContext` returns it), with the posted partition, or nil when the reorder has none. A returned error rolls the whole reorder back. The transaction is a `lagoon.Transaction`, so work registered with `lagoon.AfterCommit` runs only once the new order is committed; purging a site's page cache after a menu reorder is the typical case (`cooler.PurgeSiteCacheAfterCommit`, see [Site cache purge](../services/caching.md#site-cache-purge)). +The model's save hooks do not run on a reorder, and neither do the controller's `pact.FormAfterUpdate` hooks. A controller that has to react to a new order implements `pact.ReorderAfterSave`: `ReorderAfterSave(ctx, partition)` runs after every node was written, inside the same transaction (`cabana.TxFromContext` returns it), with the posted partition, or nil when the reorder has none. A returned error rolls the whole reorder back. A `cabana.ValidationError` is answered 422 and a `cabana.ForbiddenError` 403, both with their messages translated in the request locale; any other error is the opaque 500. The transaction is a `lagoon.Transaction`, so work registered with `lagoon.AfterCommit` runs only once the new order is committed; purging a site's page cache after a menu reorder is the typical case (`cooler.PurgeSiteCacheAfterCommit`, see [Site cache purge](../services/caching.md#site-cache-purge)). Both routes need the controller's permissions; the save is a write route (CSRF header, and a personal token needs a scope that allows POST). diff --git a/docs/backend/partials-and-widgets.md b/docs/backend/partials-and-widgets.md index 027a1ea9..02c3ee6a 100644 --- a/docs/backend/partials-and-widgets.md +++ b/docs/backend/partials-and-widgets.md @@ -70,7 +70,7 @@ lookup: - `action` names an action the controller registers through `pact.HasAdminActions`. - `fill` lists the writable scalar fields of the same form the action may write back. -The SPA posts `record_id`, the fill snapshot `values` and, when the element supplied one, `payload` to `.../widgets/{field}`. cabana checks the CSRF header, the controller's and the action's permissions and the record scope, then calls the action's `Run` with a `pact.AdminActionInput`. The action reads the payload from the `Payload` field as raw JSON (any value up to 64 KiB, exactly the bytes the client sent, nil when absent) and decodes it itself; it must treat any ids inside the payload as untrusted input and re-check them against its own scope, because the framework never uses the payload to select the record. The answer's `pact.AdminActionResult` carries a message and the fill values; keys outside `fill` and values that are not scalars are dropped before the response is written. It may also carry `Data`, any JSON value up to 256 KiB encoded, which is passed through as `data` and is not filtered like `fill` (a larger or unencodable value is a 500). Toolbar and record actions refuse a payload. An action may return a `cabana.ValidationError` to answer 422 on a field. +The SPA posts `record_id`, the fill snapshot `values` and, when the element supplied one, `payload` to `.../widgets/{field}`. cabana checks the CSRF header, the controller's and the action's permissions and the record scope, then calls the action's `Run` with a `pact.AdminActionInput`. The action reads the payload from the `Payload` field as raw JSON (any value up to 64 KiB, exactly the bytes the client sent, nil when absent) and decodes it itself; it must treat any ids inside the payload as untrusted input and re-check them against its own scope, because the framework never uses the payload to select the record. The answer's `pact.AdminActionResult` carries a message and the fill values; keys outside `fill` and values that are not scalars are dropped before the response is written. It may also carry `Data`, any JSON value up to 256 KiB encoded, which is passed through as `data` and is not filtered like `fill` (a larger or unencodable value is a 500). Toolbar and record actions refuse a payload. An action may return a `cabana.ValidationError` to answer 422 on a field; its messages may be translation keys, translated in the request locale. ### Widget element contract diff --git a/modules/cabana/README.md b/modules/cabana/README.md index 7214c8fe..f28c49fd 100644 --- a/modules/cabana/README.md +++ b/modules/cabana/README.md @@ -52,7 +52,7 @@ Use cases: - Money fields: a `type: money` field in `fields.yaml` edits an exact decimal amount that travels as a decimal string with a dot and at most `scale` (2 to 4, default 4) significant fraction digits (trailing zeros beyond the scale are accepted unchanged), or null; the server never parses it into a float, never rounds it and never truncates it, and fills the accepted string into a string column or a type with its own `sql.Scanner` and `driver.Valuer` unchanged. A field declares exactly one of `currency` (an ISO 4217 code) or `currencyFrom` (a sibling string column holding the code) and may declare `max` (the column ceiling); two sources, none, a scale outside 2..4, a max beyond the scale, a column of another type or a currencyFrom that is not a string column fails boot naming the field. A save answers 422 on the field with `backend::lang.money.invalid`, `money.too_precise` (`:scale`) or `money.too_large` (`:max`, compared as scaled digit strings), on controller forms, settings forms and relation forms alike. The schema serves `currency`, `currencyFrom`, `scale` and `max` on the field. A list column `type: money` takes the same `currency` or `currencyFrom` key, is never searchable and not sortable by default, and every list row carries the `currencyFrom` column's value under its own key even when it is not a declared column; the admin right-aligns the locale-formatted amount with its ISO code. - Tag lists: a `type: taglist` field in `fields.yaml` edits a list of tags (WinterCMS TagList). `mode: array` binds a JSON column (a type with `sql.Scanner` and `driver.Valuer`, such as `lagoon.Jsonable[[]string]`) and travels as a JSON array of strings; `mode: string`, the default, binds a string column. `options` is a YAML list of suggested tags; `separator` (`comma` or `space`, string mode only) and `customTags` are refused on other types, and an unknown mode or separator, options that are not a list, the money keys, `nameFrom`, `emptyOption`, `dependsOn`, `preset` or a column of the wrong Go type fails boot naming the field. A save trims every tag, drops empty ones and keeps a repeated tag once; in array mode null or an empty list stores `[]`, in string mode the tags are split on the separator and stored joined by `,` or a single space (null stores an empty string). A value of the wrong shape is a 422 on the field with `backend::lang.taglist.invalid`, and with `customTags: false` a tag outside `options` is a 422 with `backend::lang.taglist.not_allowed` (`:tag`), on controller forms, settings forms and relation forms alike; a taglist in a repeater row fails boot. The record GET serves the stored list (array mode) or string (string mode). The schema serves `mode`, `options`, `separator` and `customTags` on the field; the admin edits the tags as chips. - File uploads: a `type: fileupload` field in `fields.yaml` edits an attachOne or attachMany relation the record model declares through `attach.HasRelations` (its `AttachRelations` method) next to `attach.Owner`. The field accepts WinterCMS's `mode` (`image` or `file`), `fileTypes`, `mimeTypes`, `maxFilesize` (megabytes), `maxFiles` (attachMany only), `imageWidth`, `imageHeight`, `thumbOptions` (only `mode`: `auto`, `exact`, `crop` or `fit`), `useCaption` and `prompt`; any other key, an image-mode file type outside jpg, jpeg, png, gif and webp, a name that is not a declared relation or a `maxFilesize` whose file plus 64 KiB of multipart framing exceeds `http.body_limits.upload_bytes` fails boot. Uploads and removals are deferred, as in WinterCMS: the SPA sends a random form session key in the `X-Session-Key` header (`cabana.SessionKeyHeader`) with every file call and with the save, the server keeps the pending work in `deferred_bindings` against that key and the signed-in administrator, and the record's next create or update save applies it inside its transaction. A retry of the same upload may send `X-Upload-Id` so the server returns the already stored file. A save that fails with 422 keeps the pending uploads; another administrator's key matches nothing. The upload route caps the request body at the smaller of `http.body_limits.upload_bytes` and `maxFilesize` plus 64 KiB and answers 413 `payload_too_large` past it; the size, type and image checks run on the server (through `attach.Store`) and answer 422 on the field. A file list (`cabana.FileItem`) carries `url` and `thumb_url` only for a public relation. -- Refusals: a lifecycle hook, a relation hook or a bulk, record, toolbar or widget action that returns a `cabana.ForbiddenError` is answered 403 `forbidden` with the error's `Message` and `Details` (a field name to a list of messages), both translated in the request locale; an empty `Message` stays empty. The surrounding transaction is rolled back. Every other error that is not a `cabana.ValidationError` stays the opaque 500, logged on the server. +- Refusals: a lifecycle hook, a relation hook, a bulk, record, toolbar or widget action, a `pact.ReorderAfterSave` hook or a `pact.AdminSingleton` `SingletonRecord` call that returns a `cabana.ForbiddenError` is answered 403 `forbidden` with the error's `Message` and `Details` (a field name to a list of messages), both translated in the request locale; an empty `Message` stays empty. A `cabana.ValidationError` returned from the same places is answered 422 `validation_failed` with its `Details`, each message that is a translation key (`vendor.plugin::group.key`) translated in the request locale and any other text passed through unchanged. The plugin's error value is never modified, so it may be a shared variable. The surrounding transaction is rolled back. Every other error stays the opaque 500, logged on the server. - Danger record actions: `pact.AdminRecordAction.Danger` is served as `danger` on the record response's `cabana.RecordAction` (absent when false, so existing payloads are unchanged); the admin's confirm dialog then uses its danger variant. - Singleton settings screens declared with `pact.HasSettings`, read and saved by `cabana.SettingsService`. A `type: password` field on a settings form is a write-only secret on a `lagoon.Encrypted` column (any other column type fails boot naming the field): the GET omits it from `data` and reports `stored`, `env` or `empty` per field in `meta.secrets` (`cabana.SettingsMeta`; `env` when the column is empty and the model's `cabana.SecretFallbacks` config key resolves to a non-empty value); on PUT an absent key or an empty string keeps the stored value, JSON null clears it, a non-string is a 422 on the field, and the answer carries the new states, never a value. A `type: richeditor` on a settings form is sanitized on save exactly like a controller save, with the removals in `meta.notices`; `repeater` and `dependsOn` stay refused. Settings fields keep their `tab`. A `type: dropdown` whose `options` names a method takes its choices from the settings model, which must implement `pact.DropdownOptionsProvider` (boot fails naming the field otherwise); the settings schema serves them localized. A settings entry whose `Controller` field is set links to that admin controller instead: start-up fails when it also declares a model, form or model factory, or names an unknown controller; `GET /settings` lists it, with its `controller`, only to administrators who pass its permissions and may open the controller; and the singleton settings endpoints answer 404 for its code. - Backend navigation (`pact.HasNavigation`) and permissions (`pact.HasPermissions`), filtered per user by `cabana.Registry.Metadata`. An entry whose `pact.NavigationItem.Visible` func answers false for the request is left out like a denied one (a hidden main item with its side menu), and a main item whose controller is a side item hidden this way links to its first visible openable side item. An administrator's own `backend_users.permissions` are merged over the role's as in Winter (a `-1` denies a code the role grants). `cabana.Allows` implements the permission check with Winter's `hasAnyAccess` semantics for JWT sessions: superusers pass, a principal needs any one of the listed codes, and wildcards match on both sides (a grant ending in `.*` covers every code with that prefix, and a required code ending in `.*` is met by any grant under it). When the principal carries a non-full `bouncer.ScopeLimit` (a personal API token), the scope is checked first: one method-allowed scope must match every required code and the owner must hold a required code inside that scope; a superuser with a scoped token is limited to the scope, and an empty requirement list is refused unless the scope is full. @@ -308,7 +308,7 @@ func (p *Plugin) AdminFS() fs.FS { return adminFS } | `cabana.FieldPreset` | A text or mltext field's `preset` in the form schema: the source field and the type, `slug` or `exact`. | | `cabana.FormField.Separator` / `cabana.FormField.CustomTags` | A `type: taglist` field's string-mode `separator` (`comma` or `space`, empty in array mode) and its `customTags` as `fields.yaml` declares it (nil means the default, true); both omitted from the JSON when unset. | | `cabana.WriteData` / `cabana.WriteError` / `cabana.WriteErrorDetails` | Write the admin success and error envelopes. | -| `cabana.ValidationError` / `cabana.ListValidationError` | Field-level `validation_failed` errors. A `pact.AdminAction` may return a `cabana.ValidationError` to answer 422. | +| `cabana.ValidationError` / `cabana.ListValidationError` | Field-level `validation_failed` errors. A hook or a `pact.AdminAction` may return a `cabana.ValidationError` to answer 422; its `Details` messages may be translation keys, translated in the request locale. | | `cabana.ForbiddenError` | A write controller code refuses: a hook or an action returns it and the route answers 403 `forbidden` with its localized `Message` and `Details`; the transaction is rolled back. | | `cabana.AdminActionRequest` | Body of an action route: optional `record_id`, the widget's `values` and an optional `payload` (any JSON value, at most 64 KiB, widget route only). Unknown keys are refused. | | `cabana.AdminActionResult` | Answer of an action route: the localized `message`, the filtered `fill` object and, when the action returned one, `data` exactly as returned. | @@ -388,3 +388,5 @@ The Phase 18 pieces (money field and column, write-only settings secrets, riched The Phase 18.1 pieces (the taglist field and the flat reorder) are covered by Docker-free unit tests in `taglist_field_test.go` (TestTaglistFieldCompile, TestTaglistFieldValidate for the save check in both modes, TestTaglistFieldHydrate for the record GET) and `reorder_flat_test.go` (TestReorderFlatCompile, which also checks the localized list schema, and TestReorderFlatNodes for the save's node check), and by Postgres tests in `catalog_smoke_test.go` (TestTaglistFieldSmoke, TestReorderFlatSmoke with a partitioned subtest) and `reorder_test.go` (TestReorderPostgres for the tree reorder). The admin specs are `admin/tests/form/TaglistField.test.ts` and `admin/tests/views/ReorderView.test.ts`. `scripts/check-phase18.1.sh` runs them by name. A Detached bulk action (`pact.AdminBulkAction.Detached`) is covered by the Postgres test TestDetachedBulkAction in `bulk_detached_test.go`, on the roster fixture with two test-only actions: Run sees no transaction (`TxFromContext`) and holds no row lock (another connection's `SELECT ... FOR UPDATE NOWAIT` succeeds during Run); the empty, absent and partial-selection rules match the locked path; a `ValidationError` from Run is 422 and the rows written before it stay written; the same action without Detached still runs in the transaction holding the row locks. `scripts/check-phase18.1.sh` requires it in its postgres stage. + +The localization of refusals is covered by Docker-free unit tests in `localize_error_test.go` (TestLocalizeControllerErrorValidation, TestLocalizeControllerErrorForbidden and TestWriteCRUDErrorLocalized, on a two-locale catalog; the plugin's error value stays unchanged and text without `::` is never looked up) and by the Postgres tests TestValidationErrorLocalized (a form hook's `cabana.ValidationError` in en and pl) and TestReorderHookRefusalLocalized (both refusal types from `pact.ReorderAfterSave`). diff --git a/modules/cabana/actions.go b/modules/cabana/actions.go index aa715c04..326fe1ac 100644 --- a/modules/cabana/actions.go +++ b/modules/cabana/actions.go @@ -269,7 +269,8 @@ func (s *service) allowAction(w http.ResponseWriter, r *http.Request, permission } // runAction calls the plugin's Run and writes the D-10 envelope. A -// *ValidationError is a 422 and a *ForbiddenError a 403; any other error is +// *ValidationError is a 422 and a *ForbiddenError a 403, both with their +// messages localized in the request locale; any other error is // logged and answered with the generic 500 body, never the error text. The // result's Fill passes the fill filter; its Data bypasses that filter but not // the size cap: it is encoded once and embedded as-is, and a value above @@ -279,15 +280,12 @@ func (s *service) runAction(w http.ResponseWriter, r *http.Request, cc *Compiled ctx := towel.WithLocale(r.Context(), schemaLocale(r.Context(), tr)) result, err := action.Run(ctx, input) if err != nil { + // A validation failure is a 422 and a refusal (D-27) a 403, both + // with the action's messages localized. var invalid *ValidationError - if errors.As(err, &invalid) { - writeCRUDError(w, err) - return - } - // A refusal (D-27) is a 403 with the action's localized message. var refused *ForbiddenError - if errors.As(err, &refused) { - writeCRUDError(w, localizeForbidden(ctx, tr, err)) + if errors.As(err, &invalid) || errors.As(err, &refused) { + writeCRUDError(w, localizeControllerError(ctx, tr, err)) return } slog.Error("cabana: admin action failed", "controller", controllerID(cc), "action", action.Name, "field", input.Field, "error", err) diff --git a/modules/cabana/crud.go b/modules/cabana/crud.go index 1d179f18..d949ffef 100644 --- a/modules/cabana/crud.go +++ b/modules/cabana/crud.go @@ -61,7 +61,12 @@ type BulkActionResult struct { Affected int `json:"affected"` } -// ValidationError is a D-10 validation_failed failure. +// ValidationError is a D-10 validation_failed failure, answered 422. Details +// maps a field name to its messages (a string or a list), each a phrase key +// or text. A key returned by controller code (a lifecycle hook, an action, a +// reorder hook, a singleton record) is translated in the request locale +// before the response is written, like a ForbiddenError; text is written as +// given. type ValidationError struct { Details map[string]any } @@ -83,55 +88,93 @@ type ForbiddenError struct { func (e *ForbiddenError) Error() string { return "forbidden" } -// localizeForbidden returns err with a *ForbiddenError's Message and Details -// strings translated in the request locale; any other error, and a nil one, -// is returned unchanged. The plugin's error value is never modified: it may -// be a shared variable. -func localizeForbidden(ctx context.Context, tr *phrasebook.Translator, err error) error { - var refused *ForbiddenError - if err == nil || !errors.As(err, &refused) || refused == nil { - return err +// localizeControllerError returns err with the messages of a plugin-authored +// refusal translated in the request locale: the Details of a +// *ValidationError, or the Message and Details of a *ForbiddenError. The two +// types are tested in writeCRUDError's order, so the error localized is the +// one written. Any other error, a nil one, and a ValidationError without +// Details are returned unchanged. The plugin's error value is never modified: +// it may be a shared package variable read by concurrent requests in other +// locales, so a new error, map and slices are built. +func localizeControllerError(ctx context.Context, tr *phrasebook.Translator, err error) error { + if err == nil { + return nil } if ctx == nil { ctx = context.Background() } + var invalid *ValidationError + if errors.As(err, &invalid) { + if invalid == nil || len(invalid.Details) == 0 { + return err + } + ctx = towel.WithLocale(ctx, schemaLocale(ctx, tr)) + return &ValidationError{Details: localizeDetails(ctx, tr, invalid.Details)} + } + var refused *ForbiddenError + if !errors.As(err, &refused) || refused == nil { + return err + } ctx = towel.WithLocale(ctx, schemaLocale(ctx, tr)) - out := &ForbiddenError{Message: translateKey(ctx, tr, refused.Message)} + out := &ForbiddenError{Message: localizeDetail(ctx, tr, refused.Message)} if len(refused.Details) > 0 { - out.Details = make(map[string]any, len(refused.Details)) - for field, value := range refused.Details { - switch messages := value.(type) { - case string: - out.Details[field] = []string{translateKey(ctx, tr, messages)} - case []string: - list := make([]string, len(messages)) - for i, text := range messages { - list[i] = translateKey(ctx, tr, text) - } - out.Details[field] = list - case []any: - list := make([]any, len(messages)) - for i, item := range messages { - if text, ok := item.(string); ok { - list[i] = translateKey(ctx, tr, text) - } else { - list[i] = item - } - } - out.Details[field] = list - default: - out.Details[field] = value + out.Details = localizeDetails(ctx, tr, refused.Details) + } + return out +} + +// localizeDetails is the copy-on-write loop over a refusal's Details: a +// string becomes a one-message list, a []string or []any becomes a new slice +// with its strings localized, and any other value is passed through. The +// input map and its slices are never written to. +func localizeDetails(ctx context.Context, tr *phrasebook.Translator, details map[string]any) map[string]any { + out := make(map[string]any, len(details)) + for field, value := range details { + switch messages := value.(type) { + case string: + out[field] = []string{localizeDetail(ctx, tr, messages)} + case []string: + list := make([]string, len(messages)) + for i, text := range messages { + list[i] = localizeDetail(ctx, tr, text) } + out[field] = list + case []any: + list := make([]any, len(messages)) + for i, item := range messages { + if text, ok := item.(string); ok { + list[i] = localizeDetail(ctx, tr, text) + } else { + list[i] = item + } + } + out[field] = list + default: + out[field] = value } } return out } +// localizeDetail translates one refusal message when it is a phrase key and +// returns any other text unchanged. Every phrasebook key is +// vendor.plugin::group.path and the lookup is exact, so text without "::" +// can never resolve; skipping it keeps already-translated messages (lagoon +// rule messages, framework text) byte-identical and keeps the development +// log free of a missing-key warning per message. An empty message stays +// empty. +func localizeDetail(ctx context.Context, tr *phrasebook.Translator, text string) string { + if !strings.Contains(text, "::") { + return text + } + return translateKey(ctx, tr, text) +} + // transaction runs fn in lagoon.Transaction on the service's database and -// localizes a *ForbiddenError that comes out of it, so a refusal leaves the -// service ready to be written. +// localizes a *ValidationError or *ForbiddenError that comes out of it, so a +// refusal leaves the service ready to be written. func (s CRUDService) transaction(ctx context.Context, fn func(ctx context.Context, tx *gorm.DB) error) error { - return localizeForbidden(ctx, s.tr, lagoon.Transaction(ctx, s.DB, fn)) + return localizeControllerError(ctx, s.tr, lagoon.Transaction(ctx, s.DB, fn)) } // CapabilityError is a fail-closed Fill/Validate failure with controller context. @@ -369,7 +412,7 @@ func (s CRUDService) BulkAction(ctx context.Context, cc *CompiledController, nam ctx = towel.WithLocale(ctx, schemaLocale(ctx, s.tr)) if action.Detached { result, err := s.detachedBulkAction(ctx, cc, name, action, ids) - return result, localizeForbidden(ctx, s.tr, err) + return result, localizeControllerError(ctx, s.tr, err) } var result BulkActionResult err = s.transaction(ctx, func(ctx context.Context, tx *gorm.DB) error { @@ -456,7 +499,9 @@ func (s CRUDService) Show(ctx context.Context, cc *CompiledController, id any) ( return res.Data, nil } -// ShowRecord is Show plus the relation labels of the record (D-18). +// ShowRecord is Show plus the relation labels of the record (D-18). A +// ValidationError or ForbiddenError from a controller hook it runs is +// returned with its messages localized in the request locale. func (s CRUDService) ShowRecord(ctx context.Context, cc *CompiledController, id any) (RecordResult, error) { if s.DB == nil { return RecordResult{}, errors.New("cabana: database is not configured") @@ -493,7 +538,9 @@ func (s CRUDService) ShowRecord(ctx context.Context, cc *CompiledController, id return err }) if err != nil { - return RecordResult{}, err + // FormRecordTitle, a record action's Applies and the + // RelationLockProvider may refuse the read with phrase keys. + return RecordResult{}, localizeControllerError(ctx, s.tr, err) } return result, nil } diff --git a/modules/cabana/field_type.go b/modules/cabana/field_type.go index 4ca05b8f..b393c15b 100644 --- a/modules/cabana/field_type.go +++ b/modules/cabana/field_type.go @@ -772,12 +772,12 @@ func derefValue(v any) any { } // localizeNotices turns the notices of a save into the response's -// meta.notices in the request locale, the way localizeForbidden localizes a -// refusal: Message is a phrase key or text, resolved with Choice when Count -// is above zero and Get otherwise; without a translator the text is kept. At -// most maxFieldNotices are kept per field and locale and each message is cut -// to maxNoticeRunes. The result is nil when there is no notice, so the -// response carries no notices key. +// meta.notices in the request locale, the way localizeControllerError +// localizes a refusal: Message is a phrase key or text, resolved with Choice +// when Count is above zero and Get otherwise; without a translator the text +// is kept. At most maxFieldNotices are kept per field and locale and each +// message is cut to maxNoticeRunes. The result is nil when there is no +// notice, so the response carries no notices key. func localizeNotices(ctx context.Context, tr *phrasebook.Translator, notices []pluginNotice) []FieldNotice { if len(notices) == 0 { return nil diff --git a/modules/cabana/localize_error_test.go b/modules/cabana/localize_error_test.go new file mode 100644 index 00000000..e34b9d2a --- /dev/null +++ b/modules/cabana/localize_error_test.go @@ -0,0 +1,189 @@ +package cabana + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + "testing/fstest" + + "git.golem15.com/golem15/summercms/modules/phrasebook" + "git.golem15.com/golem15/summercms/modules/towel" +) + +const ( + menuURLScheme = "acme.menu::lang.errors.url_scheme" + menuTarget = "acme.menu::lang.errors.target" + menuMissing = "acme.menu::lang.errors.missing" +) + +var menuTexts = map[string][2]string{ + "en": {"The link must start with http or https.", "Choose where the link opens."}, + "pl": {"Link musi zaczynać się od http lub https.", "Wybierz, gdzie otwiera się link."}, +} + +// menuTranslator has the acme.menu error keys in en and pl. +func menuTranslator(t *testing.T) *phrasebook.Translator { + t.Helper() + cat := phrasebook.NewCatalog() + files := fstest.MapFS{} + for locale, texts := range menuTexts { + files["lang/"+locale+"/lang.yaml"] = &fstest.MapFile{Data: []byte(fmt.Sprintf("errors:\n url_scheme: %q\n target: %q\n", texts[0], texts[1]))} + } + if err := cat.Load("acme.menu", files); err != nil { + t.Fatal(err) + } + return phrasebook.NewTranslator(cat, phrasebook.Options{Locale: "en", Fallback: "en"}) +} + +// menuInvalid is a ValidationError with every Details shape a plugin uses. +func menuInvalid() *ValidationError { + return &ValidationError{Details: map[string]any{ + "url": []string{menuURLScheme}, + "target": menuTarget, + "mixed": []any{menuURLScheme, 7}, + "title": []string{"The title field is required."}, + "nested": map[string]any{"pl": []string{menuURLScheme}}, + "absent": []string{menuMissing}, + }} +} + +func TestLocalizeControllerErrorValidation(t *testing.T) { + tr := menuTranslator(t) + shared := menuInvalid() + before := menuInvalid() + for _, locale := range []string{"pl", "en", "pl"} { + ctx := towel.WithLocale(context.Background(), locale) + got := localizeControllerError(ctx, tr, shared) + var ve *ValidationError + if !errors.As(got, &ve) { + t.Fatalf("%s: got %T, want *ValidationError", locale, got) + } + if ve == shared { + t.Fatalf("%s: the plugin's value was returned, not a copy", locale) + } + want := map[string]any{ + "url": []string{menuTexts[locale][0]}, + "target": []string{menuTexts[locale][1]}, + "mixed": []any{menuTexts[locale][0], 7}, + "title": []string{"The title field is required."}, + "nested": map[string]any{"pl": []string{menuURLScheme}}, + "absent": []string{menuMissing}, + } + if !reflect.DeepEqual(ve.Details, want) { + t.Fatalf("%s: details = %#v, want %#v", locale, ve.Details, want) + } + // New slices: writing into the result does not reach the plugin's value. + ve.Details["url"].([]string)[0] = "changed" + ve.Details["title"].([]string)[0] = "changed" + ve.Details["mixed"].([]any)[0] = "changed" + if !reflect.DeepEqual(shared, before) { + t.Fatalf("%s: the plugin's value was modified: %#v", locale, shared.Details) + } + } +} + +func TestLocalizeControllerErrorUnchanged(t *testing.T) { + tr := menuTranslator(t) + ctx := towel.WithLocale(context.Background(), "pl") + if got := localizeControllerError(ctx, tr, nil); got != nil { + t.Fatalf("nil error = %v", got) + } + plain := errors.New("x") + if got := localizeControllerError(ctx, tr, plain); got != plain { + t.Fatalf("plain error = %#v, want the identical value", got) + } + empty := &ValidationError{} + if got := localizeControllerError(ctx, tr, empty); got != empty { + t.Fatalf("nil details = %#v, want the identical value", got) + } + noDetails := &ValidationError{Details: map[string]any{}} + if got := localizeControllerError(ctx, tr, noDetails); got != noDetails { + t.Fatalf("empty details = %#v, want the identical value", got) + } + var nilVE *ValidationError + if got := localizeControllerError(ctx, tr, nilVE); got != error(nilVE) { + t.Fatalf("nil *ValidationError = %#v", got) + } + // A wrapped ValidationError is found and localized. + got := localizeControllerError(ctx, tr, fmt.Errorf("save: %w", &ValidationError{Details: map[string]any{"url": []string{menuURLScheme}}})) + ve, ok := got.(*ValidationError) + if !ok || !reflect.DeepEqual(ve.Details, map[string]any{"url": []string{menuTexts["pl"][0]}}) { + t.Fatalf("wrapped = %#v", got) + } + // Without a translator the keys stay. + got = localizeControllerError(nil, nil, &ValidationError{Details: map[string]any{"url": menuURLScheme}}) + if ve, ok := got.(*ValidationError); !ok || !reflect.DeepEqual(ve.Details, map[string]any{"url": []string{menuURLScheme}}) { + t.Fatalf("nil translator = %#v", got) + } +} + +func TestLocalizeControllerErrorForbidden(t *testing.T) { + tr := menuTranslator(t) + shared := &ForbiddenError{Message: menuTarget, Details: map[string]any{"url": []string{menuURLScheme}, "plain": "Text."}} + for _, locale := range []string{"en", "pl"} { + got := localizeControllerError(towel.WithLocale(context.Background(), locale), tr, shared) + fe, ok := got.(*ForbiddenError) + if !ok || fe == shared { + t.Fatalf("%s: got %#v", locale, got) + } + want := &ForbiddenError{Message: menuTexts[locale][1], Details: map[string]any{"url": []string{menuTexts[locale][0]}, "plain": []string{"Text."}}} + if !reflect.DeepEqual(fe, want) { + t.Fatalf("%s: got %#v, want %#v", locale, fe, want) + } + } + if shared.Message != menuTarget || !reflect.DeepEqual(shared.Details, map[string]any{"url": []string{menuURLScheme}, "plain": "Text."}) { + t.Fatalf("the plugin's value was modified: %#v", shared) + } + // An empty Message stays empty and no Details stay nil. + got := localizeControllerError(context.Background(), tr, &ForbiddenError{}) + if fe, ok := got.(*ForbiddenError); !ok || fe.Message != "" || fe.Details != nil { + t.Fatalf("empty refusal = %#v", got) + } +} + +func TestWriteCRUDErrorLocalized(t *testing.T) { + tr := menuTranslator(t) + err := localizeControllerError(towel.WithLocale(context.Background(), "pl"), tr, menuInvalid()) + rec := httptest.NewRecorder() + writeCRUDError(rec, err) + var env ErrorEnvelope + if jerr := json.Unmarshal(rec.Body.Bytes(), &env); jerr != nil { + t.Fatal(jerr) + } + if rec.Code != http.StatusUnprocessableEntity || env.Error.Code != "validation_failed" { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + if !reflect.DeepEqual(env.Error.Details["url"], []any{menuTexts["pl"][0]}) { + t.Fatalf("details.url = %#v", env.Error.Details["url"]) + } +} + +// TestLocalizeControllerErrorPlainTextNotLookedUp: text without "::" is +// never looked up, so the development log has no missing-key warning for it; +// an unknown key does warn, which proves the capture works. Not parallel: it +// swaps the default logger. +func TestLocalizeControllerErrorPlainTextNotLookedUp(t *testing.T) { + var buf bytes.Buffer + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + tr := menuTranslator(t) + ctx := towel.WithLocale(context.Background(), "en") + localizeControllerError(ctx, tr, &ValidationError{Details: map[string]any{"title": []string{"The title field is required."}, "body": "Too long."}}) + if strings.Contains(buf.String(), "missing translation key") { + t.Fatalf("plain text was looked up: %s", buf.String()) + } + localizeControllerError(ctx, tr, &ValidationError{Details: map[string]any{"url": []string{menuMissing}}}) + if !strings.Contains(buf.String(), "missing translation key") { + t.Fatalf("an unknown key logged nothing; the capture does not work: %q", buf.String()) + } +} diff --git a/modules/cabana/phase121_fixture_test.go b/modules/cabana/phase121_fixture_test.go index 2cb14e24..3a9be820 100644 --- a/modules/cabana/phase121_fixture_test.go +++ b/modules/cabana/phase121_fixture_test.go @@ -561,6 +561,10 @@ const rosterLocked = "Locked" // copy and never write into it. var rosterRefused = &cabana.ForbiddenError{Message: "acme.roster::lang.people.locked"} +// rosterInvalid is a shared validation failure whose message is a phrase +// key: the framework must localize a copy and never write into it. +var rosterInvalid = &cabana.ValidationError{Details: map[string]any{"name": []string{"acme.roster::lang.people.refused_name"}}} + // FormVirtualFields lists the form fields that are not columns of the form: // the password pair and the create-only notify checkbox. func (rosterController) FormVirtualFields() []string { @@ -622,8 +626,9 @@ func (rosterController) FormBeforeDelete(_ context.Context, model any) error { } // FormBeforeUpdate stores a submitted password, refuses the reserved name -// with a ForbiddenError naming the field, and fails with a plain error for -// the name Boom. +// with a ForbiddenError naming the field, rejects the name Rejected with the +// shared rosterInvalid ValidationError, and fails with a plain error for the +// name Boom. func (c rosterController) FormBeforeUpdate(ctx context.Context, model any) error { storePassword(model.(*rosterPerson), c.spy.recordVirtual("before-update", ctx)) switch model.(*rosterPerson).Name { @@ -634,6 +639,8 @@ func (c rosterController) FormBeforeUpdate(ctx context.Context, model any) error } case "Silent": return &cabana.ForbiddenError{} + case "Rejected": + return rosterInvalid case "Boom": return fmt.Errorf("the roster database said hunter2") } diff --git a/modules/cabana/phase121_forbidden_test.go b/modules/cabana/phase121_forbidden_test.go index aac20d96..68b0a334 100644 --- a/modules/cabana/phase121_forbidden_test.go +++ b/modules/cabana/phase121_forbidden_test.go @@ -148,6 +148,27 @@ func TestForbiddenLocalized(t *testing.T) { } } +// TestValidationErrorLocalized: a hook's ValidationError whose message is a +// phrase key is a 422 with the message in the request locale, and the +// plugin's shared value keeps its key. +func TestValidationErrorLocalized(t *testing.T) { + env, gdb := newRosterEnv(t) + id := rosterInsert(t, gdb, rosterPerson{Tenant: "acme", Name: "Cy"}) + for locale, want := range map[string]string{ + "en": "This name is reserved.", + "pl": "Ta nazwa jest zastrzeżona.", + } { + rec := rosterLocale(env, http.MethodPut, rosterPath(id, ""), `{"name":"Rejected"}`, locale) + got := rosterError(t, rec) + if rec.Code != http.StatusUnprocessableEntity || got.Code != "validation_failed" || !reflect.DeepEqual(got.Details, map[string]any{"name": []any{want}}) { + t.Fatalf("%s: status=%d error=%+v", locale, rec.Code, got) + } + } + if !reflect.DeepEqual(rosterInvalid.Details, map[string]any{"name": []string{"acme.roster::lang.people.refused_name"}}) { + t.Fatalf("the plugin's error value was modified: %+v", rosterInvalid.Details) + } +} + // TestForbiddenRollsBack: nothing of a refused request stays, also when the // refusal comes after rows were written. func TestForbiddenRollsBack(t *testing.T) { diff --git a/modules/cabana/relation.go b/modules/cabana/relation.go index 8cb371d1..bb778e82 100644 --- a/modules/cabana/relation.go +++ b/modules/cabana/relation.go @@ -868,9 +868,10 @@ func relationSelects(db *gorm.DB, cr *CompiledRelation, target any, cols []Relat } // transaction runs fn in lagoon.Transaction on the service's database and -// localizes a *ForbiddenError a relation hook returned (D-27). +// localizes a *ValidationError or *ForbiddenError (D-27) a relation hook +// returned. func (s RelationService) transaction(ctx context.Context, fn func(ctx context.Context, tx *gorm.DB) error) error { - return localizeForbidden(ctx, s.tr, lagoon.Transaction(ctx, s.DB, fn)) + return localizeControllerError(ctx, s.tr, lagoon.Transaction(ctx, s.DB, fn)) } // Link links eligible related records to the parent and never restamps diff --git a/modules/cabana/relation_field.go b/modules/cabana/relation_field.go index facc1262..8d2e0499 100644 --- a/modules/cabana/relation_field.go +++ b/modules/cabana/relation_field.go @@ -388,7 +388,8 @@ func (s CRUDService) RelationOptions(ctx context.Context, cc *CompiledController } locked, _, err := relationLocks(ctx, cc, field) if err != nil { - return nil, ListMeta{}, err + // A refusal from the RelationLockProvider carries plugin phrase keys. + return nil, ListMeta{}, localizeControllerError(ctx, s.tr, err) } markLocked(rows, locked) last := 1 diff --git a/modules/cabana/reorder.go b/modules/cabana/reorder.go index ddcbc096..3b726488 100644 --- a/modules/cabana/reorder.go +++ b/modules/cabana/reorder.go @@ -297,7 +297,9 @@ func (s *service) reorderGet(w http.ResponseWriter, r *http.Request) { // every node, runs the controller's optional ReorderAfterSave hook (pact) with // the partition (its error rolls the reorder back) and answers the new tree. // The transaction is a lagoon.Transaction, so a hook can register work with -// lagoon.AfterCommit that runs only once the new order is committed. +// lagoon.AfterCommit that runs only once the new order is committed. A +// ValidationError (422) or ForbiddenError (403) from the hook is answered with +// its messages localized in the request locale. func (s *service) reorderPost(w http.ResponseWriter, r *http.Request) { s.protect(w, r, func(cc *CompiledController) { if cc.reorder == nil || cc.singleton { @@ -359,6 +361,8 @@ func (s *service) reorderPost(w http.ResponseWriter, r *http.Request) { tree, err = s.reorderTree(txCtx, tx, cc, in.Partition, false) return err }) + // A refusal from ReorderAfterSave carries plugin phrase keys. + err = localizeControllerError(ctx, s.translator(), err) if err != nil { s.writeReorderError(w, r, cc, err) return diff --git a/modules/cabana/reorder_hook_test.go b/modules/cabana/reorder_hook_test.go index 93154097..97a710a2 100644 --- a/modules/cabana/reorder_hook_test.go +++ b/modules/cabana/reorder_hook_test.go @@ -9,6 +9,7 @@ import ( "strings" "sync" "testing" + "testing/fstest" "time" "git.golem15.com/golem15/summercms/modules/backpack" @@ -35,6 +36,12 @@ func (hookShelfPlugin) Requires() []string { return nil } func (hookShelfPlugin) Register(*backpack.App) error { return nil } func (hookShelfPlugin) Boot(*backpack.App) error { return nil } func (hookShelfPlugin) AdminFS() fs.FS { return shelfFS } + +// LangFS serves the acme.shelf refusal messages of +// TestReorderHookRefusalLocalized. +func (hookShelfPlugin) LangFS() fs.FS { + return fstest.MapFS{"lang/en/lang.yaml": {Data: []byte("errors:\n order: \"This order is not allowed.\"\n locked: \"This shelf is locked.\"\n")}} +} func (p hookShelfPlugin) AdminControllers() []pact.AdminController { return []pact.AdminController{hookedPins{hook: p.hook}} } @@ -217,6 +224,35 @@ func TestReorderHookErrorRollsBack(t *testing.T) { } } +// TestReorderHookRefusalLocalized: a ValidationError or ForbiddenError from +// ReorderAfterSave whose messages are phrase keys is answered with the +// sentences in the request locale, never the keys, and the reorder is rolled +// back. +func TestReorderHookRefusalLocalized(t *testing.T) { + for _, tc := range []struct { + name string + err error + status int + want string + key string + }{ + {"validation error", &cabana.ValidationError{Details: map[string]any{"nodes": []string{"acme.shelf::lang.errors.order"}}}, http.StatusUnprocessableEntity, `"nodes":["This order is not allowed."]`, "acme.shelf::lang.errors.order"}, + {"forbidden", &cabana.ForbiddenError{Message: "acme.shelf::lang.errors.locked"}, http.StatusForbidden, `"message":"This shelf is locked."`, "acme.shelf::lang.errors.locked"}, + } { + t.Run(tc.name, func(t *testing.T) { + env := newShelfHookEnv(t, hookShelfPlugin{hook: func(context.Context, *uint64) error { return tc.err }}) + before := sorts(t, env.db) + code, body := env.swap(t) + if code != tc.status || !strings.Contains(body, tc.want) || strings.Contains(body, tc.key) { + t.Fatalf("status=%d want %d body=%s", code, tc.status, body) + } + if after := sorts(t, env.db); fmt.Sprint(after) != fmt.Sprint(before) { + t.Fatalf("sort values changed: before %v after %v", before, after) + } + }) + } +} + func TestReorderWithoutHookUnchanged(t *testing.T) { plain := newShelfHookEnv(t, shelfPlugin{}) code, plainBody := plain.swap(t) diff --git a/modules/cabana/singleton.go b/modules/cabana/singleton.go index c6919217..89f1dad6 100644 --- a/modules/cabana/singleton.go +++ b/modules/cabana/singleton.go @@ -103,7 +103,8 @@ func (s *service) singletonRoute(w http.ResponseWriter, r *http.Request) { if !errors.As(err, &missing) { slog.Error("cabana: singleton record failed", "controller", controllerID(cc), "error", err) } - writeCRUDError(w, err) + // A refusal from SingletonRecord carries plugin phrase keys. + writeCRUDError(w, localizeControllerError(ctx, s.translator(), err)) return } WriteData(w, http.StatusOK, result, nil)