From 4ab251fc467a5eac511e346fdc3544b6980296a9 Mon Sep 17 00:00:00 2001 From: debabsah Date: Sat, 3 Oct 2026 14:01:34 -0700 Subject: [PATCH] fix: restore by one rule for every error type, and never overwrite a backup - A chart update Superset rejected after the import left the dashboard half-updated, while a connection drop at the same step restored it. Both now restore the backup. - A non-Superset error (a bug) mid-build skipped the restore and surfaced as a traceback without the backup path. It now follows the same rule and returns the normal report. - Failures after the chart updates (linkage, scopes, queries) still leave the new version live, unchanged. - Backup names go to the microsecond and use exclusive create, so two applies in one second no longer overwrite the first backup. Tests: four offline cases (each fails on main), and three new live fault scenarios in tools/faultline.py (import-time bug, rejected and dropped in-place chart update) run on 4.1.4, 5.0.0 and 6.1.0 in CI. --- CHANGELOG.md | 9 +++ chartwright/apply.py | 37 +++++++++-- docs/VERIFICATION.md | 17 ++--- tests/test_apply_rollback.py | 119 +++++++++++++++++++++++++++++++++++ tools/faultline.py | 15 +++-- 5 files changed, 181 insertions(+), 16 deletions(-) create mode 100644 tests/test_apply_rollback.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b9422f7..f7a086d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,15 @@ The design brain, plus the fixes found reviewing it. stays registered whatever the sign-in method. Both now say what is true either way: Chartwright signs in with a Superset account's own password (database or LDAP) or Preset API tokens, never through SSO or OAuth. +- A failed build now puts the backup back by the same rule whatever the error. A chart + update that Superset rejected after the import left the dashboard half-updated, while a + connection drop at the same step restored it; both now restore. An error that wasn't a + Superset error (a bug in Chartwright) skipped the restore and lost the backup path; + it now follows the same rule and returns the normal report. Failures after the chart + updates (linkage, filter scopes, chart queries) still leave the new version live. +- Two applies to the same dashboard within one second wrote the same backup name, and + the second overwrote the first. Backup names now go to the microsecond + (`20261003T141502.123456.zip`) and are never overwritten. - When an automatic restore failed, the suggested `chartwright restore ` command left out the required `--profile`, so pasting it gave a usage error. It now includes it. - `docs/VERIFICATION.md` said CI runs on every push, and that the live check covers a diff --git a/chartwright/apply.py b/chartwright/apply.py index d61c3d7..b14f6d2 100644 --- a/chartwright/apply.py +++ b/chartwright/apply.py @@ -238,6 +238,26 @@ def backup_dir_for(profile: str, slug: str) -> "Path": return base / profile / slug +def write_backup(backup_dir: "Path", data: bytes) -> "Path": + """Write a backup zip under a new name; never overwrite an earlier one. + + Names are local time to the microsecond (``20261003T141502.123456.zip``), + fixed width, so they sort oldest to newest. Two applies within one second + used to share a name and the second overwrote the first. Exclusive create + makes a collision a retry, never a silent overwrite.""" + import datetime + + while True: + stamp = datetime.datetime.now().strftime("%Y%m%dT%H%M%S.%f") + path = backup_dir / f"{stamp}.zip" + try: + with open(path, "xb") as fh: + fh.write(data) + return path + except FileExistsError: + continue + + def restore_bundle(zip_bytes: bytes, slug: str, client: SupersetClient) -> ApplyReport: """Restore a backup bundle COMPLETELY, not just import it. The importer never overwrites existing charts (docs/CONTRACTS.md), so surviving @@ -321,7 +341,6 @@ def apply(spec: DashboardSpec, client: SupersetClient, profile: str = "default") if existing is not None: # Last-known-good insurance before we mutate anything: the previous # owned state, restorable with `chartwright restore --profile ...`. - import datetime import os backup_dir = backup_dir_for(profile, spec.dashboard.slug) @@ -330,11 +349,8 @@ def apply(spec: DashboardSpec, client: SupersetClient, profile: str = "default") # Zips hold dashboard/dataset metadata; gate the default tree to # the owner. (No-op on Windows; custom dirs are the user's to manage.) os.chmod(backup_dir.parent.parent, 0o700) - stamp = datetime.datetime.now().strftime("%Y%m%dT%H%M%S") - backup_path = backup_dir / f"{stamp}.zip" backup_bytes = client.export_dashboard(existing["id"]) - backup_path.write_bytes(backup_bytes) - report.backup = str(backup_path) + report.backup = str(write_backup(backup_dir, backup_bytes)) def _auto_restore(reason: str) -> None: """Import failed mid-mutation: put the previous state back rather @@ -399,7 +415,11 @@ def _auto_restore(reason: str) -> None: updated, update_errors = _update_owned_charts_in_place( spec, client, bundle, existing_by_uuid, resolution) if update_errors: + # Still the import step: some charts may carry the new params + # and others the old. Same outcome as a connection drop here, + # which already restored (SupersetAPIError while stage is import). report.import_detail = "; ".join(update_errors) + _auto_restore("updating charts in place failed after the import") return report if updated: report.warnings.append( @@ -441,6 +461,13 @@ def _auto_restore(reason: str) -> None: if report.stage in ("prepare", "import"): _auto_restore(f"apply failed during {report.stage}: {e}") return report + except Exception as e: # noqa: BLE001 - a bug mid-mutation must still restore and report + # Not a Superset error: most likely a bug here. Same rule as above, and + # the report keeps the backup path, which a traceback would lose. + report.import_detail = f"unexpected error during {report.stage}: {type(e).__name__}: {e}" + if report.stage in ("prepare", "import"): + _auto_restore(f"apply failed during {report.stage}: {type(e).__name__}: {e}") + return report report.stage = "done" report.ok = True diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index b2d1944..e0ac653 100644 --- a/docs/VERIFICATION.md +++ b/docs/VERIFICATION.md @@ -35,7 +35,7 @@ injection. | Layer | Proves | Where it runs | |---|---|---| -| Offline suite (343 tests, 35 modules) | Contract, determinism, round-trips, credentials | every PR and push to main, Linux + Windows, mcp 1 and 2 | +| Offline suite (347 tests, 36 modules) | Contract, determinism, round-trips, credentials | every PR and push to main, Linux + Windows, mcp 1 and 2 | | Chart-option contract | Every emitted chart option is declared by each version's plugin source | every PR and push to main | | Live guarantee check | 15-chart apply, per-chart data check, ids stable across re-apply | every PR and push to main, all 3 versions | | Lifecycle soak | 500 randomized edit cycles with invariants held | 500 cycles on 6.1.0 and 4.1.4 before release; 25 cycles per version on every PR and push to main | @@ -159,15 +159,18 @@ All four scenarios pass on 4.1.4, 5.0.0, and 6.1.0. ## 6. Fault injection (`tools/faultline.py`) A wrapping client trips a chosen method with the same typed error a real -network drop produces, or a fake HTTP 500 for the importer. A fault at -every stage boundary (the dataset round-trip and stale-chart deletion -during preparation, an importer 500 and a mid-import drop, the post-import -link verification, the data check) must produce a typed report naming the +network drop produces, a fake HTTP 500 for a rejected request, or a plain +`RuntimeError` standing in for a bug in the tool itself. A fault at every +stage boundary (the dataset round-trip and stale-chart deletion during +preparation; an importer 500, a mid-import drop and an import-time bug; +a rejected or dropped in-place chart update; the post-import link +verification; the data check) must produce a typed report naming the stage, never a traceback, and a recorded backup whenever mutation had begun. Then: -- for faults during preparation or import, the automatic restore fires, - and `plan` against the pre-apply spec comes back clean: the dashboard is +- for faults during preparation or import, in-place chart updates + included, the automatic restore fires whatever the error type, and + `plan` against the pre-apply spec comes back clean: the dashboard is back, not half-updated; - for faults after import, a clean re-apply converges. diff --git a/tests/test_apply_rollback.py b/tests/test_apply_rollback.py new file mode 100644 index 0000000..28bc496 --- /dev/null +++ b/tests/test_apply_rollback.py @@ -0,0 +1,119 @@ +"""When apply fails partway, the backup goes back on the same rule for every +error type: a failure in the prepare or import step (chart updates included) +restores it; a later failure leaves the new version live. Either way the +report carries the backup path. Live coverage of the same rule, on all three +Superset releases, is tools/faultline.py.""" + +import pytest + +import chartwright.apply as ap +from chartwright.resolver import Resolution +from chartwright.spec import load_spec + +SPEC = { + "spec_version": "1", + "dashboard": {"title": "Rollback", "slug": "rollback"}, + "charts": [{"name": "Orders", "type": "big_number_total", "metric": "COUNT(*)", + "dataset": {"database": "warehouse", "table": "orders"}}], + "layout": {"rows": [["Orders"]]}, +} + + +class StubClient: + """Just enough of SupersetClient for apply's flow, with an existing dashboard.""" + + base_url = "http://superset.test" + + def __init__(self, crash_on: str | None = None): + self.crash_on = crash_on + + def _maybe_crash(self, name): + if self.crash_on == name: + raise RuntimeError(f"bug in {name}") + + def find_dashboard_by_slug(self, slug): + return {"id": 7} + + def export_dashboard(self, did): + return b"backup-zip" + + def dashboard_charts(self, did): + self._maybe_crash("dashboard_charts") + return [{"slice_name": "Orders"}] + + def charts_by_uuids(self, mapping): + return {u: {"id": 11, "slice_name": n} for u, n in mapping.items()} + + def import_dashboard_bundle(self, bundle, overwrite=True): + self._maybe_crash("import_dashboard_bundle") + + class R: + status_code = 200 + text = "" + return R() + + +@pytest.fixture +def flow(monkeypatch, tmp_path): + """Isolate apply's orchestration: resolution, compile and restore stubbed.""" + monkeypatch.setenv("CHARTWRIGHT_BACKUP_DIR", str(tmp_path)) + monkeypatch.setattr(ap, "resolve", lambda spec, client: Resolution()) + monkeypatch.setattr(ap, "_ownership_guard", lambda spec, client: None) + monkeypatch.setattr(ap, "_roundtrip_dataset_files", lambda res, client: {}) + monkeypatch.setattr(ap, "compile_bundle", lambda spec, res, extra_files=None: b"bundle") + monkeypatch.setattr(ap, "_apply_filter_scopes", lambda spec, client, did: []) + monkeypatch.setattr(ap, "smoke", lambda spec, res, client: []) + restores = [] + + def fake_restore(data, slug, client): + restores.append(data) + return ap.ApplyReport(ok=True, stage="done") + + monkeypatch.setattr(ap, "restore_bundle", fake_restore) + return restores + + +def test_a_rejected_chart_update_restores_the_backup(flow, monkeypatch): + monkeypatch.setattr(ap, "_update_owned_charts_in_place", + lambda *a, **k: ([], ["chart 'Orders': update PUT HTTP 500 boom"])) + rep = ap.apply(load_spec(SPEC), StubClient(), "dev") + assert not rep.ok and rep.stage == "import" + assert flow == [b"backup-zip"] + assert any("AUTO-RESTORED" in w for w in rep.warnings) + assert rep.backup + + +def test_an_unexpected_error_during_import_restores_and_reports(flow, monkeypatch): + rep = ap.apply(load_spec(SPEC), StubClient(crash_on="import_dashboard_bundle"), "dev") + assert not rep.ok and rep.stage == "import" + assert "RuntimeError" in rep.import_detail + assert flow == [b"backup-zip"] + assert rep.backup + + +def test_an_unexpected_error_after_import_reports_without_restoring(flow, monkeypatch): + monkeypatch.setattr(ap, "_update_owned_charts_in_place", lambda *a, **k: (["Orders"], [])) + calls = {"n": 0} + client = StubClient() + real = client.dashboard_charts + + def second_call_crashes(did): + calls["n"] += 1 + if calls["n"] == 2: # call 1 is the prepare-stage stale check + raise RuntimeError("bug in linkage") + return real(did) + + client.dashboard_charts = second_call_crashes + rep = ap.apply(load_spec(SPEC), client, "dev") + assert not rep.ok and rep.stage == "linkage" + assert "RuntimeError" in rep.import_detail + assert flow == [] # later steps leave the new version live + assert rep.backup + + +def test_backups_written_in_the_same_second_never_overwrite(tmp_path): + a = ap.write_backup(tmp_path, b"first") + b = ap.write_backup(tmp_path, b"second") + assert a != b + assert a.read_bytes() == b"first" and b.read_bytes() == b"second" + assert sorted(p.name for p in tmp_path.iterdir()) == [a.name, b.name] diff --git a/tools/faultline.py b/tools/faultline.py index 513f926..d3c76bc 100644 --- a/tools/faultline.py +++ b/tools/faultline.py @@ -1,13 +1,15 @@ """Fault injection: every apply-stage boundary must fail SAFE. A FaultyClient delegates to the real client but trips an armed method with -either a typed SupersetAPIError (what a real network drop surfaces as after -client._send's conversion) or a fake HTTP 500 (the importer's failure shape). +a typed SupersetAPIError (what a real network drop surfaces as after +client._send's conversion), a fake HTTP 500 (a rejected request), or a +RuntimeError (a bug in the tool itself, not a Superset error). For each fault we assert the contract: - apply returns a TYPED report (never a traceback) naming the right stage; - report.backup is set whenever mutation had begun on an existing dashboard; - - auto-restore fires for prepare/import faults and the dashboard is BACK to - its pre-apply state (plan against the previous spec is clean); + - auto-restore fires for prepare/import faults (in-place chart updates + included), whatever the error type, and the dashboard is BACK to its + pre-apply state (plan against the previous spec is clean); - later-stage faults (linkage/scope/smoke) leave a recoverable dashboard: a clean re-apply converges (plan clean, ids stable). Also proves restore completeness: `restore_bundle` rolls back surviving @@ -68,6 +70,8 @@ def wrapper(*a, **kw): self.tripped = True if self._kind == "http500": return FakeResponse() + if self._kind == "crash": + raise RuntimeError(f"injected bug in {name}") raise SupersetAPIError(f"injected fault in {name}", None, None) return attr(*a, **kw) @@ -178,6 +182,9 @@ def main(argv: list[str] | None = None) -> int: ("delete_chart", "error", "prepare", True, 1), # stale-owned deletion (v2 drops a chart) ("import_dashboard_bundle", "http500", "import", True, 1), # importer 500 ("import_dashboard_bundle", "error", "import", True, 1), # network drop mid-import + ("import_dashboard_bundle", "crash", "import", True, 1), # a bug, not a Superset error + ("put_json", "http500", "import", True, 1), # in-place chart update rejected + ("put_json", "error", "import", True, 1), # network drop mid chart update ("dashboard_charts", "error", "linkage", False, 2), # call#1 is the prepare-stage stale check ("chart_data", "error", "smoke", False, 1), # smoke query ]