From 40c259281e397aa98dd2dbc9004f4796477f2cf7 Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Mon, 27 Apr 2026 15:40:46 -0600 Subject: [PATCH] fix(dataset-editor): drop null warning_markdown from extra JSON serialisation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 && ` 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) --- .../DatasourceModal/DatasourceModal.test.tsx | 34 ++++++++++++++++++- .../Datasource/DatasourceModal/index.tsx | 4 +-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx index 816da881b913..e095020126d7 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx @@ -29,7 +29,7 @@ import fetchMock from 'fetch-mock'; import { SupersetClient } from '@superset-ui/core'; import mockDatasource from 'spec/fixtures/mockDatasource'; import React from 'react'; -import DatasourceModalComponent from '.'; +import DatasourceModalComponent, { buildExtraJsonObject } from '.'; // Cast to accept partial mock props in tests const DatasourceModal = DatasourceModalComponent as unknown as React.FC< @@ -316,3 +316,35 @@ describe('DatasourceModal', () => { }); }); }); + +describe('buildExtraJsonObject', () => { + test('returns "{}" for an item with no warning and no certification', () => { + expect(buildExtraJsonObject({} as any)).toBe('{}'); + }); + + test('drops warning_markdown when its value is null', () => { + expect(buildExtraJsonObject({ warning_markdown: null } as any)).toBe('{}'); + }); + + test('drops warning_markdown when its value is an empty string', () => { + expect(buildExtraJsonObject({ warning_markdown: '' } as any)).toBe('{}'); + }); + + test('preserves a non-empty warning_markdown verbatim', () => { + expect(buildExtraJsonObject({ warning_markdown: '⚠ caveat' } as any)).toBe( + '{"warning_markdown":"⚠ caveat"}', + ); + }); + + test('preserves certification and drops null warning_markdown', () => { + expect( + buildExtraJsonObject({ + certified_by: 'data-team', + certification_details: 'verified', + warning_markdown: null, + } as any), + ).toBe( + '{"certification":{"certified_by":"data-team","details":"verified"}}', + ); + }); +}); diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx index 65744e543eb7..4049268c3e57 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx @@ -71,7 +71,7 @@ const StyledDatasourceModal = styled(Modal)` } `; -function buildExtraJsonObject( +export function buildExtraJsonObject( item: DatasetObject['metrics'][0] | DatasetObject['columns'][0], ) { const certification = @@ -83,7 +83,7 @@ function buildExtraJsonObject( : undefined; return JSON.stringify({ certification, - warning_markdown: item?.warning_markdown, + warning_markdown: item?.warning_markdown || undefined, }); }