diff --git a/CHANGELOG.md b/CHANGELOG.md index 81f629e..d2a5b11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,17 @@ All notable changes to Jarl are documented here. The format follows ## [Unreleased] +### Fixed + +- A changeset with a staged record delete could not be applied after that + record was deleted elsewhere (e.g. with the CLI), and it was hard to see + which operation to remove: the error only said "record 832 does not + exist". A staged delete of a record that is already gone now counts as + done (with a warning), and the other operations that no longer apply + (an edit or disable of a deleted record, a change to an object changed + since) are named with their number and summary. The Changes page marks + that operation and offers to remove it right in the error. + ### Added - Prometheus metrics at `/metrics` ([docs/metrics.md](docs/metrics.md)), diff --git a/docs/api.md b/docs/api.md index 8571c5d..509290f 100644 --- a/docs/api.md +++ b/docs/api.md @@ -149,6 +149,14 @@ honours `If-Match: `, and batch operations take `base_version`. If the object changed in the meantime the request fails with 409. Staged operations record the version they were staged against automatically. +**Stale changesets.** When an object a staged operation needs was changed +or deleted after it was staged (from the CLI, say), preview and apply fail +on that operation: the error has `op` (its number in the changeset), +`op_summary` and a message saying what happened; remove the operation +(`DELETE /changesets/{id}/ops/{seq}`) or stage it again. A staged delete of +a record that is already gone is not an error: it counts as done, with a +warning. + ## Resources `{zone}` is a zone ID or name (`root` for the root zone). Add `?view=NAME` diff --git a/internal/engine/errors.go b/internal/engine/errors.go index 7cf395e..3a16fbe 100644 --- a/internal/engine/errors.go +++ b/internal/engine/errors.go @@ -25,15 +25,21 @@ const ( // Error is an engine error with details for the caller. type Error struct { - Kind ErrorKind `json:"code"` - Message string `json:"message"` - OpSeq int `json:"op,omitempty"` // the operation that failed - Problems []validate.Problem `json:"problems,omitempty"` - Check []CheckMessage `json:"check,omitempty"` - Diff string `json:"diff,omitempty"` + Kind ErrorKind `json:"code"` + Message string `json:"message"` + OpSeq int `json:"op,omitempty"` // the operation that failed + // OpSummary describes that operation, so people can find it in the + // changeset (record IDs mean little to them). + OpSummary string `json:"op_summary,omitempty"` + Problems []validate.Problem `json:"problems,omitempty"` + Check []CheckMessage `json:"check,omitempty"` + Diff string `json:"diff,omitempty"` } func (e *Error) Error() string { + if e.OpSeq > 0 && e.OpSummary != "" { + return fmt.Sprintf("operation %d (%s): %s", e.OpSeq, e.OpSummary, e.Message) + } if e.OpSeq > 0 { return fmt.Sprintf("operation %d: %s", e.OpSeq, e.Message) } diff --git a/internal/engine/ops.go b/internal/engine/ops.go index f4fe72c..442f64f 100644 --- a/internal/engine/ops.go +++ b/internal/engine/ops.go @@ -120,7 +120,7 @@ func applyOp(st *model.State, op model.Op, c *applyCtx) error { err = errf(KindBadRequest, "unknown operation %q", op.Kind) } if e, ok := AsError(err); ok && e.OpSeq == 0 { - e.OpSeq = op.Seq + e.OpSeq, e.OpSummary = op.Seq, op.Summary } return err } @@ -457,7 +457,7 @@ func applyRecordCreate(st *model.State, op model.Op, c *applyCtx) error { func applyRecordUpdate(st *model.State, op model.Op, c *applyCtx) error { r := st.Records[op.ID] if r == nil { - return errf(KindNotFound, "record %d does not exist", op.ID) + return errf(KindConflict, "the record (ID %d) no longer exists: it was deleted after this change was staged; remove this operation from the changeset", op.ID) } if err := checkVersion(op, r.Version, recordLabel(r)); err != nil { return err @@ -533,7 +533,10 @@ func applyRecordDelete(st *model.State, op model.Op, c *applyCtx) error { if op.ID != 0 { r := st.Records[op.ID] if r == nil { - return errf(KindNotFound, "record %d does not exist", op.ID) + // Deleted since this was staged (by someone else, or the + // CLI): what the operation wanted is done. + c.warn("%s: the record was already deleted outside this changeset, so there was nothing to delete", strings.TrimPrefix(op.Summary, "delete ")) + return nil } if err := checkVersion(op, r.Version, recordLabel(r)); err != nil { return err diff --git a/internal/engine/stale_test.go b/internal/engine/stale_test.go new file mode 100644 index 0000000..b269670 --- /dev/null +++ b/internal/engine/stale_test.go @@ -0,0 +1,51 @@ +package engine + +import ( + "strings" + "testing" + + "jarl/internal/model" +) + +// Records deleted or changed after a change to them was staged (in the +// CLI while a web UI changeset is open, say). +func TestStaleChangeset(t *testing.T) { + env := newEnv(t) + setupZone(env) + env.submit(env.admin, rec("example.com", "one", "A", "192.0.2.51"), rec("example.com", "two", "A", "192.0.2.52"), rec("example.com", "three", "A", "192.0.2.53")) + st := env.state() + one, two, three := findRec(st, "one.example.com.", "A"), findRec(st, "two.example.com.", "A"), findRec(st, "three.example.com.", "A") + + // Two deletes staged; one of the records is deleted elsewhere first. + cs, err := env.e.CreateChangeset(env.ctx, env.admin, "web", "") + if err != nil { + t.Fatal(err) + } + if _, err := env.e.Stage(env.ctx, cs.ID, []Request{NewRequest(OpRecordDelete, one.ID, nil), NewRequest(OpRecordDelete, two.ID, nil)}, env.admin); err != nil { + t.Fatal(err) + } + env.submit(env.admin, NewRequest(OpRecordDelete, one.ID, nil)) + res, err := env.e.Apply(env.ctx, cs.ID, env.admin, ApplyOptions{}) + if err != nil { + t.Fatalf("apply after the record was deleted elsewhere: %v", err) + } + if len(res.Warnings) != 1 || !strings.Contains(res.Warnings[0], "one.example.com. A 192.0.2.51: the record was already deleted") { + t.Errorf("warnings = %q", res.Warnings) + } + if st := env.state(); st.Records[two.ID] != nil { + t.Error("two was not deleted") + } + + // A staged edit of a record deleted elsewhere names the operation. + cs, _ = env.e.CreateChangeset(env.ctx, env.admin, "web", "") + env.e.Stage(env.ctx, cs.ID, []Request{NewRequest(OpRecordUpdate, three.ID, RecordInput{Data: model.Ptr("192.0.2.54")})}, env.admin) + env.submit(env.admin, NewRequest(OpRecordDelete, three.ID, nil)) + p, err := env.e.Preview(env.ctx, cs.ID, env.admin) + if err != nil || p.Error == nil || p.Error.OpSeq != 1 || !strings.HasPrefix(p.Error.OpSummary, "change three.example.com. A 192.0.2.53") { + t.Fatalf("preview = %+v, %v", p, err) + } + _, err = env.e.Apply(env.ctx, cs.ID, env.admin, ApplyOptions{}) + if ee, ok := AsError(err); !ok || ee.Kind != KindConflict || !strings.Contains(err.Error(), "operation 1 (change three.example.com. A") || !strings.Contains(err.Error(), "remove this operation") { + t.Errorf("apply = %v", err) + } +} diff --git a/internal/web/static/assets/pages/changes.js b/internal/web/static/assets/pages/changes.js index 71778d5..d586cdb 100644 --- a/internal/web/static/assets/pages/changes.js +++ b/internal/web/static/assets/pages/changes.js @@ -44,18 +44,33 @@ async function draftPanel(d) { const applyBtn = h("button", { class: "primary", disabled: true }, "Apply changes"); const forceBox = h("label", { class: "check small hidden" }, h("input", { type: "checkbox" }), "Overwrite files changed outside Jarl"); const ops = d.ops || []; + const removeOp = async (seq) => { + try { + const cs = await api("DELETE", "/changesets/" + d.id + "/ops/" + seq); + useDraft(cs); + rerender(); + } catch (e) { toast(e.message, "err", 8000); } + }; const opsTable = table([ { label: "#", render: (o) => String(o.seq) }, { label: "Change", render: (o) => o.summary || o.op, tdClass: "mono small" }, { label: "Added", render: (o) => time(o.added_at) }, - { label: "", render: (o) => h("button", { class: "small", title: "Remove this operation from the changeset", onclick: async () => { - try { - const cs = await api("DELETE", "/changesets/" + d.id + "/ops/" + o.seq); - useDraft(cs); - rerender(); - } catch (e) { toast(e.message, "err", 8000); } - } }, "Remove"), tdClass: "actions-cell" }, + { label: "", render: (o) => h("button", { class: "small", title: "Remove this operation from the changeset", onclick: () => removeOp(o.seq) }, "Remove"), tdClass: "actions-cell" }, ], ops); + // failedOp marks the operation that no longer applies and says how to + // get past it. + const failedOp = (err) => { + const i = ops.findIndex((o) => o.seq === err.op); + const rows = opsTable.querySelectorAll("tbody tr"); + if (i < 0 || !rows[i]) return errorBox({ message: err.message, details: err }); + rows[i].classList.add("op-failed"); + return h("div", { class: "msg err" }, + h("div", null, h("strong", null, "Operation " + err.op + " can't be applied: "), err.message), + h("div", { class: "mono small" }, ops[i].summary || ops[i].op), + h("div", { style: { marginTop: "8px" } }, + h("button", { class: "small", onclick: () => removeOp(err.op) }, "Remove operation " + err.op), + " ", h("span", { class: "muted small" }, "It is marked in the list above. Then check the rest again."))); + }; const runPreview = async () => { try { const p = await get("/changesets/" + d.id + "/preview"); @@ -64,7 +79,7 @@ async function draftPanel(d) { const drift = (p.drift || []); forceBox.classList.toggle("hidden", drift.length === 0); mount(preview, - p.error ? errorBox({ message: p.error.message, details: p.error }) : null, + p.error ? (p.error.op ? failedOp(p.error) : errorBox({ message: p.error.message, details: p.error })) : null, valid ? h("div", { class: "msg ok" }, "Valid. named-checkconf accepts the new configuration.", (p.serials || []).length ? " Serials: " + p.serials.map((s) => s.zone.replace(/\.$/, "") + " " + s.old + "→" + s.new).join(", ") : "") : null, !p.error && !valid ? h("div", { class: "msg err" }, "This changeset can't be applied yet:") : null, problemsList(p.problems), diff --git a/internal/web/static/assets/style.css b/internal/web/static/assets/style.css index 1fbe1d2..dd6b253 100644 --- a/internal/web/static/assets/style.css +++ b/internal/web/static/assets/style.css @@ -154,6 +154,8 @@ td.actions-cell { white-space: nowrap; text-align: right; width: 1%; } tr.editing td { background: var(--accent-soft); } tr.pending-add td { background: var(--add); } tr.pending-change td, tr.pending-disable td, tr.pending-enable td { background: var(--warn-soft); } +tr.op-failed td { background: var(--err-soft); } +tr.op-failed td:first-child { box-shadow: inset 3px 0 0 var(--err); } tr.pending-delete td { background: var(--del); text-decoration: line-through; color: var(--muted); } tr.disabled td:not(.actions-cell), tr.disabled td strong { color: var(--muted); } /* Disabled records: hatched, so they stand out from live and pending rows. */