mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-12 01:45:14 +02:00
schema-lint chassis v1 (WIP): tier surfacing + plan doc
First commit of the chassis v1 branch. Lands a small, foundational
slice without behavior change, plus a planning doc that lays out the
remaining 7 commits in sequence so the PR can be reviewed
incrementally.
This commit:
- Adds SchemaMigrationStep::diagnostic() returning the full
&'static DiagnosticCode (family + tier + severity) for
UnsupportedChange steps with codes. Renderers can now reach the
tier without re-implementing the code → tier lookup.
- CLI `omnigraph schema plan` output now displays tier alongside
code:
unsupported change on node:Person.age [OG-DS-104, destructive]:
removing property 'Person.age' is not supported in schema
migration v1
Operators see at-a-glance the kind of risk each rejection
represents — not just the rule identifier.
- No behavior change. All 11 existing schema_apply tests still pass.
Planning doc at docs/schema-lint-v1-plan.md tracks the 7 remaining
commits to bring v1 to feature-complete:
1. (this commit) Tier surfacing in plan output.
2. Soft / Hard mode enum on drop steps.
3. Tombstone fields on catalog IR.
4. Planner emits DropProperty { Soft } by default.
5. Apply path implements Soft mode.
6. Convert PR #62 destructive-rejection tests.
7. --allow-data-loss flag + Hard mode.
8. (optional) Tombstone unhide / restore command.
Delete the planning doc when v1 lands. Intentionally checked in to
the WIP branch so the scope is reviewable; not intended as a
permanent doc.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
6bad829ed0
commit
617a77d7c8
3 changed files with 122 additions and 7 deletions
|
|
@ -1036,13 +1036,24 @@ fn render_schema_plan_step(step: &SchemaMigrationStep) -> String {
|
|||
render_annotations(annotations)
|
||||
),
|
||||
SchemaMigrationStep::UnsupportedChange {
|
||||
entity,
|
||||
reason,
|
||||
code,
|
||||
} => match code {
|
||||
Some(c) => format!("unsupported change on {} [{}]: {}", entity, c, reason),
|
||||
None => format!("unsupported change on {}: {}", entity, reason),
|
||||
},
|
||||
entity, reason, ..
|
||||
} => {
|
||||
// When a schema-lint code is attached, render code + tier
|
||||
// so operators see at-a-glance the kind of risk (destructive
|
||||
// / validated / safe) — not just the rule identifier.
|
||||
// Reach the diagnostic via the `diagnostic()` helper so the
|
||||
// CLI doesn't need to know how the lookup works.
|
||||
match step.diagnostic() {
|
||||
Some(diag) => format!(
|
||||
"unsupported change on {} [{}, {}]: {}",
|
||||
entity,
|
||||
diag.code,
|
||||
schema_lint_tier_label(diag.tier),
|
||||
reason,
|
||||
),
|
||||
None => format!("unsupported change on {}: {}", entity, reason),
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1054,6 +1065,14 @@ fn schema_type_kind_label(kind: omnigraph_compiler::SchemaTypeKind) -> &'static
|
|||
}
|
||||
}
|
||||
|
||||
fn schema_lint_tier_label(tier: omnigraph_compiler::SafetyTier) -> &'static str {
|
||||
match tier {
|
||||
omnigraph_compiler::SafetyTier::Safe => "safe",
|
||||
omnigraph_compiler::SafetyTier::Validated => "validated",
|
||||
omnigraph_compiler::SafetyTier::Destructive => "destructive",
|
||||
}
|
||||
}
|
||||
|
||||
fn render_prop_type(prop_type: &omnigraph_compiler::PropType) -> String {
|
||||
let base = if let Some(values) = &prop_type.enum_values {
|
||||
format!("Enum({})", values.join("|"))
|
||||
|
|
|
|||
|
|
@ -93,6 +93,24 @@ impl SchemaMigrationStep {
|
|||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// If this step carries a schema-lint code, return the full
|
||||
/// catalog entry — including family, safety tier, and default
|
||||
/// severity. Used by renderers that want to display richer
|
||||
/// context than just the code string (e.g. `omnigraph schema
|
||||
/// plan` annotating each line with its tier).
|
||||
///
|
||||
/// Returns `None` for steps that carry no code (the 12 of 17
|
||||
/// `UnsupportedChange` paths still untagged in v0, plus every
|
||||
/// non-`UnsupportedChange` variant).
|
||||
pub fn diagnostic(&self) -> Option<&'static crate::lint::DiagnosticCode> {
|
||||
match self {
|
||||
Self::UnsupportedChange {
|
||||
code: Some(c), ..
|
||||
} => crate::lint::lookup(c),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub fn plan_schema_migration(
|
||||
|
|
|
|||
78
docs/schema-lint-v1-plan.md
Normal file
78
docs/schema-lint-v1-plan.md
Normal file
|
|
@ -0,0 +1,78 @@
|
|||
# Schema-lint chassis v1 — implementation plan
|
||||
|
||||
Work-in-progress checklist for the next slice of the chassis. v0 (the code-tagged diagnostics layer, MR-694 first PR #87) shipped on `main`. v1 brings the chassis to **enforced behavior**: `--allow-data-loss` flag, the `Soft | Hard` mode dimension on drops, and the first real "destructive but supported" migration step.
|
||||
|
||||
This document tracks scope so the PR can land in incremental commits without losing the thread. Delete after the work is merged.
|
||||
|
||||
## Done in this branch so far
|
||||
|
||||
- [x] `SchemaMigrationStep::diagnostic()` helper — returns the full `DiagnosticCode` (family + tier + severity) for `UnsupportedChange` steps with codes.
|
||||
- [x] CLI `omnigraph schema plan` output displays tier alongside the code: `unsupported change on node:Person.age [OG-DS-104, destructive]: ...`.
|
||||
- [x] No behavior change. All 11 existing `schema_apply` tests still pass.
|
||||
|
||||
## Next commits (in order)
|
||||
|
||||
### Commit 2 — `Soft` / `Hard` mode enum on drop steps
|
||||
|
||||
- [ ] Add `DropMode { Soft, Hard }` enum in `schema_plan.rs`. Tier follows the mode: `Soft → safe` (catalog tombstone, no data touched), `Hard → destructive` (Lance-level removal).
|
||||
- [ ] Add new `SchemaMigrationStep` variants:
|
||||
- `DropProperty { type_kind, type_name, property_name, mode }`
|
||||
- `DropType { type_kind, name, mode }`
|
||||
- [ ] Existing `UnsupportedChange` emission paths for property/type removal keep emitting `UnsupportedChange` until the planner is wired in commit 3. New variants are dormant.
|
||||
- [ ] Unit test: the variants serialize/deserialize cleanly (SchemaIR round-trip).
|
||||
|
||||
### Commit 3 — Tombstone fields on catalog IR
|
||||
|
||||
- [ ] Add `tombstoned: bool` (default `false`, `#[serde(default)]` for backward compat) to `NodeIR`, `EdgeIR`, `PropertyIR`. Same on the `Catalog` types if needed.
|
||||
- [ ] Readers (`build_catalog`, `build_catalog_from_ir`) propagate the flag.
|
||||
- [ ] Manifest read path filters tombstoned items from the query-visible surface — they exist on disk, not in `Catalog`'s public iteration.
|
||||
- [ ] Unit tests: a tombstoned property is invisible to `node_type.properties.get(...)`; the underlying Arrow column survives the rewrite.
|
||||
|
||||
### Commit 4 — Planner emits `DropProperty { Soft }` by default
|
||||
|
||||
- [ ] In `plan_properties`'s leftover-property branch: emit `DropProperty { Soft }` instead of `UnsupportedChange` for OG-DS-104.
|
||||
- [ ] Same shape for node-type removal (`plan_nodes` leftover branch → `DropType { Soft }`, OG-DS-102) and edge-type removal (`plan_edges` leftover → `DropType { Soft }`, OG-DS-103).
|
||||
- [ ] CLI plan output: render the new variants with mode visible.
|
||||
|
||||
### Commit 5 — Apply path implements `Soft` mode
|
||||
|
||||
- [ ] `apply_schema_with_lock` handles `DropProperty { Soft }`: writes `tombstoned: true` into the catalog/IR, no data manipulation.
|
||||
- [ ] Same for `DropType { Soft }`.
|
||||
- [ ] Recovery sidecar discipline: register a `RecoveryKind::CatalogOnly` sidecar entry; soft drops are roll-forward-safe.
|
||||
- [ ] Integration test (extends `tests/schema_apply.rs`): remove a property, assert apply succeeds, row count preserved, schema query no longer surfaces the property.
|
||||
|
||||
### Commit 6 — Convert PR #62 destructive-rejection tests
|
||||
|
||||
- [ ] `tests/schema_apply.rs`'s 6 PR #62 tests currently assert "removing X fails with `OG-DS-XXX`". Convert each to:
|
||||
- **Without `--allow-data-loss`**: soft drop succeeds (catalog tombstone, rows preserved).
|
||||
- **With `--allow-data-loss`**: hard drop succeeds (rows deleted) — covered by commit 7.
|
||||
- [ ] Add new tests for the supported soft path (verify tombstone state).
|
||||
|
||||
### Commit 7 — `--allow-data-loss` CLI flag + `Hard` mode
|
||||
|
||||
- [ ] Add `--allow-data-loss` boolean flag to `omnigraph schema apply` and `omnigraph schema plan` (plan shows what would happen if applied with the flag).
|
||||
- [ ] Thread through to `apply_schema_with_lock(.., allow_data_loss: bool)`.
|
||||
- [ ] Planner: when `--allow-data-loss` is set, emit `Hard` mode instead of `Soft` for drop paths.
|
||||
- [ ] Apply path for `Hard` mode: `stage_overwrite` removing the column (DropProperty) or `Dataset::restore`-style table drop (DropType). Full recovery-sidecar discipline (`RecoveryKind::FullRewrite`).
|
||||
- [ ] Integration tests: hard drop deletes data; without flag, hard drop is impossible (planner only emits soft).
|
||||
|
||||
### Commit 8 — Tombstone unhide / restore command (optional)
|
||||
|
||||
- [ ] `omnigraph schema unhide <type-or-property>` reverses a soft drop within a grace period.
|
||||
- [ ] Tests: tombstone → unhide → property is queryable again with rows intact.
|
||||
- [ ] Defer if scope creeps; can ship in a follow-up PR.
|
||||
|
||||
## Open questions
|
||||
|
||||
- **Tombstone visibility in `omnigraph schema show`**: should the CLI list tombstoned items by default (with a flag indicator), or hide them by default? Recommend: hide by default; `--include-tombstoned` reveals.
|
||||
- **Query-level enforcement on tombstoned types**: should a `match { $p: TombstonedType }` query fail at parse time, lint time, or runtime? Recommend: lint warning at parse time (new code in the QL family); runtime falls back to empty result.
|
||||
- **Hard-drop semantics for `DropType`**: drop the Lance dataset entirely vs. retain on disk for forensics. Recommend: dataset deletion via Lance API (the `--allow-data-loss` flag justifies it; operators can take a snapshot first via `branch_create` if they want forensics).
|
||||
|
||||
## Not in v1 scope (deferred)
|
||||
|
||||
- Severity config in `omnigraph.yaml` (per-rule `error` / `warn` / `force`).
|
||||
- `@allow(OG-XXX-NNN, "rationale")` suppression directives.
|
||||
- Pre-migration checks (MR-941).
|
||||
- CD / VE / LK / NM family rules (MR-942..MR-945).
|
||||
- CI integration (MR-946).
|
||||
- The remaining 12 of 17 `UnsupportedChange` paths still untagged with codes (interface removal, edge endpoint change, edge cardinality change, etc.). Each goes through its own MR-XXX issue.
|
||||
Loading…
Add table
Add a link
Reference in a new issue