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).
This commit is contained in:
2026-10-27 15:20:33 +01:00
parent 2b7174d406
commit 676ca539aa
4 changed files with 91 additions and 4 deletions

View File

@@ -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.

View File

@@ -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.

View File

@@ -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{}

View File

@@ -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())
}
}