aminghadersohi commented on code in PR #43633: URL: https://github.com/apache/superset/pull/43633#discussion_r3883131995
########## 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: This spread is untested — deleting it leaves all four cases in `datasetCertification.test.ts` green, because no fixture puts an unrecognized key inside `certification`. Verified against the fixtures. That matters beyond coverage: this line is the sole evidence that unknown sub-keys are meant to survive an edit, which is the premise of my note on line 83 about the clear path dropping them. Right now nothing pins the intended behaviour in either direction. Adding a case where `certification: { certified_by, details, expires_at }` is edited and `expires_at` survives would fix that; if the answer is that unknown sub-keys are not worth preserving, delete the spread and make the two paths agree. Also uncovered: non-object `extra` / non-object `certification`, details-only writes, and the input the UI actually sends when a user clears the fields — `{ certified_by: '', certification_details: '' }`, not `{}`. ########## 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: **Confirming @mikebridge's second High — it holds, and this diff is what activates it.** Verified by reading the code at HEAD (not executed; no node_modules in this worktree, so please run it before merging): - `TextAreaControl` passes `defaultValue={initialValue ?? value}` to the react-ace `TextAreaEditor` and never `value` (`TextAreaControl.tsx:192`). react-ace applies `defaultValue` on mount only. - The remount escape hatch is `key={name}` (`TextAreaControl.tsx:194`), but `Field` clones the control with only `value`, `onChange` and `label` — it does not forward `name` (`Field/index.tsx:83`). The `extra` Field passes no `name` either, so `key` is `undefined` and the editor never remounts. - `value` is destructured out of props, so it cannot reach the editor through `{...restProps}`. The Extra editor is therefore permanently uncontrolled after mount. That was harmless while it was the only writer of `datasource.extra`; this PR makes the certification fields a second writer of the same field, in the same column, in the same tab. Sequence: fill Certified by → `extra` now carries `certification`, but the Extra editor still displays the pre-certification JSON → the user types one character in Extra (e.g. adding `warning_markdown`) → it commits its stale buffer through the Basic fieldset → `certification` is gone from state and both certification fields visibly reset to empty, because `getDatasetCertification` re-derives from the reverted string on this line. Worth deciding deliberately rather than patching: a `key` bumped only from the certification `onChange` fixes the display, but two independently-debounced controls bound to one JSON blob will keep producing this class of bug. The metric path's shape — hydrate on load, serialize once at save — avoids it structurally. -- 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]
