fix(versioning): suppress automatic chart normalization changes - #43350
fix(versioning): suppress automatic chart normalization changes#43350mikebridge wants to merge 6 commits into
Conversation
dosubot
Bot
added
change:backend
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43350 +/- ##
==========================================
+ Coverage 66.73% 66.76% +0.03%
==========================================
Files 2876 2878 +2
Lines 164201 164598 +397
Branches 37887 37998 +111
==========================================
+ Hits 109577 109894 +317
- Misses 52467 52522 +55
- Partials 2157 2182 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation calculates normalization metadata using To resolve this, you should ensure the metadata is built from the same form-data representation used in the save payload. The PR already implements this by calculating I have reviewed the changes and they correctly address the identified API mismatch by ensuring consistency between the hydration normalization tracking and the save payload generation. superset-frontend/src/explore/actions/hydrateExplore.ts |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #04462c
Actionable Suggestions - 2
-
superset-frontend/src/explore/actions/saveModalActions.ts - 1
- Missing exclusions state update · Line 307-307
-
superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts - 1
- Duplicate dispatch for user edits · Line 172-172
Additional Suggestions - 2
-
superset/charts/schemas.py - 1
-
Missing test coverage for field · Line 363-372`normalization_changes` is added only to `ChartPutSchema` (PUT) and not `ChartPostSchema` (POST), consistent with the version-history advisory use case. However, `fields.Raw` accepts any JSON value without type enforcement — a malformed array will cause `api.py:677` to return a 400 on the entire PUT request rather than a targeted field error. No unit test verifies the field's acceptance behavior.
-
-
superset/commands/chart/update.py - 1
-
Missing docstring for new parameter · Line 69-78The new constructor parameter `normalization_changes: object = None` (line 73) has no type alias or docstring. Using `object` as the type is unusually broad; consider whether a specific `NormalizableChanges` type alias exists in `superset/versioning/changes/normalization.py` to improve type safety and discoverability.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/features/versionHistory/reducer.ts - 2
- Null access without guard check · Line 341-343
- Null access without guard check · Line 356-359
-
superset/commands/chart/update.py - 1
- Normalization metadata flow broken for MCP · Line 93-93
Review Details
-
Files reviewed - 17 · Commit Range:
871524c..871524c- superset-frontend/src/explore/actions/hydrateExplore.test.ts
- superset-frontend/src/explore/actions/hydrateExplore.ts
- superset-frontend/src/explore/actions/saveModalActions.test.ts
- superset-frontend/src/explore/actions/saveModalActions.ts
- superset-frontend/src/features/versionHistory/reducer.test.ts
- superset-frontend/src/features/versionHistory/reducer.ts
- superset-frontend/src/features/versionHistory/sessionLogMiddleware.test.ts
- superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts
- superset-frontend/src/features/versionHistory/types.ts
- superset/charts/api.py
- superset/charts/schemas.py
- superset/commands/chart/update.py
- superset/versioning/changes/listener.py
- superset/versioning/changes/normalization.py
- superset/versioning/changes/state.py
- tests/integration_tests/versioning/change_records_tests.py
- tests/unit_tests/versioning/test_normalization_changes.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
There was a problem hiding this comment.
Code Review Agent Run #1dca38
Actionable Suggestions - 1
-
superset/commands/chart/update.py - 1
- Missing guard bypasses short-circuit · Line 91-98
Additional Suggestions - 1
-
superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts - 1
-
Missing JSDoc for type design · Line 58-62The `type: unknown` in `ExploreBoundaryAction` is intentional anti-corruption layer design, but lacks documentation. Future maintainers may refactor this to `string` expecting type safety.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/versioning/changes/normalization.py - 1
- Unreachable depth check dead code · Line 144-148
Review Details
-
Files reviewed - 14 · Commit Range:
871524c..ee01270- superset-frontend/src/explore/actions/hydrateExplore.test.ts
- superset-frontend/src/explore/actions/hydrateExplore.ts
- superset-frontend/src/explore/actions/saveModalActions.test.ts
- superset-frontend/src/explore/actions/saveModalActions.ts
- superset-frontend/src/features/versionHistory/normalization.ts
- superset-frontend/src/features/versionHistory/reducer.test.ts
- superset-frontend/src/features/versionHistory/reducer.ts
- superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts
- superset-frontend/src/features/versionHistory/types.ts
- superset/charts/schemas.py
- superset/commands/chart/update.py
- superset/versioning/changes/normalization.py
- tests/integration_tests/versioning/change_records_tests.py
- tests/unit_tests/versioning/test_listener.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #bb7594Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Live verification of this branch found the ticket's oscillation surviving in one direction: StashFormDataContainer removes invisible controls' values from form_data in render effects after hydration, so a save records phantom 'Cleared' rows for keys no user touched (order_desc true->null, server_page_length 10->null, totals_aggregate "SUM"->null on the reproduction). Hydration-time tracking cannot see the stash, and the transition producer skipped disappearing keys, so the removes were never advisory-covered. The stash itself is the proof of machine-ness: a drop is covered only when the stash holds the key, the stashed value still equals the persisted value (a user edit before hiding breaks the equality and stays recorded), and the outgoing payload no longer carries the key. Keys removed by anything else -- a viz-type switch, a genuine clear -- are never in the stash and always record. The backend matcher and the save-side filter were already presence-symmetric; new tests pin that, and the producer's skip now documents where drop coverage lives. Live re-verified on the running stack: a no-edit save of a chart whose stored params carry stash-hidden keys attaches exactly the three drop transitions and records zero change rows (previously three phantom removes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Live-verification round + fix (AI-assisted session on behalf of @mikebridge; full report in the spec repo, Drove the SC-116905 reproduction against this branch on a live stack (real Explore UI via Playwright, fresh metadata DB):
Fix pushed in Re-verified live: a no-edit save of a chart carrying stash-hidden keys now attaches exactly the three drop transitions and records zero change rows (previously three phantom removes). Suites: frontend 18/18 (215 tests), backend versioning 126/126, changed-file pre-commit green. Known residual, deliberately out of scope (worth a line in the PR body): save-time stampings ( |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Review — fix(versioning): suppress automatic chart normalization changes
Reviewed at head 1a29be30ef3edae2e6d72dffa9980819a24d6bcb (base master, 6 commits, +1959/−36 across 21 files). Event: comment (non-blocking).
First, the headline: this is a careful, well-scoped, unusually well-evidenced change. The two-stack design (hydration stamps + save-time stash drops), the presence-aware symmetry between the frontend producer and the backend matcher, and the consistently fail-open construction all hang together. The live verification in particular is a cut above — driving the repro on a real stack with a fresh metadata DB, measuring 44 recorded rows on master vs 6 on the branch, then finding the residual phantom-Cleared bug yourself (order_desc/server_page_length/totals_aggregate), diagnosing it with a payload-intercept probe, and fixing it in commit 6. That quality of evidence is exactly what a change to a provenance surface warrants. I did not find a blocking issue.
The crux: client-asserted suppression of a provenance surface (non-blocking, worth naming)
The mechanism is that the client tells the server "these param changes were automatic, omit them from readable history," so the governing question is whether a crafted client can suppress a genuine user edit.
What the server independently corroborates (normalization.py:195 matching_normalization_context): for every advisory transition it checks presence in both directions against the real before_params (persisted Slice.params) and after_params (submitted params), and — when present — that from_value/to_value exactly equal the persisted/submitted values (_json_equal, no True==1 coercion). A transition that misdescribes reality is dropped and its record survives (test_stale_normalization_metadata_fails_open_through_chart_put).
What it does not — and structurally cannot — corroborate is authorship. The corroboration only confirms the transition truthfully describes the real before→after of that control; it cannot tell a machine normalization from a human edit, because a genuine edit is also a truthful before→after. The stash-drop case (from_value == persisted, key absent after) is the sharp edge the task flags: it is byte-identical to a user genuinely clearing that control, and the stash-membership proof lives entirely client-side (explore.hiddenFormData). So yes — a crafted client can omit any one params-control change (including a genuine clear or edit) from readable history by attaching a matching transition. That is a real property of the design, not a bug in it.
Severity, scoped honestly — low / acceptable by design, not a security finding:
- The actor must already hold chart editorship (
raise_for_editorship,update.py:202); an editor can already make any change they like. - Suppression touches only the human-readable
version_changesrows. The Continuum canonical shadows (slices_version, etc.) are written independently and are not filtered, so the ground-truth snapshot for that transaction — and restore — still reflects the true params. An auditor can reconstruct the change by diffing snapshot N-1↔N. SECURITY.mddoes not positionversion_changesas a tamper-evident/compliance audit log, and there is no role-and-capability-matrix row an editor violates by omitting a readable diff row of their own edit. Per the doc's own test ("an action the matrix does not entitle them to"), this is out of scope as a vulnerability.
The one thing I'd suggest: the PR body frames the backend validation as the defense ("validation against the exact persisted and submitted params"). That is accurate but easy to over-read — it defends against fabrication (suppressing a change that didn't happen), not against authorship spoofing. A sentence in the design note stating plainly that an editor can suppress their own readable-diff rows and that the canonical shadows remain the source of truth would set the right expectation for anyone who later leans on this surface. Not blocking.
Fail-open — verified by construction ✅
I traced every branch in normalization.py and found no path where malformed/ambiguous/mismatched metadata short-circuits into suppression:
sanitize_normalization_changes: non-list, >256 entries, >256 KiB, or anyjson.dumps/TypeError/ValueError/UnicodeError/RecursionError→()(record everything). Over-depth (>20) raises_InvalidNormalizationEnvelopeError, caught →(). Duplicate control → whole envelope rejected →().- Per-entry malformity (
_parse_normalization_transition→None) skips that one entry only, so its control still records; thefrom_present ⇔ "from_value" presentsymmetry check rejects ambiguous encodings. matching_normalization_context: only exact matches survive; no match →None→ nothing filtered.register_matching_normalization_context:try/except Exception→ log + no context.store_normalization_context: a second same-chart context in one tx invalidates both → consume returnsNone→ records (ambiguity fails open;test_context_is_consumed_once_and_same_chart_ambiguity_fails_open).bulk_insert_records: thefilter_normalization_recordscall is wrapped so an exception leavesrecordsbound to the unfiltered list.filter_normalization_recordscan only remove records for positively-matched controls — there is no "suppress all" path.
Direction of failure is consistently toward recording. Good.
Feature-flag gating — inert when off ✅ (one minor note)
- Backend: with
ENABLE_VERSIONING_CAPTUREoff,init_versioning()returns beforeregister_change_record_listener()and detaches Continuum's writers (initialization/__init__.py:817), so noversion_changesrows are written at all — suppression is moot. - Frontend:
payload.normalization_changesis attached only whenisFeatureEnabled(FeatureFlag.VersionHistory) && tracking.chartId === sliceId(saveModalActions.ts:271); flag off ⇒ nothing sent. - Minor (non-blocking):
register_matching_normalization_contextinupdate.py:91is not gated onENABLE_VERSIONING_CAPTURE, so when a client sendsnormalization_changeswhile capture is off, the backend still parses+matches and (on a match) leaves aNORMALIZATION_CONTEXT_KEYregistry onsession.infothat the cleanup listener — unregistered in that mode — never reaps. It's bounded to the request (scoped session is torn down at request end) and has no functional effect, but a cheapis_versioning_enabled()short-circuit there would make the path truly inert and save the wastedjson.loadsof both param blobs.
Commit 6 — does what it claims ✅
stashDropNormalizationTransitions (normalization.ts:160) emits a present→absent transition only when all four hold: stash holds the key, persisted holds it, outgoing payload no longer carries it, and hiddenFormData[control] still jsonValuesEqual persisted. The "user edited before hiding" case breaks the equality and records (normalization.test.ts:128). The hydration producer's !toPresent early-return is now documented as deliberately ceding drop coverage to this path. Backend test_drop_transition_matches_and_filters_a_remove_record / _requires_the_key_to_be_absent_after pin the matcher side. Consistent end to end.
Declared residual — deliberate and correctly scoped ✅
dashboards, extra_form_data, and the granularity_sqla/time_range → adhoc_filters temporal migration record once on a first modernizing save. This falls out of the design rather than being an unexamined gap: matching runs against the post-getSlicePayload savedFormData (this is also the fix for the resolved codeant thread), so a control the payload rewriter migrated away no longer matches its recorded to_present/to_value and correctly fails open to recording. Documenting it in the PR body is the right call.
Rule 26
- Backend — RAN.
tests/unit_tests/versioning/test_normalization_changes.py→ 8 passed. Neutering the suppression (filter_normalization_recordscontrol-set → empty) failed exactly the two suppression legs (test_filter_returns_fresh_records_without_matching_params_control,test_drop_transition_matches_and_filters_a_remove_record) while the fail-open/ambiguity/genuine-record legs stayed green. The "genuine change still records" negative leg is covered at integration level bytest_stale_normalization_metadata_fails_open_through_chart_put(mismatchedfrom_value⇒ realparams.row_limitchange recorded) andtest_matching_hydration_metadata_omits_only_normalization_noise(genuineslice_nameedit records alongside suppressed noise). Reverted cleanly. - Frontend — INSPECTED (
node_modulesabsent).normalization.test.tscovers the over-suppression guards directly: stashed value the user changed before hiding is not covered (:128), stashed-but-never-persisted not covered (:138), key still in payload not covered (:144), drop requires a stash (:154), and hydration transitions only when input matched persisted (:37).matchingAutomaticNormalizationTransitionsadditionally re-checksinvalidatedControlsand re-validates the outgoing value againstto_value, so a control the user touched post-hydration drops out.
Resolved-thread spot-check ✅
- codeant /
saveModalActions.ts(payload rewriter): matching now runs againstsavedFormData = JSON.parse(payload.params)(:291), i.e. the actual outgoing params — genuinely addressed, not just marked resolved. - bito /
update.py:91(missingis not Noneshort-circuit): the guardself._normalization_changes is not None and "params" in self._propertiesis present at head.
CI
Warm re-read of statusCheckRollup: 77 SUCCESS (CheckRun) + 1 SUCCESS (StatusContext), 3 NEUTRAL, 3 SKIPPED, zero failures, zero in-progress — fully green.
Verdict: No blocking issues. The suppression mechanism is sound and consistently fail-open; the one property worth stating explicitly is that this is a client-asserted omission from a readable history surface (canonical shadows unaffected), acceptable within the SECURITY.md model but worth a sentence in the design note. Minor, all optional: (1) note the authorship-spoofing property + shadow-as-source-of-truth in the body; (2) gate the backend register_matching_normalization_context on the capture flag for true inertness. Nicely done, especially the live verification and the self-found commit-6 fix.
Automated review. Approval intentionally withheld — external contributor; a human maintainer owns the approve/merge decision.
Code Review Agent Run #36b2feActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Chart params stored in the metadata database are not guaranteed to be canonicalized against the active Explore control defaults. Non-canonical data can come from chart imports, fixtures, older Superset versions, API or other non-Explore writers, and changes to a visualization plugin's defaults. When Explore hydrates one of these charts, it may populate default control values that were absent or null in the persisted params. Saving immediately afterward made those automatic normalization changes appear as user-authored version history.
The solution collects advisory evidence at the two points where the machine — and provably not the user — changes params, rather than trying to infer intent from the final save payload:
StashFormDataContainermoves invisible controls' values out ofform_datain render effects, which hydration-time tracking cannot see, so a later save would record phantom "Cleared" entries. At save time, a drop transition is attached for a key only when the stash itself holds it, the stashed value still equals the persisted value (a user edit before hiding breaks the equality and stays recorded), and the outgoing payload no longer carries it. Keys removed any other way — a viz-type switch, a genuine clear — are never in the stash and always record.The backend bounds and validates the metadata against the exact persisted and submitted params (both directions of presence), then omits only matching normalization noise from the human-readable change records. Persisted chart params and complete restorable version snapshots remain unchanged.
The metadata is fail-open: malformed, stale, ambiguous, or mismatched entries do not suppress history.
Known, deliberate residual: keys the save path itself stamps (
dashboards,extra_form_data, and thegranularity_sqla/time_range→adhoc_filterstemporal migration) still record once on the first modernizing save — the temporal migration is arguably genuine history.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Version-history panel for the same scenario — a chart whose params were authored outside the current Explore serializer (examples fixtures, imports, API-created charts), edited through the UI.
Before — a save where the user touched nothing records four phantom "Cleared" entries (the stash removing invisible controls' values):
After — the same cycle records exactly the user's one genuine change:
TESTING INSTRUCTIONS
ENABLE_VERSIONING_CAPTURE.order_desc,server_page_length,totals_aggregateon a Table chart).Automated coverage:
pytest -q tests/unit_tests/versioning/test_normalization_changes.py tests/unit_tests/versioning/test_listener.py— 17 passed (includes drop-transition matching/filtering); integration suiteschange_records_tests.py+version_restore_tests.pyunchanged by the drop coverageThe repository-wide all-files check also exposes pre-existing current-master failures outside this diff: a MyPy error in
superset/semantic_layers/models.pyand unrelated ECharts test type errors.ADDITIONAL INFORMATION
ENABLE_VERSIONING_CAPTURE