codeant-ai-for-open-source[bot] commented on code in PR #36754:
URL: https://github.com/apache/superset/pull/36754#discussion_r2633367970
##########
superset-frontend/src/components/DatabaseSelector/index.tsx:
##########
@@ -271,9 +266,41 @@ export function DatabaseSelector({
const schemaOptions = schemaData || EMPTY_SCHEMA_OPTIONS;
+ // Handle schema auto-selection when data changes
+ useEffect(() => {
+ if (!schemaData || loadingSchemas) return;
+
+ setErrorPayload(null);
+
+ if (schemaData.length === 1) {
+ changeSchema(schemaData[0]);
+ } else if (
+ !schemaData.find(schemaOption => schemaRef.current ===
schemaOption.value)
Review Comment:
**Suggestion:** The schema auto-selection effect checks whether the "current
selection" is present in the returned schema list using the `schema` prop ref
(`schemaRef.current`) rather than the actually selected `currentSchema`, so
when the user changes the schema without the parent feeding that value back via
the `schema` prop (uncontrolled usage), a refresh or data change can
incorrectly treat the user's selection as missing and clear or override it
(e.g., with the default schema). [logic error]
**Severity Level:** Minor ⚠️
```suggestion
!schemaData.find(
schemaOption => currentSchema?.value === schemaOption.value,
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR's new effect compares schemaRef.current (which tracks the incoming
prop `schema`) against the returned schema list. That means an uncontrolled
usage where the user picks a schema (stored in currentSchema) but the parent
doesn't update the `schema` prop will appear "missing" on refresh and get
cleared or replaced by defaults. Replacing the prop-ref check with a check
against currentSchema (the component's selected state) correctly represents the
user's actual selection and prevents accidental overrides. This fixes a real
logic bug, not a cosmetic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/DatabaseSelector/index.tsx
**Line:** 278:278
**Comment:**
*Logic Error: The schema auto-selection effect checks whether the
"current selection" is present in the returned schema list using the `schema`
prop ref (`schemaRef.current`) rather than the actually selected
`currentSchema`, so when the user changes the schema without the parent feeding
that value back via the `schema` prop (uncontrolled usage), a refresh or data
change can incorrectly treat the user's selection as missing and clear or
override it (e.g., with the default schema).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/components/DatabaseSelector/index.tsx:
##########
@@ -316,6 +331,49 @@ export function DatabaseSelector({
const catalogOptions = catalogData || EMPTY_CATALOG_OPTIONS;
+ // Handle catalog auto-selection when data changes
+ useEffect(() => {
+ if (loadingCatalogs) return;
+
+ setErrorPayload(null);
+
+ if (!showCatalogSelector) {
+ // Only clear catalog if it's not already null
+ if (currentCatalog !== null) {
+ setCurrentCatalog(null);
+ if (onCatalogChange && catalogRef.current != null) {
+ onCatalogChange(undefined);
+ }
+ }
+ } else if (catalogData && catalogData.length === 1) {
+ changeCatalog(catalogData[0]);
+ } else if (
+ catalogData &&
+ !catalogData.find(
+ catalogOption => catalogRef.current === catalogOption.value,
Review Comment:
**Suggestion:** The catalog auto-selection effect similarly uses the
`catalog` prop ref (`catalogRef.current`) to decide whether the "current
selection" exists in the catalog list, rather than the `currentCatalog` state,
so in uncontrolled usage where the parent does not keep `catalog` in sync, a
refresh or data change can incorrectly treat the selected catalog as missing
and clear or replace it (including overriding a user-picked catalog with the
default or `undefined`). [logic error]
**Severity Level:** Minor ⚠️
```suggestion
catalogOption => currentCatalog?.value === catalogOption.value,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The catalog effect currently uses catalogRef.current (the incoming prop) to
determine whether the current selection exists in the returned catalog list. As
with schemas, in uncontrolled usage the user's selection is kept in
currentCatalog state; using the prop ref causes spurious clears/replacements on
refresh. Using currentCatalog?.value here is the correct fix — it references
what the user actually selected and prevents accidental overriding with
defaults or undefined. This is a functional bugfix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/DatabaseSelector/index.tsx
**Line:** 353:353
**Comment:**
*Logic Error: The catalog auto-selection effect similarly uses the
`catalog` prop ref (`catalogRef.current`) to decide whether the "current
selection" exists in the catalog list, rather than the `currentCatalog` state,
so in uncontrolled usage where the parent does not keep `catalog` in sync, a
refresh or data change can incorrectly treat the selected catalog as missing
and clear or replace it (including overriding a user-picked catalog with the
default or `undefined`).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/hooks/apiResources/catalogs.test.ts:
##########
@@ -0,0 +1,204 @@
+/**
+ * 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.
+ */
+import rison from 'rison';
+import fetchMock from 'fetch-mock';
+import { act, renderHook } from '@testing-library/react-hooks';
+import {
+ createWrapper,
+ defaultStore as store,
+} from 'spec/helpers/testing-library';
+import { api } from 'src/hooks/apiResources/queryApi';
+import { useCatalogs } from './catalogs';
+
+const fakeApiResult = {
+ result: ['catalog_a', 'catalog_b'],
+ default: 'catalog_a',
+};
+const fakeApiResult2 = {
+ result: ['catalog_c', 'catalog_d'],
+ default: null,
+};
+
+const expectedResult = fakeApiResult.result.map((value: string) => ({
+ value,
+ label: value,
+ title: value,
+}));
+const expectedResult2 = fakeApiResult2.result.map((value: string) => ({
+ value,
+ label: value,
+ title: value,
+}));
+
+// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+describe('useCatalogs hook', () => {
+ beforeEach(() => {
+ fetchMock.reset();
+ store.dispatch(api.util.resetApiState());
+ });
+
+ test('returns api response mapping json result with default catalog', async
() => {
+ const expectDbId = 'db1';
+ const forceRefresh = false;
+ const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`;
+ fetchMock.get(catalogApiRoute, fakeApiResult);
+ const onSuccess = jest.fn();
+ const { result, waitFor } = renderHook(
+ () =>
+ useCatalogs({
+ dbId: expectDbId,
+ onSuccess,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+ await waitFor(() =>
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(1),
+ );
+ expect(result.current.data).toEqual(expectedResult);
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ expect(
+ fetchMock.calls(
+ `end:/api/v1/database/${expectDbId}/catalogs/?q=${rison.encode({
+ force: forceRefresh,
+ })}`,
+ ).length,
+ ).toBe(1);
+ expect(onSuccess).toHaveBeenCalledTimes(1);
+ act(() => {
+ result.current.refetch();
+ });
+ await waitFor(() =>
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(2),
+ );
+ expect(
+ fetchMock.calls(
+ `end:/api/v1/database/${expectDbId}/catalogs/?q=${rison.encode({
+ force: true,
+ })}`,
+ ).length,
+ ).toBe(1);
+ expect(onSuccess).toHaveBeenCalledTimes(2);
+ expect(result.current.data).toEqual(expectedResult);
+ expect(result.current.defaultCatalog).toBe('catalog_a');
Review Comment:
**Suggestion:** After calling `refetch`, the test again only waits for the
fetch call count to reach 2 before checking the URL, `onSuccess` call count,
and hook data/default, so these assertions can run before the refetch state
update has completed; moving all related expectations into a single `waitFor`
removes this async race and stabilizes the test. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
await waitFor(() => {
expect(fetchMock.calls(catalogApiRoute).length).toBe(2);
expect(
fetchMock.calls(
`end:/api/v1/database/${expectDbId}/catalogs/?q=${rison.encode({
force: true,
})}`,
).length,
).toBe(1);
expect(onSuccess).toHaveBeenCalledTimes(2);
expect(result.current.data).toEqual(expectedResult);
expect(result.current.defaultCatalog).toBe('catalog_a');
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
After triggering refetch, the test again waits only on the network call
count. The hook's internal state (data, defaultCatalog) and the onSuccess
handler invocation may not be fully observed at that exact moment. Grouping
these expectations inside a single waitFor ensures the assertions run only
after the hook finishes updating, reducing intermittent failures. This is a
valid stability improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/catalogs.test.ts
**Line:** 91:103
**Comment:**
*Race Condition: After calling `refetch`, the test again only waits for
the fetch call count to reach 2 before checking the URL, `onSuccess` call
count, and hook data/default, so these assertions can run before the refetch
state update has completed; moving all related expectations into a single
`waitFor` removes this async race and stabilizes the test.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/hooks/apiResources/tables.ts:
##########
@@ -167,7 +167,7 @@ export const {
export function useTables(options: Params) {
const { dbId, catalog, schema, onSuccess, onError } = options || {};
const isMountedRef = useRef(false);
- const { currentData: schemaOptions, isFetching } = useSchemas({
+ const { data: schemaOptions, isFetching } = useSchemas({
Review Comment:
**Suggestion:** When `useSchemas` has not yet loaded data, `schemaOptions`
is `undefined`, so `schemaOptions?.map(...)` evaluates to `undefined` and `new
Set(undefined)` will throw a runtime `TypeError` because `undefined` is not
iterable; defaulting `schemaOptions` to an empty array ensures the `Set`
constructor always receives a valid iterable. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const { data: schemaOptions = [], isFetching } = useSchemas({
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
schemaOptions is used immediately after in:
const schemaOptionsMap = useMemo(
() => new Set(schemaOptions?.map(({ value }) => value)),
[schemaOptions],
);
If schemaOptions is undefined while useSchemas is still loading,
schemaOptions?.map(...) returns undefined and new Set(undefined) will throw
"TypeError: undefined is not iterable". Defaulting schemaOptions to an empty
array (data: schemaOptions = []) guarantees the Set constructor always receives
a valid iterable and avoids a real runtime crash. This is a real bug fix, not
just cosmetic.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/tables.ts
**Line:** 170:170
**Comment:**
*Logic Error: When `useSchemas` has not yet loaded data,
`schemaOptions` is `undefined`, so `schemaOptions?.map(...)` evaluates to
`undefined` and `new Set(undefined)` will throw a runtime `TypeError` because
`undefined` is not iterable; defaulting `schemaOptions` to an empty array
ensures the `Set` constructor always receives a valid iterable.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/hooks/apiResources/catalogs.test.ts:
##########
@@ -0,0 +1,204 @@
+/**
+ * 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.
+ */
+import rison from 'rison';
+import fetchMock from 'fetch-mock';
+import { act, renderHook } from '@testing-library/react-hooks';
+import {
+ createWrapper,
+ defaultStore as store,
+} from 'spec/helpers/testing-library';
+import { api } from 'src/hooks/apiResources/queryApi';
+import { useCatalogs } from './catalogs';
+
+const fakeApiResult = {
+ result: ['catalog_a', 'catalog_b'],
+ default: 'catalog_a',
+};
+const fakeApiResult2 = {
+ result: ['catalog_c', 'catalog_d'],
+ default: null,
+};
+
+const expectedResult = fakeApiResult.result.map((value: string) => ({
+ value,
+ label: value,
+ title: value,
+}));
+const expectedResult2 = fakeApiResult2.result.map((value: string) => ({
+ value,
+ label: value,
+ title: value,
+}));
+
+// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+describe('useCatalogs hook', () => {
+ beforeEach(() => {
+ fetchMock.reset();
+ store.dispatch(api.util.resetApiState());
+ });
+
+ test('returns api response mapping json result with default catalog', async
() => {
+ const expectDbId = 'db1';
+ const forceRefresh = false;
+ const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`;
+ fetchMock.get(catalogApiRoute, fakeApiResult);
+ const onSuccess = jest.fn();
+ const { result, waitFor } = renderHook(
+ () =>
+ useCatalogs({
+ dbId: expectDbId,
+ onSuccess,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+ await waitFor(() =>
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(1),
+ );
+ expect(result.current.data).toEqual(expectedResult);
+ expect(result.current.defaultCatalog).toBe('catalog_a');
Review Comment:
**Suggestion:** The first test waits only for the fetch call count before
asserting on `result.current.data` and `result.current.defaultCatalog`, so
there's a race condition where the network request is recorded but the hook
state hasn't updated yet, leading to flaky or intermittently failing
assertions; wrapping all these expectations inside the same `waitFor` ensures
they only run after React state has settled. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
await waitFor(() => {
expect(fetchMock.calls(catalogApiRoute).length).toBe(1);
expect(result.current.data).toEqual(expectedResult);
expect(result.current.defaultCatalog).toBe('catalog_a');
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion correctly points out a real race: the test waits only for the
mock network call to be recorded but then immediately asserts on hook state,
which may not have updated yet. Combining the fetch-count check and the state
assertions inside the same waitFor ensures React state updates have settled
before making assertions and reduces flakiness. This is a behavioral test
stabilization, not a stylistic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/catalogs.test.ts
**Line:** 75:79
**Comment:**
*Race Condition: The first test waits only for the fetch call count
before asserting on `result.current.data` and `result.current.defaultCatalog`,
so there's a race condition where the network request is recorded but the hook
state hasn't updated yet, leading to flaky or intermittently failing
assertions; wrapping all these expectations inside the same `waitFor` ensures
they only run after React state has settled.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/databases/api.py:
##########
@@ -795,23 +832,30 @@ def schemas(self, pk: int, **kwargs: Any) ->
FlaskResponse:
catalog,
schemas,
)
+ default_schema = self._get_default_schema(database, catalog,
schemas, pk)
+
if params.get("upload_allowed"):
if not database.allow_file_upload:
- return self.response(200, result=[])
+ return self.response(200, result=[], default=None)
if allowed_schemas :=
database.get_schema_access_for_file_upload():
# some databases might return the list of schemas in
uppercase,
# while the list of allowed schemas is manually inputted so
# could be lowercase
allowed_schemas = {schema.lower() for schema in
allowed_schemas}
+ filtered_schemas = [
+ schema
+ for schema in schemas
+ if schema.lower() in allowed_schemas
+ ]
+ # Check if default is in filtered list
+ if default_schema and default_schema.lower() not in
allowed_schemas:
+ default_schema = None
return self.response(
200,
- result=[
- schema
- for schema in schemas
- if schema.lower() in allowed_schemas
- ],
+ result=filtered_schemas,
+ default=default_schema,
)
Review Comment:
**Suggestion:** When the `upload_allowed` filter is requested but the
database is configured for file upload with no schemas explicitly allowed
(empty or falsy `get_schema_access_for_file_upload()`), the endpoint silently
ignores `upload_allowed` and returns all accessible schemas with a default,
which misrepresents which schemas actually accept uploads and can confuse the
UI. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
# No schemas are configured for upload; honor the
upload_allowed filter
return self.response(200, result=[], default=None)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current flow silently falls through to returning all accessible schemas
when upload_allowed is requested but get_schema_access_for_file_upload()
returns empty/false. That misrepresents which schemas accept uploads. Returning
an empty list (and None default) when no upload-allowed schemas are configured
is the correct, least-surprising behavior for an "upload_allowed" filter and
fixes a real logic/UX bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/databases/api.py
**Line:** 855:857
**Comment:**
*Logic Error: When the `upload_allowed` filter is requested but the
database is configured for file upload with no schemas explicitly allowed
(empty or falsy `get_schema_access_for_file_upload()`), the endpoint silently
ignores `upload_allowed` and returns all accessible schemas with a default,
which misrepresents which schemas actually accept uploads and can confuse the
UI.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/databases/api.py:
##########
@@ -317,6 +317,32 @@ class DatabaseRestApi(BaseSupersetModelRestApi):
"changed_by": [["id", BaseFilterRelatedUsers, lambda: []]],
}
+ @staticmethod
+ def _get_default_schema(
+ database: Database,
+ catalog: str | None,
+ accessible_schemas: set[str],
+ pk: int,
+ ) -> str | None:
+ """
+ Get the default schema for a database/catalog, with error handling.
+
+ Returns None if the default cannot be determined or is not accessible.
+ """
+ try:
+ default_schema = database.get_default_schema(catalog)
Review Comment:
**Suggestion:** The helper for computing the default schema ignores the
database's configured default catalog when the `catalog` argument is `None`,
which can cause the returned default schema to be inconsistent with the schema
set used by the security manager and therefore point to the wrong schema on
engines where the default schema depends on the default catalog. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
# Normalize to the database default catalog when none is
provided,
# to keep behavior consistent with permission checks.
effective_catalog = catalog or database.get_default_catalog()
default_schema = database.get_default_schema(effective_catalog)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Normalizing an absent catalog to the database's default catalog before
asking for the default schema is a defensible behavioral change and prevents
inconsistencies between permission checks and the computed default schema. The
existing implementation can return a schema computed against "no catalog" while
downstream checks use schemas filtered by catalog-aware logic, which can cause
the default to be omitted or be incorrect. The proposed change is a real logic
improvement (not just stylistic) that aligns behavior and reduces surprising UI
behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/databases/api.py
**Line:** 333:333
**Comment:**
*Logic Error: The helper for computing the default schema ignores the
database's configured default catalog when the `catalog` argument is `None`,
which can cause the returned default schema to be inconsistent with the schema
set used by the security manager and therefore point to the wrong schema on
engines where the default schema depends on the default catalog.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/hooks/apiResources/catalogs.test.ts:
##########
@@ -0,0 +1,204 @@
+/**
+ * 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.
+ */
+import rison from 'rison';
+import fetchMock from 'fetch-mock';
+import { act, renderHook } from '@testing-library/react-hooks';
+import {
+ createWrapper,
+ defaultStore as store,
+} from 'spec/helpers/testing-library';
+import { api } from 'src/hooks/apiResources/queryApi';
+import { useCatalogs } from './catalogs';
+
+const fakeApiResult = {
+ result: ['catalog_a', 'catalog_b'],
+ default: 'catalog_a',
+};
+const fakeApiResult2 = {
+ result: ['catalog_c', 'catalog_d'],
+ default: null,
+};
+
+const expectedResult = fakeApiResult.result.map((value: string) => ({
+ value,
+ label: value,
+ title: value,
+}));
+const expectedResult2 = fakeApiResult2.result.map((value: string) => ({
+ value,
+ label: value,
+ title: value,
+}));
+
+// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+describe('useCatalogs hook', () => {
+ beforeEach(() => {
+ fetchMock.reset();
+ store.dispatch(api.util.resetApiState());
+ });
+
+ test('returns api response mapping json result with default catalog', async
() => {
+ const expectDbId = 'db1';
+ const forceRefresh = false;
+ const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`;
+ fetchMock.get(catalogApiRoute, fakeApiResult);
+ const onSuccess = jest.fn();
+ const { result, waitFor } = renderHook(
+ () =>
+ useCatalogs({
+ dbId: expectDbId,
+ onSuccess,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+ await waitFor(() =>
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(1),
+ );
+ expect(result.current.data).toEqual(expectedResult);
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ expect(
+ fetchMock.calls(
+ `end:/api/v1/database/${expectDbId}/catalogs/?q=${rison.encode({
+ force: forceRefresh,
+ })}`,
+ ).length,
+ ).toBe(1);
+ expect(onSuccess).toHaveBeenCalledTimes(1);
+ act(() => {
+ result.current.refetch();
+ });
+ await waitFor(() =>
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(2),
+ );
+ expect(
+ fetchMock.calls(
+ `end:/api/v1/database/${expectDbId}/catalogs/?q=${rison.encode({
+ force: true,
+ })}`,
+ ).length,
+ ).toBe(1);
+ expect(onSuccess).toHaveBeenCalledTimes(2);
+ expect(result.current.data).toEqual(expectedResult);
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ });
+
+ test('returns cached data without api request', async () => {
+ const expectDbId = 'db1';
+ const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`;
+ fetchMock.get(catalogApiRoute, fakeApiResult);
+ const { result, rerender, waitFor } = renderHook(
+ () =>
+ useCatalogs({
+ dbId: expectDbId,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+ await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(1);
+ rerender();
+ await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(1);
+ });
+
+ test('returns refreshed data after switching databases', async () => {
+ const expectDbId = 'db1';
+ const catalogApiRoute = `glob:*/api/v1/database/*/catalogs/*`;
+ fetchMock.get(catalogApiRoute, url =>
+ url.includes(expectDbId) ? fakeApiResult : fakeApiResult2,
+ );
+ const onSuccess = jest.fn();
+ const { result, rerender, waitFor } = renderHook(
+ ({ dbId }) =>
+ useCatalogs({
+ dbId,
+ onSuccess,
+ }),
+ {
+ initialProps: { dbId: expectDbId },
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+
+ await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(1);
+ expect(onSuccess).toHaveBeenCalledTimes(1);
+
+ rerender({ dbId: 'db2' });
+ await waitFor(() => expect(result.current.data).toEqual(expectedResult2));
+ expect(result.current.defaultCatalog).toBeNull();
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(2);
+ expect(onSuccess).toHaveBeenCalledTimes(2);
+
+ rerender({ dbId: expectDbId });
+ await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(2);
+ expect(onSuccess).toHaveBeenCalledTimes(2);
+
+ // clean up cache
+ act(() => {
+ store.dispatch(api.util.invalidateTags(['Catalogs']));
+ });
+
+ await waitFor(() =>
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(4),
+ );
+ expect(fetchMock.calls(catalogApiRoute)[2][0]).toContain(expectDbId);
+ await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+ expect(result.current.defaultCatalog).toBe('catalog_a');
+ });
+
+ test('returns null defaultCatalog when API response has no default', async
() => {
+ const expectDbId = 'db-no-default';
+ const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`;
+ fetchMock.get(catalogApiRoute, { result: ['catalog1', 'catalog2'] });
+ const { result, waitFor } = renderHook(
+ () =>
+ useCatalogs({
+ dbId: expectDbId,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+ await waitFor(() =>
+ expect(fetchMock.calls(catalogApiRoute).length).toBe(1),
+ );
+ expect(result.current.defaultCatalog).toBeNull();
Review Comment:
**Suggestion:** The "no default" test currently only waits for the fetch
call count before asserting `defaultCatalog` is null, so it can pass even if
the hook never updates its data (because the `?? null` fallback yields null),
masking real bugs; combining the fetch count and `defaultCatalog` assertion
inside `waitFor` guarantees the state has been updated and that the hook truly
returns a null default from the API response. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
await waitFor(() => {
expect(fetchMock.calls(catalogApiRoute).length).toBe(1);
expect(result.current.defaultCatalog).toBeNull();
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The test validates that defaultCatalog is null but only waits for the fetch
call to be recorded. If the hook's state hasn't updated (or a fallback produces
null), the assertion might be a false positive. Moving the defaultCatalog
assertion into the same waitFor ensures the hook actually produced the null
default as a result of the API response, making the test meaningful and less
flaky.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/catalogs.test.ts
**Line:** 199:202
**Comment:**
*Race Condition: The "no default" test currently only waits for the
fetch call count before asserting `defaultCatalog` is null, so it can pass even
if the hook never updates its data (because the `?? null` fallback yields
null), masking real bugs; combining the fetch count and `defaultCatalog`
assertion inside `waitFor` guarantees the state has been updated and that the
hook truly returns a null default from the API response.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/hooks/apiResources/schemas.test.ts:
##########
@@ -208,14 +209,37 @@ describe('useSchemas hook', () => {
await waitFor(() =>
expect(fetchMock.calls(schemaApiRoute).length).toBe(1));
expect(result.current.data).toEqual(expectedResult3);
+ expect(result.current.defaultSchema).toBe('test schema c');
expect(onSuccess).toHaveBeenCalledTimes(1);
rerender({ dbId, catalog: 'catalog2' });
await waitFor(() =>
expect(fetchMock.calls(schemaApiRoute).length).toBe(2));
expect(result.current.data).toEqual(expectedResult2);
+ expect(result.current.defaultSchema).toBeNull();
rerender({ dbId, catalog: expectCatalog });
expect(result.current.data).toEqual(expectedResult3);
+ expect(result.current.defaultSchema).toBe('test schema c');
expect(fetchMock.calls(schemaApiRoute).length).toBe(2);
});
+
+ test('returns null defaultSchema when API response has no default', async ()
=> {
+ const expectDbId = 'db-no-default';
+ const schemaApiRoute = `glob:*/api/v1/database/${expectDbId}/schemas/*`;
+ fetchMock.get(schemaApiRoute, { result: ['schema1', 'schema2'] });
+ const { result, waitFor } = renderHook(
+ () =>
+ useSchemas({
+ dbId: expectDbId,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+ await waitFor(() =>
expect(fetchMock.calls(schemaApiRoute).length).toBe(1));
+ expect(result.current.defaultSchema).toBeNull();
Review Comment:
**Suggestion:** In the "returns null defaultSchema when API response has no
default" test, you only wait for the fetchMock call count to reach 1 before
asserting on `defaultSchema`, which can run before the RTK Query state has
finished updating, making the test flaky; combining the call-count and
`defaultSchema` checks inside the same `waitFor` ensures the assertion only
runs once the hook state is settled. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
await waitFor(() => {
expect(fetchMock.calls(schemaApiRoute).length).toBe(1);
expect(result.current.defaultSchema).toBeNull();
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current test only waits for the network call count, then immediately
asserts on result.current.defaultSchema.
RTK Query / the hook may still be updating internal state after the fetch
completes, so separating the two assertions can cause a race and intermittent
failures.
Combining both expectations inside the same waitFor ensures the test waits
until the hook's state reflects the response before asserting, which is a
legitimate, non-cosmetic stability fix for a flaky test.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/schemas.test.ts
**Line:** 242:243
**Comment:**
*Logic Error: In the "returns null defaultSchema when API response has
no default" test, you only wait for the fetchMock call count to reach 1 before
asserting on `defaultSchema`, which can run before the RTK Query state has
finished updating, making the test flaky; combining the call-count and
`defaultSchema` checks inside the same `waitFor` ensures the assertion only
runs once the hook state is settled.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
