aminghadersohi commented on code in PR #43633: URL: https://github.com/apache/superset/pull/43633#discussion_r3877773763
########## 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: Two side effects of always re-serializing here: 1. Every keystroke in Certified by rewrites the entire Extra blob to 2-space JSON. The raw `extra` `TextAreaControl` is rendered directly below in the same column (`DatasourceEditor.tsx`, `fieldKey="extra"`), so the user watches their own formatting get rewritten under them while they type in an adjacent field. 2. On a dataset with no `extra`, typing a value and then clearing it leaves `extra` as the literal `"{}"` rather than empty — a no-op edit now dirties the record. Consider returning `extra ?? ''` when the result is an empty object, and leaving `extra` untouched when the certification values did not actually change. ########## 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: The clear path is asymmetric with the write path. Line 78 deliberately spreads `...existingCertification` so unrecognized keys inside `certification` survive an edit, but clearing both fields deletes the whole object and takes those keys with it: ``` setDatasetCertification( '{"certification":{"certified_by":"A","details":"B","expires_at":"2030-01-01"}}', {}, ) // => "{}" (expires_at gone) ``` If preserving unknown sub-keys is worth doing on write it is worth doing on clear — delete only `certified_by`/`details` and drop `certification` only once the object is empty. As written this also weakens the PR description's "clearing the fields removes only certification". ########## 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: These two real-timer `setTimeout(500)` sleeps step each field edit past `FAST_DEBOUNCE` one at a time. That is what makes the test unable to catch the interleaving hazard codeant flagged on `DatasourceEditor.tsx:1652` — it only ever exercises the fully serialized path, which is the one case that cannot race. A test that fires both `change` events back to back and then `waitFor`s on the PUT body would cover the case that actually worries me here. Separately, real-timer sleeps are a flake and latency source in a shared jest shard; prefer `waitFor` on the committed value over a fixed 500ms. -- 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]
