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 `447693ad99`. Dataset certification is hydrated into
editor state instead of rewriting `extra` per keystroke, and its handler merges
only certification fields through a functional `setDatasource` update. The save
path applies those fields to the latest `datasource.extra`.
`DatasourceEditorDatasetCertification.test.tsx` 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 `447693ad99`. Malformed or non-object Extra is
detected with `isDatasetExtraValid`; 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 `447693ad99`. The merge removes only `certified_by` and `details`;
it preserves unknown certification subkeys and deletes the `certification`
object only when nothing remains. The clear regression uses the UI's actual `{
certified_by: '', certification_details: '' }` shape and verifies `expires_at`
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 `447693ad99`. 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 `undefined`), 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 `447693ad99`. 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 `waitFor`s on the
PUT body. A separate editor test fires the same changes back-to-back and
`waitFor`s on committed `onChange` 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 `447693ad99`. The certification
controls no longer write `datasource.extra` 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 `447693ad99`. Tests now verify unknown `certification.expires_at`
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. `DatasetCertification` is also
narrowed to only its two supported fields instead of intersecting
`Record<string, unknown>`.
--
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]
