Repository navigation
feat(domain)!: reject declared properties under a restricted scope - #424
Open
Tomoscorbin wants to merge 4 commits into
Open
Tomoscorbin wants to merge 4 commits into
Tomoscorbin wants to merge 4 commits into
Conversation
"Unmanaged" meant different things depending on where it was sensed: the differ skipped unmanaged properties entirely, while validation compared every other unmanaged aspect and rejected its drift. The property exception lived in the differ's docstring, and generate depended on it at a distance. TableScope now answers a per-aspect ReconciliationMode — MANAGE, REQUIRE_MATCH, or IGNORE — derived from the existing minimum-scope ladder plus one fallback table that states what each unmanaged aspect does instead. The differ's rule becomes generic (an IGNORE aspect is not compared), UnmanagedAspectDrift becomes the literal REQUIRE_MATCH interpreter, and manages() survives as "mode is MANAGE" for the sites that only need the boolean. No behaviour changes. The DeltaTable scope docstring now states the property exception it previously omitted: a restricted scope carries declared properties without deploying or checking them.
The differ's property gate read
scope.reconciles(TableAspect.PROPERTIES) is ReconciliationMode.IGNORE
which asks the reader to know the whole mode vocabulary just to parse
a guard. TableScope now names the projection, next to manages():
ignores(aspect) — the scope carries the aspect without comparing it.
The differ reads "if the scope ignores properties, nothing to
compare", and the docstrings and architecture doc say "ignore" in
plain English instead of quoting the enum member.
reconciles() stays the single source; both predicates derive from it.
No behaviour changes.
…dicates reconciles() returned a mode, so every call site had to know the enum vocabulary to read a guard — unlike manages() and is_within(), where the verb is the meaning. Validation, the last mode consumer, really asks "must this aspect match?", so TableScope now answers all three questions by name: manages(), requires_match(), and ignores(). With no caller left for the raw mode, ReconciliationMode and the 11-row _UNMANAGED_MODE_BY_ASPECT table collapse into the predicates: ignores() is "properties, when unmanaged"; requires_match() is every other unmanaged aspect. A new aspect still fails loudly — every predicate goes through _MINIMUM_SCOPE_BY_ASPECT, which raises KeyError for an unmapped aspect — and require-match is the safe meaning for an aspect added without a decision, since drift is refused, never converged. No behaviour changes.
A declaration with scope="metadata" (or narrower) could carry properties that meant nothing: they were value-validated, then never deployed and never compared. The silence was documented in the metadata how-to, but nothing told the user their properties block was inert. DesiredTable now rejects declared properties when the scope does not manage them, next to the matching renamed_from rule, so the invariant also covers custom DesiredTableSource implementations. The error arrives at declaration time — import, in user modules — naming the keys to remove. The differ's ignore gate stays: the live table's properties (pipeline- owned, or written by a previous fully managed sync) still assert nothing under a restricted scope and are never read as drift. What changes is only the declared side: everything a declaration states now asserts something. BREAKING CHANGE: a restricted-scope declaration that also declares properties now fails at construction instead of passing silently. Remove the properties block, or widen the scope to "full".
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #423.
What
A declaration whose scope does not manage properties can no longer declare them.
DesiredTablerejects the combination at construction — forDeltaTableusers that means import time, with the keys named:The invariant sits in
DesiredTable.__post_init__next to the matchingrenamed_fromrule, so it also covers customDesiredTableSourceimplementations, not just the api path.Nothing changes on the observed side: a restricted scope still makes no property assertion, so pipeline-owned properties (or values written by a previous fully managed sync) are never compared and never read as drift. The differ's ignore gate stays.
Why
Follow-up to #423's review discussion.
DeltaTable(scope="metadata", properties={...})was accepted, value-validated, and then silently inert — neither deployed nor checked, documented only in the metadata how-to. That representable-but-meaningless state is why the scope model needed a "carried but ignored" story at all. Rejecting it at declaration time defines the state out of existence: everything a declaration states now asserts something.Breaking change: a restricted-scope declaration that also declares properties now fails loudly at import instead of passing silently. The fix is to delete the inert properties block (no behaviour change) or widen the scope to
"full"(if deployment was the actual intent).One test relied on the old state:
test_a_restricted_scope_mirrors_cdf_reserved_column_namescarried the CDF property under a restricted scope to show the reserved-column rule stays quiet. The reserved-name rule only fires when the declaration enables CDF, which a restricted scope now cannot do, so the test keeps pinning the mirrored_change_typecolumn without the property. The differ test for carried-but-ignored declared properties is deleted — that state is no longer representable — while its observed-side companion remains.Gate: 1403 passed, ruff check/format clean, mypy clean (178 files), all 7 import contracts kept.
🤖 Generated with Claude Code