fix(dataset-editor): drop null warning_markdown from extra JSON serialisation#39706
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mikebridge
force-pushed
the
sc-104972-fix-extra-rewrite
branch
from
17c3fef to
385c312
Compare
|
Not a part of the codebase I'm familiar with at all - but this seems like a clear improvement to me! Thanks for explaining in such detail. |
mikebridge
force-pushed
the
sc-104972-fix-extra-rewrite
branch
4 times, most recently
from
610d43e to
6f0345f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39706 +/- ##
=======================================
Coverage 64.49% 64.49%
=======================================
Files 2566 2566
Lines 133972 133973 +1
Branches 31124 31125 +1
=======================================
+ Hits 86403 86404 +1
Misses 46074 46074
Partials 1495 1495
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run #2f2784Actionable 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 |
…lisation
The `buildExtraJsonObject` helper in `DatasourceModal/index.tsx` was
silently rewriting every column's and metric's `extra` field from `"{}"`
to `'{"warning_markdown":null}'` on every save, even when no warning
was set or touched.
Round-trip:
1. Backend `TableColumn.data` exposes `warning_markdown` at top level
from `CertificationMixin`, returning Python `None` when the key is
absent in the stored `extra` JSON.
2. The API response delivers `column.warning_markdown: null` to the
frontend.
3. `buildExtraJsonObject` re-serialised that null back into `extra`
via `JSON.stringify({ ..., warning_markdown: item?.warning_markdown })`.
`JSON.stringify` keeps explicit `null`; only `undefined` is dropped.
4. Save POSTs the rewritten `extra`; backend stores it; next load
round-trips the same way. The rewrite is permanent.
The two forms (`"{}"` and `'{"warning_markdown":null}'`) are
functionally equivalent for downstream consumers, so users have not
noticed. The bug surfaces in environments that compare pre- and
post-save state at the byte level — most visibly the entity-versioning
diff engine (sc-103156), which produced N+1 spurious change records
per save (one per existing column whose `extra` was rewritten, plus
the user-intended change).
Fix: coerce empty/null `warning_markdown` to `undefined` before
serialisation. `JSON.stringify` then drops the key entirely. Matches
the existing UI semantic where `warning_markdown && <icon />` already
treats empty/null as absent. The fix applies uniformly to columns
and metrics — both flow through the same helper.
Existing rows whose `extra` is `'{"warning_markdown":null}'` heal
naturally on their next save (the helper now serialises them to `{}`).
No data migration needed.
Verified locally on the entity-versioning branch: spurious change
records vanish; only user-intended changes appear in the
version-history diff. Closes sc-104972.
Tests:
- Five new unit tests in DatasourceModal.test.tsx covering the
empty/null/empty-string/truthy/certification-and-null cases against
the now-exported `buildExtraJsonObject` helper.
- Existing 17 DatasourceModal tests pass unmodified.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-104972-fix-extra-rewrite
branch
from
6f0345f to
40c2592
Compare
|
The current code sets |
Code Review Agent Run #5d7febActionable Suggestions - 0Additional Suggestions - 1
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 |
…lisation (#39706) Co-authored-by: Mike Bridge <michael.bridge@ext.preset.io> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
SUMMARY
Every Dataset Editor save silently rewrote every column's and metric's
extraJSON-string field from\"{}\"to'{\"warning_markdown\":null}', even when no one touched any warning-related setting. The two forms are functionally equivalent for downstream consumers, so the bug has been latent in production for a long time. It became visible during the entity-versioning work, where the diff engine surfaced N+1 spurious change records per save — one per existing column whoseextragot rewritten, plus the actual user-intended change.Root cause
TableColumn.data(superset/connectors/sqla/models.py:1075-1094) serialises every column withwarning_markdownexposed at the top level, derived fromCertificationMixin.warning_markdown. That property parsesextraJSON and returns PythonNonewhen the key is absent — which serialises to JSONnull.column.warning_markdown: nullto the frontend.buildExtraJsonObject(inDatasourceModal/index.tsx) re-serialises that null back intoextraviaJSON.stringify({ ..., warning_markdown: item?.warning_markdown }).JSON.stringifykeeps explicitnull; onlyundefinedis dropped.extra; the backend stores it; the next load round-trips identically. The rewrite is permanent.Fix
One expression change in
buildExtraJsonObject: coerce empty/nullwarning_markdowntoundefinedbefore serialisation.JSON.stringifythen drops the key entirely when the value is empty/null. Matches the existing UI semantic wherewarning_markdown && <WarningIconWithTooltip />already treats empty/null as absent. The fix applies uniformly to columns and metrics — both flow through the same helper.The helper is also now exported (was module-local) so the new unit tests can call it directly.
BEFORE/AFTER
Pre-fix on the entity-versioning branch, adding one calculated column to a dataset with 8 existing columns produces 9 records in
version_changes:After this fix on the same branch, the same edit produces exactly 1 record — only the actual added column.
TESTING INSTRUCTIONS
Open the Dataset Editor on any dataset with several columns (e.g., `birth_names`).
Settings tab → edit Description → Save.
Query Postgres directly:
Expected: every column's `extra` is unchanged from before the save (still `'{}'` for columns that started that way). Pre-fix, every column's `extra` would have been rewritten to `'{"warning_markdown":null}'`.
Regression check — warning roundtrip: Metrics tab → expand a metric → set "⚠ test" in the Warning field → Save → reopen → confirm warning icon appears. Then expand the metric → clear the field → Save → reopen → confirm warning icon is gone.
Run the unit tests:
Existing data healing
Rows whose `extra` is currently `'{"warning_markdown":null}'` (pre-fix noise from previous saves) heal naturally on their next save — the helper serialises them to `'{}'`. No data migration needed.
Out of scope
ADDITIONAL INFORMATION
🤖 Generated with Claude Code