From 676ca539aa2a18237defea98c0a3ad2163d6c162 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Tue, 27 Oct 2026 15:20:33 +0100 Subject: [PATCH] fix(32): CR-01 check required extension fields on the lifted value A required: true field in a form extension became a rule on the owner's model, where its value never exists, so every save of the host form failed, also for administrators who cannot see the extension. mergedRules now skips extension fields; liftExtensionValues checks required on the submitted value for visible extensions only (create: must be sent and not blank; update: only when sent). --- docs/backend/admin-controllers.md | 1 + modules/cabana/crud.go | 6 ++ modules/cabana/form_extensions.go | 23 +++++-- .../cabana/form_extensions_postgres_test.go | 65 +++++++++++++++++++ 4 files changed, 91 insertions(+), 4 deletions(-) diff --git a/docs/backend/admin-controllers.md b/docs/backend/admin-controllers.md index 7a4cfd28..7d3e999c 100644 --- a/docs/backend/admin-controllers.md +++ b/docs/backend/admin-controllers.md @@ -609,6 +609,7 @@ Every field and tab of a compiled form has a stable name: the field key, and the An extension adds value fields (`text`, `textarea`, `number`, `checkbox`, `switch`, `dropdown` with listed options, `markdown`, `mltext`, `mltextarea`, `mlmarkdown` and plugin field types), `widget` fields and `partial` fields. Types that bind to the owner's model are refused at boot, `fileupload` included: an extension owns no attachments, so a plugin field type with `Upload` stores files instead. - On save, the extension's keys leave the request body before anything else reads it, so they never reach `Fill`, the owner's columns, its translation rows or its attachments. After the owner row is written (and its translations and after hooks have run), `Save` runs inside the same transaction with `pact.AdminFormSave`: the `Record` with its `ID`, `Created`, `Values`, the submitted values of the extension's fields whose `context` allows the operation, and `Changes`, what the owner's save changed (old and new values, a translated value per locale), as the save's `pact.RecordSaved` will report it after the commit. `Save` runs on every create and update, with or without values. +- `required: true` on an extension field is checked on the submitted value, not as a rule on the owner's model, and only for administrators who see the extension: a create must send a value that is not null or blank (for a translatable field, the default locale's text), an update checks the value only when it is sent. - A translatable field (`mltext`, `mltextarea`, `mlmarkdown`, a translatable plugin field type) reaches `Save` as a `map[string]string` of locale to text, and `Load` returns it the same way. A plugin field type's `Decode` runs before `Save` (its notices are kept, a `pact.FormFieldError` is a 422 on the field) and its `Load` after the extension's `Load`. - A `Save` error rolls the whole save back, the owner's row included. `pact.AdminFormErrors` (field name to messages, phrase keys or text) answers 422 with those lines on the fields; any other error is logged and answered with the generic 500 body. `pact.AdminFormSaveResult.Notices` are save notices per field, shown on the field like a field type's notices; a notice may name an owner field. - On show and in the save response, `Load` returns the values by field name; keys that are not the extension's fields are dropped. diff --git a/modules/cabana/crud.go b/modules/cabana/crud.go index 593f1ab1..2dd940a6 100644 --- a/modules/cabana/crud.go +++ b/modules/cabana/crud.go @@ -1351,6 +1351,12 @@ func mergedRules(ctx context.Context, cc *CompiledController, model any, op stri return out } for _, field := range cc.Form.Fields { + // D-91: a form extension's field is not a column of this model; its + // required is checked on the lifted value (liftExtensionValues), and + // only for administrators who see the extension. + if cc.extensionOf(field.Name) != nil { + continue + } // Relation fields are not writable columns. required stays on the // schema for the client, but it cannot be checked by Fill. A virtual // field is checked against its submitted value. diff --git a/modules/cabana/form_extensions.go b/modules/cabana/form_extensions.go index fe3964ca..7ea5677b 100644 --- a/modules/cabana/form_extensions.go +++ b/modules/cabana/form_extensions.go @@ -1161,7 +1161,8 @@ type extensionLift struct { // extension keeps the values of its value fields whose context allows the // operation: a translatable one as a map of locale to text (a string is the // default locale's text), any other built-in type as a single JSON value, a -// plugin field type as sent. +// plugin field type as sent. A required field of a visible extension is +// checked here, never against the owner's model (mergedRules skips it). func liftExtensionValues(ctx context.Context, in extensionLift, body map[string]any) (extensionValues, error) { out := extensionValues{} cc := in.CC @@ -1176,14 +1177,21 @@ func liftExtensionValues(ctx context.Context, in extensionLift, body map[string] for _, ext := range cc.formExtensions { for _, name := range ext.fields { value, present := body[name] - if !present { - continue - } delete(body, name) if !in.Visible[ext] || !ext.valueField(name) || !contextAllows(cc, name, in.Op) { continue } field := ext.own[name] + // A required extension field must be sent on create; on update + // only a sent value is checked, so a partial save keeps the + // stored one. Blank is null or whitespace, for a translatable + // field the default locale's text. + if !present { + if field.Required && in.Op == "create" { + details[name] = []string{"The " + name + " field is required."} + } + continue + } if isMLField(field) { if !resolved { var err error @@ -1197,10 +1205,17 @@ func liftExtensionValues(ctx context.Context, in extensionLift, body map[string] details[name] = []string{message} continue } + if def, sent := texts[locales.def]; field.Required && ((sent && blankValue(def)) || (!sent && in.Op == "create")) { + details[name] = []string{"The " + name + " field is required."} + continue + } value = texts } else if field.fieldType == nil && nestedValue(value) { details[name] = []string{"The " + name + " field must be a single value."} continue + } else if field.Required && blankValue(value) { + details[name] = []string{"The " + name + " field is required."} + continue } if out[ext] == nil { out[ext] = map[string]any{} diff --git a/modules/cabana/form_extensions_postgres_test.go b/modules/cabana/form_extensions_postgres_test.go index fc90a74e..0285f8e9 100644 --- a/modules/cabana/form_extensions_postgres_test.go +++ b/modules/cabana/form_extensions_postgres_test.go @@ -533,3 +533,68 @@ func TestFormExtensionSave(t *testing.T) { } }) } + +// TestFormExtensionRequired (CR-01): a required extension field is checked +// on the lifted value, never as a rule on the owner's model. An administrator +// who sees the extension must send it on create and may not blank it; one who +// does not see it saves the host form freely. +func TestFormExtensionRequired(t *testing.T) { + store := &feStore{} + extra := feContributor{ + id: "acme.extra", + fsys: feExtraFS("fields:\n c:\n type: text\n required: true\n after: a\n"), + extensions: []pact.AdminActionExtension{{ + Controller: "acme.blog.posts", + Form: &pact.AdminFormExtension{Fields: "extensions/posts.yaml", Permissions: []string{"acme.extra.manage"}, Load: store.load, Save: store.save}, + }}, + } + env := newFEEnv(t, feOwner{fields: feOwnerFields}, extra) + editor := &bouncer.Principal{ID: 2, Backend: true, PermissionGrants: map[string]bool{"acme.blog.access": true}} + create := func(principal *bouncer.Principal, body map[string]any) *httptest.ResponseRecorder { + t.Helper() + return feCall(t, env.svc, principal, "acme.blog.posts", http.MethodPost, "record", "", body) + } + + for name, body := range map[string]map[string]any{ + "missing": {"a": "A"}, + "blank": {"a": "A", "c": " "}, + "null": {"a": "A", "c": nil}, + } { + rec := create(superUser(), body) + if rec.Code != http.StatusUnprocessableEntity || !strings.Contains(rec.Body.String(), "The c field is required.") { + t.Fatalf("%s: status=%d body=%s", name, rec.Code, rec.Body.String()) + } + } + rec := create(superUser(), map[string]any{"a": "A", "c": "C"}) + if rec.Code != http.StatusCreated { + t.Fatalf("create with c: status=%d body=%s", rec.Code, rec.Body.String()) + } + created := pgFTDecode(t, rec) + idText := "" + switch v := created.Data["id"].(type) { + case float64: + idText = strconv.FormatFloat(v, 'f', -1, 64) + case string: + idText = v + } + if idText == "" { + t.Fatalf("create data=%v", created.Data) + } + + // An update without c keeps the stored value; a blank c is refused. + if rec := feCall(t, env.svc, superUser(), "acme.blog.posts", http.MethodPut, "record", idText, map[string]any{"a": "A2"}); rec.Code != http.StatusOK { + t.Fatalf("partial update: status=%d body=%s", rec.Code, rec.Body.String()) + } + if rec := feCall(t, env.svc, superUser(), "acme.blog.posts", http.MethodPut, "record", idText, map[string]any{"a": "A3", "c": ""}); rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("blank update: status=%d body=%s", rec.Code, rec.Body.String()) + } + + // The extension is hidden from editor: required does not apply to them. + rec = create(editor, map[string]any{"a": "E"}) + if rec.Code != http.StatusCreated { + t.Fatalf("editor create: status=%d body=%s", rec.Code, rec.Body.String()) + } + if rec := feCall(t, env.svc, editor, "acme.blog.posts", http.MethodPut, "record", idText, map[string]any{"a": "E2", "c": ""}); rec.Code != http.StatusOK { + t.Fatalf("editor update: status=%d body=%s", rec.Code, rec.Body.String()) + } +}