aminghadersohi commented on code in PR #45843:
URL: https://github.com/apache/superset/pull/45843#discussion_r4431712118
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -1522,13 +1540,14 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
if (dbFetched) {
setDB({
type: ActionType.Fetched,
+ isEditMode,
payload: dbFetched,
});
// keep a copy of the name separate for display purposes
// because it shouldn't change when the form is updated
setDbName(dbFetched.database_name);
}
- }, [dbFetched]);
+ }, [dbFetched, isEditMode]);
Review Comment:
I fixed the remaining late-write path in
c3fe8f85cd9d8f822458eb755260e77d7a6dc4e4.
My earlier reply was wrong to classify `DatabaseModal/index.tsx:1094` as not
affected: that save branch calls `updateResource`, and late POST/PUT
publication plus the save continuation can mutate a reopened modal. The
GET-only guard was insufficient.
POST/PUT resource and error publication use the same session-generation
predicate as reads. The `dbFetched` copy checks the publication generation too,
and obsolete save continuations cannot advance or close the next session. Hook
promises still return the create id/update result to callers; live-session
success behavior is retained.
Regression test: `late create response after X leaves the next connection as
a fresh POST`
Other regressions:
- `late update response after X leaves the next connection as a fresh POST`
- `useSingleViewResource: discarded %s preserves the active resource and
returns its response` (create/update)
- `useSingleViewResource: discarded %s errors do not update state or show a
toast` (create/update)
With the final tests and production sources from
eac7fed0f320b5c99547dab6c1b896af8bfc7329, all six fail. The modal cases fail at
`index.test.tsx:638`: the new session's database picker is missing. Hook cases
fail at `hooks.test.tsx:1079` (old resource replaces active resource) and
`:1119` (obsolete error published). After restoring the new sources, both modal
cases retain the picker and send a second POST without the first id, with zero
subsequent PUTs to database 77.
Validation:
- `cd superset-frontend && npx jest src/features/databases/DatabaseModal
src/pages/DatabaseList --runInBand`: 12 suites, 233 tests passed.
- `cd superset-frontend && npx jest src/views/CRUD/hooks.test.tsx
--runInBand`: 46 tests passed.
- `pre-commit run` and branch-changed-files pre-commit: exit 0, including
frontend type checking and branch-scoped mypy.
Siblings checked:
- superset-frontend/src/views/CRUD/hooks.ts:475 — fixed in
c3fe8f85cd9d8f822458eb755260e77d7a6dc4e4 (POST resource/error publication
honors the caller predicate; the returned id is unchanged).
- superset-frontend/src/views/CRUD/hooks.ts:526 — fixed in
c3fe8f85cd9d8f822458eb755260e77d7a6dc4e4 (PUT resource/error publication honors
the predicate; the returned result is unchanged).
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:983 —
fixed in c3fe8f85cd9d8f822458eb755260e77d7a6dc4e4 (validation, POST/PUT
publication, and save continuations share the requesting session's generation).
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1566 —
fixed in c3fe8f85cd9d8f822458eb755260e77d7a6dc4e4 (the only dbFetched-to-form
copy also checks the publication generation; dependencies remain
dbFetched-only).
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1196 —
fixed in c3fe8f85cd9d8f822458eb755260e77d7a6dc4e4 (initial edit, Back at 1365,
and CTA read at 1936 use the shared response predicate; CTA
response/local-storage navigation remains unchanged).
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:937 —
fixed in c3fe8f85cd9d8f822458eb755260e77d7a6dc4e4 (Close/X and successful close
invalidate the generation; Back-to-picker at 1374 and visibility/target cleanup
at 1562 invalidate it too).
- superset-frontend/src/pages/DatabaseList/index.tsx:1184 — not affected:
unchanged mounted caller; onHide clears selection/visibility at 559 and
delegates resource publication/copying to DatabaseModal; the regression harness
mirrors this lifecycle.
- superset-frontend/src/features/home/RightMenu.tsx:734 — not affected:
unchanged create-only caller, no separate resource-copy effect; its requests
are guarded inside DatabaseModal, including late creates.
- superset-frontend/src/features/cssTemplates/CssTemplateModal.tsx:205 — not
affected: its resource-copy effect does not choose PUT; onSave at 110 uses
prop-derived isEditMode at 90, not a copied database id.
-
superset-frontend/src/features/annotationLayers/AnnotationLayerModal.tsx:210 —
not affected: its resource-copy effect does not choose PUT; onSave at 133 uses
the layer prop's isEditMode at 101.
- superset-frontend/src/features/annotations/AnnotationModal.tsx:265 — not
affected: its resource-copy effect does not choose PUT; onSave at 141 uses the
annotation prop's isEditMode at 102; hide also copies resource only in edit
mode at 133.
- superset-frontend/src/features/themes/ThemeModal.tsx:483 — not affected:
resource and initial-theme copies do not choose PUT; onSave at 259 uses the
theme prop's isEditMode at 174.
- superset-frontend/src/features/rls/RowLevelSecurityModal.tsx:261 — not
affected: mapped resource copies do not choose PUT; onSave at 297 requires
prop-derived isEditMode at 214 as well as an id.
- superset-frontend/src/features/alerts/AlertReportModal.tsx:2262 — not
affected: resource-to-alert/notification copies do not choose PUT; onSave at
1236 uses the alert prop's isEditMode at 700.
- superset-frontend/src/pages/AlertReportList/index.tsx:302 — not affected:
update-only active-toggle caller, no resource-to-form copying; it retains the
existing four-argument update API and default predicate.
- superset-frontend/src/pages/ExecutionLogList/index.tsx:79 — not affected:
read-only report header consumer, no create/update or form-copy path.
- superset-frontend/src/features/datasets/AddDataset/Footer/index.tsx:66 —
not affected: create-only consumer uses loading and the returned id for
navigation at 103; it never copies state.resource into a form or calls update.
- superset-frontend/src/hooks/apiResources/apiResources.ts:106 — not
affected: grep's same-name fetchResource is a separate makeApi request, with
its own cancellation guard, not this hook's returned function.
The non-database form classifications concern this database-id-selected
create-to-PUT overwrite, not a blanket claim that every form is race-free.
Their prop-driven save mode and default hook behavior are unchanged. I also
checked the test/mock consumers in `hooks.test.tsx`,
`DatabaseModal/index.test.tsx`, `AlertReportModal.test.tsx`, and
`AddDataset/Footer/Footer.test.tsx`; none is an additional production
publication/copy path.
--
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]
