rebenitez1802 commented on code in PR #45843:
URL: https://github.com/apache/superset/pull/45843#discussion_r4442957813
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -1324,13 +1364,14 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
const handleBackButtonOnFinish = () => {
if (dbFetched) {
- fetchResource(dbFetched.id as number);
+ fetchDatabase(dbFetched.id as number);
}
setShowCTAbtns(false);
setEditNewDb(true);
};
const handleBackButtonOnConnect = () => {
+ databaseRequestGeneration.current += 1;
Review Comment:
🟡 Medium — Back during a pending save leaves the modal stuck on a spinner
`onSave` returns early on `!shouldApplyResponse()` without calling
`setLoading(false)`, and this bump makes that happen while the modal stays
open. Back isn't disabled during a save. If it's clicked during the POST, or
during save-time `validate_parameters` for dynamic forms, the body stays hidden
behind `!isLoading`, `db` is null so the footer is empty, and only the
title-bar X recovers. On master and at 066d5561e2, a pending POST lands on step
3 instead. (During validation, master POSTs the abandoned connection, which
HEAD correctly prevents.)
```suggestion
databaseRequestGeneration.current += 1;
// A save abandoned by Back cannot reach its own setLoading(false).
setLoading(false);
```
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -1075,7 +1109,10 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
db.id as number,
dbToUpdate as DatabaseObject,
dbToUpdate.configuration_method === ConfigurationMethod.DynamicForm,
// onShow toast on SQLA Forms
+ true,
+ shouldApplyResponse,
);
+ if (!shouldApplyResponse()) return;
if (result) {
if (onDatabaseAdd) onDatabaseAdd();
Review Comment:
🟡 Medium — A save that finishes after the modal closes no longer refreshes
the database list
Click Connect or Finish, then X or Close while the request is still pending
(create and update run a server-side connection test, so this can take
seconds). The server still writes the connection, but `onDatabaseAdd()`
(`refreshData()` in DatabaseList, `setQuery({ databaseAdded: true })` in
RightMenu) now sits behind the session guard. The list keeps stale data until a
reload. On master and at 066d5561e2 the list refreshes. The parent refresh
reflects a real server write, so it should run regardless of session. Only the
modal state changes need gating.
```suggestion
// The server write happened even if this session ended; refresh the
// parent list, but do not advance or close a later session.
if (result && onDatabaseAdd) onDatabaseAdd();
if (!shouldApplyResponse()) return;
if (result) {
```
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -1100,7 +1137,9 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
const dbId = await createResource(
dbToUpdate as DatabaseObject,
dbToUpdate.configuration_method === ConfigurationMethod.DynamicForm,
// onShow toast on SQLA Forms
+ shouldApplyResponse,
);
+ if (!shouldApplyResponse()) return;
if (dbId) {
setHasConnectedDb(true);
if (onDatabaseAdd) onDatabaseAdd();
Review Comment:
🟡 Medium — Same fix for the create branch
Same issue for `createResource`: after X during a pending POST, the database
is created but the parent list is never refreshed.
```suggestion
if (dbId && onDatabaseAdd) onDatabaseAdd();
if (!shouldApplyResponse()) return;
if (dbId) {
setHasConnectedDb(true);
```
##########
superset-frontend/src/features/databases/DatabaseModal/index.test.tsx:
##########
@@ -336,6 +340,385 @@ describe('DatabaseModal', () => {
useRedux: true,
});
+ /** Check that closing a mounted modal cannot reuse a saved connection id. */
+ const expectFreshCreateAfterEdit = async (
+ closeAction: 'Close' | 'X' | 'Finish' | 'Late X',
+ ): Promise<void> => {
+ const resourceHook = jest.spyOn(hooks, 'useSingleViewResource');
+ const previousImplementation = resourceHook.getMockImplementation()!;
+ resourceHook.mockImplementation(realUseSingleViewResource);
+ let releaseEditFetch: () => void = () => {};
+ const editFetchReady = new Promise<void>(resolve => {
+ releaseEditFetch = resolve;
+ });
+ const editResponse = {
+ result: {
+ id: 10,
+ database_name: 'Existing connection',
+ backend: 'sqlite',
+ configuration_method: ConfigurationMethod.SqlalchemyUri,
+ sqlalchemy_uri: 'sqlite:///existing.db',
+ },
+ };
+ fetchMock.get(
+ 'glob:*/api/v1/database/10/connection',
+ closeAction === 'Late X'
+ ? async () => {
+ await editFetchReady;
+ return editResponse;
+ }
+ : editResponse,
+ );
+ fetchMock.put(DATABASE_FETCH_ENDPOINT, {
+ id: 10,
+ result: {
+ database_name: 'Existing connection',
+ backend: 'sqlite',
+ configuration_method: ConfigurationMethod.SqlalchemyUri,
+ sqlalchemy_uri: 'sqlite:///existing.db',
+ },
+ });
+ fetchMock.post('glob:*/api/v1/database/test_connection/', {
+ message: 'OK',
+ });
+ const Harness = () => {
+ const [show, setShow] = useState(true);
+ const [databaseId, setDatabaseId] = useState<number | undefined>(10);
+ return (
+ <>
+ <button type="button" onClick={() => setShow(true)}>
+ + Database
+ </button>
+ <button
+ type="button"
+ onClick={() => {
+ setDatabaseId(10);
+ setShow(true);
+ }}
+ >
+ Edit existing
+ </button>
+ <DatabaseModal
+ show={show}
+ databaseId={databaseId}
+ onHide={() => {
+ setShow(false);
+ setDatabaseId(undefined);
+ }}
+ />
+ </>
+ );
+ };
+ try {
+ render(<Harness />, { useRedux: true });
+ if (closeAction === 'Late X') {
+ await waitFor(() =>
+ expect(
+
fetchMock.callHistory.calls('glob:*/api/v1/database/10/connection'),
+ ).toHaveLength(1),
+ );
+ } else {
+ expect(
+ await screen.findByDisplayValue('Existing connection'),
+ ).toBeInTheDocument();
+ }
+ await userEvent.click(document.querySelector('.ant-modal-wrap')!);
+ expect(screen.getByRole('dialog')).toBeInTheDocument();
+ if (closeAction === 'Finish') {
+ await userEvent.click(screen.getByRole('button', { name: 'Finish' }));
+ } else if (closeAction === 'X' || closeAction === 'Late X') {
+ await userEvent.click(screen.getAllByLabelText('Close')[0]);
+ } else {
+ await userEvent.click(screen.getByText('Close'));
+ }
+ await waitFor(() =>
+ expect(screen.queryByRole('dialog')).not.toBeInTheDocument(),
+ );
+ if (closeAction !== 'Late X') fetchMock.clearHistory();
+ await userEvent.click(screen.getByRole('button', { name: '+ Database'
}));
+ await screen.findByRole('dialog');
+ if (closeAction === 'Late X') {
+ await act(async () => {
+ releaseEditFetch();
+ await fetchMock.callHistory.flush(true);
+ });
+ }
+ const picker = screen.queryByRole('heading', {
+ name: /select a database to connect/i,
+ });
+ // Exercise saving even if stale state incorrectly skips the picker.
+ if (picker) {
+ await userEvent.click(
+ await screen.findByRole('button', { name: /sqlite/i }),
+ );
+ }
+ fireEvent.change(await screen.findByTestId('database-name-input'), {
+ target: { value: 'My new sandbox' },
+ });
+ fireEvent.change(screen.getByTestId('sqlalchemy-uri-input'), {
+ target: { value: 'sqlite:///sandbox.db' },
+ });
+ await userEvent.click(
+ screen.getByRole('button', { name: /test connection/i }),
+ );
+ const connect = screen.getByRole('button', { name: 'Connect' });
+ await waitFor(() => expect(connect).toBeEnabled());
+ await userEvent.click(connect);
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls(DATABASE_CONNECT_ENDPOINT).length +
+ fetchMock.callHistory.calls(DATABASE_FETCH_ENDPOINT, {
+ method: 'PUT',
+ }).length,
+ ).toBeGreaterThan(0),
+ );
+ expect(
+ fetchMock.callHistory.calls(DATABASE_FETCH_ENDPOINT, {
+ method: 'PUT',
+ }),
+ ).toHaveLength(0);
+ expect(picker).not.toBeNull();
+ expect(
+ fetchMock.callHistory.calls(DATABASE_CONNECT_ENDPOINT),
+ ).toHaveLength(1);
+ expect(
+ JSON.parse(
+ fetchMock.callHistory.lastCall(DATABASE_CONNECT_ENDPOINT)!.options
+ .body as string,
+ ),
+ ).not.toHaveProperty('id');
+ await waitFor(() =>
+ expect(screen.queryByRole('dialog')).not.toBeInTheDocument(),
+ );
+ // A successful create also updates the resource cache before closing.
+ await userEvent.click(screen.getByRole('button', { name: '+ Database'
}));
+ expect(
+ await screen.findByRole('heading', {
+ name: /select a database to connect/i,
+ }),
+ ).toBeInTheDocument();
+ await userEvent.click(screen.getAllByLabelText('Close')[0]);
+ await waitFor(() =>
+ expect(screen.queryByRole('dialog')).not.toBeInTheDocument(),
+ );
+ await userEvent.click(
+ screen.getByRole('button', { name: 'Edit existing' }),
+ );
+ expect(
+ await screen.findByDisplayValue('Existing connection'),
+ ).toBeInTheDocument();
+ } finally {
+ releaseEditFetch();
+ resourceHook.mockImplementation(previousImplementation);
+ }
+ };
+
+ test('closing edit then creating uses POST without the old id', async () => {
+ await expectFreshCreateAfterEdit('Close');
+ });
+
+ test('X edit then creating uses POST without the old id', async () => {
+ await expectFreshCreateAfterEdit('X');
+ });
+
+ test('saving edit then creating uses POST without the old id', async () => {
+ await expectFreshCreateAfterEdit('Finish');
+ });
+
+ test('late edit fetch after X then creating uses POST without the old id',
async () => {
+ await expectFreshCreateAfterEdit('Late X');
+ });
+
+ /** Check that late writes cannot populate or close a subsequent create
session. */
+ const expectFreshCreateAfterLateWrite = async (
+ operation: 'create' | 'update',
+ ): Promise<void> => {
+ const resourceHook = jest.spyOn(hooks, 'useSingleViewResource');
+ const previousImplementation = resourceHook.getMockImplementation()!;
+ resourceHook.mockImplementation(realUseSingleViewResource);
+ let releaseWrite: () => void = () => {};
+ const writeReady = new Promise<void>(resolve => {
+ releaseWrite = resolve;
+ });
+ const connection = {
+ id: 77,
+ database_name: 'First connection',
+ backend: 'postgresql',
+ configuration_method: ConfigurationMethod.DynamicForm,
+ };
+ let createCount = 0;
+ fetchMock.modifyRoute('database-connect', {
+ response: async () => {
+ createCount += 1;
+ if (operation === 'create' && createCount === 1) await writeReady;
+ const id = createCount === 1 ? 77 : 88;
+ return { id, result: { ...connection, id } };
+ },
+ });
+ fetchMock.put('glob:*/api/v1/database/77', async () => {
+ await writeReady;
+ return { id: 77, result: connection };
+ });
+ const onDatabaseAdd = jest.fn();
+ const Harness = () => {
+ const [show, setShow] = useState(true);
+ return (
+ <>
+ <button type="button" onClick={() => setShow(true)}>
+ + Database
+ </button>
+ <DatabaseModal
+ show={show}
+ onHide={() => setShow(false)}
+ onDatabaseAdd={onDatabaseAdd}
+ />
+ </>
+ );
+ };
+ /** Fill a new dynamic-form connection without relying on saved state. */
+ const connectPostgres = async (name: string, host: string) => {
+ await userEvent.click(
+ await screen.findByRole('button', { name: /postgresql/i }),
+ );
+ fireEvent.change(
+ document.querySelector<HTMLInputElement>(
+ 'input[name="database_name"]',
+ )!,
+ {
+ target: { value: name },
+ },
+ );
+ for (const [field, value] of Object.entries({
+ host,
+ port: '5432',
+ database: 'postgres',
+ username: 'testdb',
+ })) {
+ fireEvent.change(
+ document.querySelector<HTMLInputElement>(`input[name="${field}"]`)!,
+ { target: { value } },
+ );
+ }
+ fireEvent.blur(
+ document.querySelector<HTMLInputElement>('input[name="username"]')!,
+ );
+ const connect = screen.getByRole('button', { name: 'Connect' });
+ await waitFor(() => expect(connect).toBeEnabled());
+ await userEvent.click(connect);
+ };
+ try {
+ render(<Harness />, { useRedux: true });
+ await connectPostgres('First connection', 'first.example');
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls(DATABASE_CONNECT_ENDPOINT),
+ ).toHaveLength(1),
+ );
+ if (operation === 'update') {
+ expect(await screen.findByText(/step 3 of 3/i)).toBeInTheDocument();
+ await userEvent.click(screen.getByRole('button', { name: 'Finish' }));
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls('glob:*/api/v1/database/77'),
+ ).toHaveLength(1),
+ );
+ }
+ await userEvent.click(screen.getAllByLabelText('Close')[0]);
+ await waitFor(() =>
+ expect(screen.queryByRole('dialog')).not.toBeInTheDocument(),
+ );
+ await userEvent.click(screen.getByRole('button', { name: '+ Database'
}));
+ await screen.findByRole('dialog');
+ onDatabaseAdd.mockClear();
+ await act(async () => {
+ releaseWrite();
+ await fetchMock.callHistory.flush(true);
+ });
+ expect(
+ screen.getByRole('heading', {
+ name: /select a database to connect/i,
+ }),
+ ).toBeInTheDocument();
+ expect(onDatabaseAdd).not.toHaveBeenCalled();
Review Comment:
🟡 Medium — This assertion pins the missing refresh
With the two `onDatabaseAdd` moves above, the late create and update
responses refresh the parent exactly once.
```suggestion
expect(onDatabaseAdd).toHaveBeenCalledTimes(1);
```
--
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]
