fitzee commented on code in PR #43633:
URL: https://github.com/apache/superset/pull/43633#discussion_r3902010122
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1631,6 +1635,49 @@ function DatasourceEditor({
onDatasourceChange,
]);
+ const renderCertificationFieldset = useCallback(() => {
+ const certification = getDatasetCertification(datasource.extra);
+
+ return isSqla ? (
+ <Fieldset
+ title={t('Certification')}
+ item={certification}
+ onChange={updatedCertification => {
+ onDatasourceChange({
+ ...datasource,
+ extra: setDatasetCertification(
+ datasource.extra,
+ updatedCertification,
+ ),
+ });
Review Comment:
Confirmed and fixed in . Dataset certification is hydrated into editor state
instead of rewriting per keystroke, and its handler merges only certification
fields through a functional update. The save path applies those fields to the
latest . fires Default URL, Certified by, and details changes back-to-back and
waits for the committed datasource to contain all three values.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts:
##########
@@ -0,0 +1,87 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export type DatasetCertification = Record<string, unknown> & {
+ certified_by?: string;
+ certification_details?: string;
+};
+
+type JsonObject = Record<string, unknown>;
+
+const isJsonObject = (value: unknown): value is JsonObject =>
+ typeof value === 'object' && value !== null && !Array.isArray(value);
+
+const parseExtra = (extra?: string): JsonObject | undefined => {
+ if (!extra?.trim()) {
+ return {};
+ }
+
+ try {
+ const parsed: unknown = JSON.parse(extra);
+ return isJsonObject(parsed) ? parsed : undefined;
+ } catch {
+ return undefined;
+ }
+};
+
+export const getDatasetCertification = (
+ extra?: string,
+): DatasetCertification => {
+ const certification = parseExtra(extra)?.certification;
+ if (!isJsonObject(certification)) {
+ return {};
+ }
+
+ return {
+ certified_by:
+ typeof certification.certified_by === 'string'
+ ? certification.certified_by
+ : undefined,
+ certification_details:
+ typeof certification.details === 'string'
+ ? certification.details
+ : undefined,
+ };
+};
+
+export const setDatasetCertification = (
+ extra: string | undefined,
+ { certified_by, certification_details }: DatasetCertification,
+): string => {
+ const parsedExtra = parseExtra(extra);
+
+ // Do not replace malformed raw metadata while the user is correcting it in
+ // the adjacent Extra editor.
+ if (!parsedExtra) {
+ return extra ?? '';
+ }
Review Comment:
Confirmed and fixed in . Malformed or non-object Extra is detected with ;
both certification inputs are disabled and show “Fix the Extra JSON to edit
certification” rather than accepting a value that cannot be saved. The new
editor regression test covers the disabled/error state, and helper tests cover
malformed and non-object roots.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts:
##########
@@ -0,0 +1,87 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export type DatasetCertification = Record<string, unknown> & {
+ certified_by?: string;
+ certification_details?: string;
+};
+
+type JsonObject = Record<string, unknown>;
+
+const isJsonObject = (value: unknown): value is JsonObject =>
+ typeof value === 'object' && value !== null && !Array.isArray(value);
+
+const parseExtra = (extra?: string): JsonObject | undefined => {
+ if (!extra?.trim()) {
+ return {};
+ }
+
+ try {
+ const parsed: unknown = JSON.parse(extra);
+ return isJsonObject(parsed) ? parsed : undefined;
+ } catch {
+ return undefined;
+ }
+};
+
+export const getDatasetCertification = (
+ extra?: string,
+): DatasetCertification => {
+ const certification = parseExtra(extra)?.certification;
+ if (!isJsonObject(certification)) {
+ return {};
+ }
+
+ return {
+ certified_by:
+ typeof certification.certified_by === 'string'
+ ? certification.certified_by
+ : undefined,
+ certification_details:
+ typeof certification.details === 'string'
+ ? certification.details
+ : undefined,
+ };
+};
+
+export const setDatasetCertification = (
+ extra: string | undefined,
+ { certified_by, certification_details }: DatasetCertification,
+): string => {
+ const parsedExtra = parseExtra(extra);
+
+ // Do not replace malformed raw metadata while the user is correcting it in
+ // the adjacent Extra editor.
+ if (!parsedExtra) {
+ return extra ?? '';
+ }
+
+ if (certified_by || certification_details) {
+ const existingCertification = parsedExtra.certification;
+ parsedExtra.certification = {
+ ...(isJsonObject(existingCertification) ? existingCertification : {}),
+ certified_by: certified_by || undefined,
+ details: certification_details || undefined,
+ };
+ } else {
+ delete parsedExtra.certification;
Review Comment:
Fixed in . The merge removes only and ; it preserves unknown certification
subkeys and deletes the object only when nothing remains. The clear regression
uses the UI's actual shape and verifies survives.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts:
##########
@@ -0,0 +1,87 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export type DatasetCertification = Record<string, unknown> & {
+ certified_by?: string;
+ certification_details?: string;
+};
+
+type JsonObject = Record<string, unknown>;
+
+const isJsonObject = (value: unknown): value is JsonObject =>
+ typeof value === 'object' && value !== null && !Array.isArray(value);
+
+const parseExtra = (extra?: string): JsonObject | undefined => {
+ if (!extra?.trim()) {
+ return {};
+ }
+
+ try {
+ const parsed: unknown = JSON.parse(extra);
+ return isJsonObject(parsed) ? parsed : undefined;
+ } catch {
+ return undefined;
+ }
+};
+
+export const getDatasetCertification = (
+ extra?: string,
+): DatasetCertification => {
+ const certification = parseExtra(extra)?.certification;
+ if (!isJsonObject(certification)) {
+ return {};
+ }
+
+ return {
+ certified_by:
+ typeof certification.certified_by === 'string'
+ ? certification.certified_by
+ : undefined,
+ certification_details:
+ typeof certification.details === 'string'
+ ? certification.details
+ : undefined,
+ };
+};
+
+export const setDatasetCertification = (
+ extra: string | undefined,
+ { certified_by, certification_details }: DatasetCertification,
+): string => {
+ const parsedExtra = parseExtra(extra);
+
+ // Do not replace malformed raw metadata while the user is correcting it in
+ // the adjacent Extra editor.
+ if (!parsedExtra) {
+ return extra ?? '';
+ }
+
+ if (certified_by || certification_details) {
+ const existingCertification = parsedExtra.certification;
+ parsedExtra.certification = {
+ ...(isJsonObject(existingCertification) ? existingCertification : {}),
+ certified_by: certified_by || undefined,
+ details: certification_details || undefined,
+ };
+ } else {
+ delete parsedExtra.certification;
+ }
+
+ return JSON.stringify(parsedExtra, null, 2);
Review Comment:
Fixed in . Certification edits no longer serialize Extra on each keystroke;
they remain in first-class editor state and merge once at save. The helper
returns the original raw string when values are unchanged (including preserving
formatting and ), so type-then-clear on an empty dataset does not create . Unit
coverage pins both cases.
##########
superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx:
##########
@@ -135,6 +136,81 @@ describe('DatasourceModal', () => {
expect(JSON.parse(putCall?.options?.body as string).editors).toEqual([1]);
});
+ test('saves dataset certification from Settings without dropping Extra
metadata', async () => {
+ cleanup();
+ renderAndWait({
+ ...mockedProps,
+ datasource: {
+ ...mockedProps.datasource,
+ extra: JSON.stringify({
+ custom_key: { enabled: true },
+ warning_markdown: 'Use only finalized records',
+ }),
+ } as typeof mockedProps.datasource & { extra: string },
+ });
+
+ await userEvent.click(await screen.findByRole('tab', { name: 'Settings'
}));
+
+ const certifiedBy = await screen.findByPlaceholderText('Certified by');
+ fireEvent.change(certifiedBy, { target: { value: 'E2E Team' } });
+ await new Promise(resolve => setTimeout(resolve, 500));
Review Comment:
Fixed in . The two real-time 500 ms sleeps are gone. The modal test fires
Default URL and both certification changes in one debounce window, advances
only the configured debounce with fake timers, and s on the PUT body. A
separate editor test fires the same changes back-to-back and s on committed
state. The focused run is 4 suites / 30 tests passing.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1631,6 +1635,49 @@ function DatasourceEditor({
onDatasourceChange,
]);
+ const renderCertificationFieldset = useCallback(() => {
+ const certification = getDatasetCertification(datasource.extra);
Review Comment:
Confirmed and addressed structurally in . The certification controls no
longer write during editing, so they cannot make the uncontrolled Ace buffer
stale. They hydrate once into dedicated editor fields and serialize at save
against the latest raw Extra only when those fields were actually edited; raw
Extra remains authoritative otherwise. The Extra help text now directs
certification edits to the adjacent section.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts:
##########
@@ -0,0 +1,87 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export type DatasetCertification = Record<string, unknown> & {
+ certified_by?: string;
+ certification_details?: string;
+};
+
+type JsonObject = Record<string, unknown>;
+
+const isJsonObject = (value: unknown): value is JsonObject =>
+ typeof value === 'object' && value !== null && !Array.isArray(value);
+
+const parseExtra = (extra?: string): JsonObject | undefined => {
+ if (!extra?.trim()) {
+ return {};
+ }
+
+ try {
+ const parsed: unknown = JSON.parse(extra);
+ return isJsonObject(parsed) ? parsed : undefined;
+ } catch {
+ return undefined;
+ }
+};
+
+export const getDatasetCertification = (
+ extra?: string,
+): DatasetCertification => {
+ const certification = parseExtra(extra)?.certification;
+ if (!isJsonObject(certification)) {
+ return {};
+ }
+
+ return {
+ certified_by:
+ typeof certification.certified_by === 'string'
+ ? certification.certified_by
+ : undefined,
+ certification_details:
+ typeof certification.details === 'string'
+ ? certification.details
+ : undefined,
+ };
+};
+
+export const setDatasetCertification = (
+ extra: string | undefined,
+ { certified_by, certification_details }: DatasetCertification,
+): string => {
+ const parsedExtra = parseExtra(extra);
+
+ // Do not replace malformed raw metadata while the user is correcting it in
+ // the adjacent Extra editor.
+ if (!parsedExtra) {
+ return extra ?? '';
+ }
+
+ if (certified_by || certification_details) {
+ const existingCertification = parsedExtra.certification;
+ parsedExtra.certification = {
+ ...(isJsonObject(existingCertification) ? existingCertification : {}),
Review Comment:
Covered in . Tests now verify unknown survives both edit and clear,
details-only writes work, UI-style empty-string clears work,
malformed/non-object Extra and non-object certification are handled, and
unchanged formatting is preserved. is also narrowed to only its two supported
fields instead of intersecting .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
