aminghadersohi commented on code in PR #43633:
URL: https://github.com/apache/superset/pull/43633#discussion_r3906348704
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1631,12 +1639,67 @@ function DatasourceEditor({
onDatasourceChange,
]);
+ const renderCertificationFieldset = useCallback(() => {
+ const certificationError = !isDatasetExtraValid(datasource.extra)
+ ? t('Fix the Extra JSON to edit certification')
+ : undefined;
+
+ return isSqla ? (
+ <Fieldset
+ title={t('Certification')}
+ item={datasource}
+ onFieldChange={(fieldKey, value) => {
+ if (
+ fieldKey !== 'certified_by' &&
+ fieldKey !== 'certification_details'
+ ) {
+ return;
+ }
+ setDatasource(previousDatasource => ({
+ ...previousDatasource,
+ [fieldKey]: typeof value === 'string' ? value : undefined,
+ dataset_certification_changed: true,
Review Comment:
`dataset_certification_changed` is one-way — nothing resets it, so it stays
true for the rest of the session after a single keystroke. Two consequences,
both narrow:
The harmless one: type a character into Certified by, delete it, save. The
flag is still true, but `setDatasetCertification` early-returns because the
normalized values match what was hydrated, so `extra` is untouched. Fine as-is.
The one worth a decision: it makes the field silently authoritative over a
hand-edit to `certification` inside Extra, whichever the user touched last.
```
setDatasetCertification('{"certification":{"certified_by":"TeamB"}}', {
certified_by: 'TeamA' })
// => {"certification":{"certified_by":"TeamA"}}
```
Reachable when someone touches a certification field and *then* edits
`certification` in the Extra box in the same session — hydration is mount-only,
so the fields never re-derive and the user gets no signal that their Extra edit
will lose. The new help text steers people away from hand-writing
certification, so this is a corner; but "the dedicated fields win" is currently
an undocumented, invisible precedence rule. Re-deriving the fields when `extra`
changes, or clearing the flag when the values return to their hydrated
originals, would both close it.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts:
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 = {
+ 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 isDatasetExtraValid = (extra?: string): boolean =>
+ parseExtra(extra) !== 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 | undefined => {
+ 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;
+ }
+
+ const normalizedCertifiedBy = certified_by || undefined;
+ const normalizedDetails = certification_details || undefined;
+ const existing = getDatasetCertification(extra);
+
+ // Avoid reformatting raw Extra JSON when the certification did not change.
+ if (
+ existing.certified_by === normalizedCertifiedBy &&
+ existing.certification_details === normalizedDetails
+ ) {
+ return extra;
+ }
+
+ const existingCertification = parsedExtra.certification;
+ const certification = isJsonObject(existingCertification)
+ ? { ...existingCertification }
+ : {};
+ delete certification.certified_by;
+ delete certification.details;
+
+ if (normalizedCertifiedBy) {
+ certification.certified_by = normalizedCertifiedBy;
+ }
+ if (normalizedDetails) {
+ certification.details = normalizedDetails;
+ }
+
+ if (Object.keys(certification).length > 0) {
+ parsedExtra.certification = certification;
+ } else {
+ delete parsedExtra.certification;
+ }
+
+ return JSON.stringify(parsedExtra);
Review Comment:
This dropped the `, null, 2`, so the first real certification edit collapses
a hand-formatted Extra blob to one line:
```
setDatasetCertification('{\n "a": 1,\n "b": 2\n}', { certified_by: 'X' })
// => {"a":1,"b":2,"certification":{"certified_by":"X"}}
```
The early return above already establishes that preserving the user's
formatting is the intent, and this is the one path that still discards it.
`JSON.stringify(parsedExtra, null, 2)` gets most of the way there; not
re-serializing at all unless the object actually differs would finish it.
##########
superset-frontend/src/components/Datasource/components/Fieldset/index.tsx:
##########
@@ -51,12 +53,21 @@ export default function Fieldset({
const handleChange = useCallback(
(fieldKey: fieldKeyType, val: any) => {
- onChange?.({
+ const updatedItem = {
...itemRef.current,
[fieldKey]: val,
- });
+ };
+ // Multiple debounced controls can commit in the same React batch, before
+ // the effect above has synchronized the item passed back by the parent.
+ // Advance the ref synchronously so the later commit includes its
sibling.
+ itemRef.current = updatedItem;
Review Comment:
Worth a comment saying what this is still for. With `onFieldChange` in
place, this synchronous advance is dead for every consumer that uses it — the
field-level path never reads `updatedItem`. It is load-bearing only for the one
remaining `onChange` consumer, the `compact` Fieldset at
`DatasourceEditor.tsx:1901`, which pairs with `onDatasourceChange` doing a
non-functional whole-object `setDatasource(newDatasource)` — the exact shape
this PR removed everywhere else, and `fieldKey="sql"` does route through it.
I could not construct a reachable failure: that Fieldset is on the Source
tab, so interleaving it with a Settings-tab edit inside one 500 ms window needs
a tab switch, and the sub-frame ordering it would otherwise need is not
something a user can hit. So this is a latent shape, not a bug. Your note that
full-record behaviour stays available for collection editors covers the *why*;
a line of comment here pointing at that consumer would stop the next reader
assuming the ref advance is what protects the Settings tab, when it is the
functional updaters that do.
--
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]
