Skip to content

Commit 1699185

Browse files
sadpandajoeclaude
andcommitted
fix(datasets): prevent infinite loop and add missing guards
- Add break statement in pagination loop error handler to prevent infinite retries on persistent API errors - Add db?.id guard to prevent API calls with undefined database ID - Add 5 new tests covering extension fallback, db undefined, and pagination error scenarios - Update existing error tests to expect single API call (not retry) Test Results: 35/35 passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent f8d38ce commit 1699185

File tree

3 files changed

+118
-18
lines changed

3 files changed

+118
-18
lines changed

superset-frontend/src/features/datasets/hooks/useDatasetLists.test.ts

Lines changed: 85 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -121,16 +121,10 @@ test('useDatasetsList extracts dataset names correctly', async () => {
121121
});
122122

123123
test('useDatasetsList handles API 500 error gracefully', async () => {
124-
// Mock error on first call, then return empty result to break the loop
124+
// Mock error - loop should break immediately
125125
const getSpy = jest
126126
.spyOn(SupersetClient, 'get')
127-
.mockRejectedValueOnce(new Error('Internal Server Error'))
128-
.mockResolvedValueOnce({
129-
json: {
130-
count: 0,
131-
result: [],
132-
},
133-
} as any);
127+
.mockRejectedValue(new Error('Internal Server Error'));
134128

135129
const { result, waitForNextUpdate } = renderHook(() =>
136130
useDatasetsList(mockDb, 'public'),
@@ -143,8 +137,8 @@ test('useDatasetsList handles API 500 error gracefully', async () => {
143137
expect(mockAddDangerToast).toHaveBeenCalledWith(
144138
'There was an error fetching dataset',
145139
);
146-
// Should be called twice - once for error, once to complete
147-
expect(getSpy).toHaveBeenCalledTimes(2);
140+
// Should only be called once - error causes break
141+
expect(getSpy).toHaveBeenCalledTimes(1);
148142
});
149143

150144
test('useDatasetsList handles empty dataset response', async () => {
@@ -248,26 +242,72 @@ test('useDatasetsList handles network timeout gracefully', async () => {
248242

249243
const getSpy = jest
250244
.spyOn(SupersetClient, 'get')
251-
.mockRejectedValueOnce(timeoutError)
252-
.mockResolvedValueOnce({
253-
json: {
254-
count: 0,
255-
result: [],
256-
},
257-
} as any);
245+
.mockRejectedValue(timeoutError);
246+
247+
const { result, waitForNextUpdate } = renderHook(() =>
248+
useDatasetsList(mockDb, 'public'),
249+
);
250+
251+
await waitForNextUpdate();
252+
253+
expect(result.current.datasets).toEqual([]);
254+
expect(result.current.datasetNames).toEqual([]);
255+
expect(mockAddDangerToast).toHaveBeenCalledWith(
256+
'There was an error fetching dataset',
257+
);
258+
// Should only be called once - error causes break
259+
expect(getSpy).toHaveBeenCalledTimes(1);
260+
});
261+
262+
test('useDatasetsList breaks pagination loop on persistent API errors', async () => {
263+
// Mock API that always fails (persistent error)
264+
const getSpy = jest
265+
.spyOn(SupersetClient, 'get')
266+
.mockRejectedValue(new Error('Persistent server error'));
258267

259268
const { result, waitForNextUpdate } = renderHook(() =>
260269
useDatasetsList(mockDb, 'public'),
261270
);
262271

263272
await waitForNextUpdate();
264273

274+
// Should only attempt once, then break (not infinite loop)
275+
expect(getSpy).toHaveBeenCalledTimes(1);
265276
expect(result.current.datasets).toEqual([]);
266277
expect(result.current.datasetNames).toEqual([]);
267278
expect(mockAddDangerToast).toHaveBeenCalledWith(
268279
'There was an error fetching dataset',
269280
);
281+
expect(mockAddDangerToast).toHaveBeenCalledTimes(1);
282+
});
283+
284+
test('useDatasetsList handles error on second page gracefully', async () => {
285+
// First page succeeds, second page fails
286+
const getSpy = jest
287+
.spyOn(SupersetClient, 'get')
288+
.mockResolvedValueOnce({
289+
json: {
290+
count: 3, // Indicates more data exists
291+
result: [{ id: 1, table_name: 'table1' }],
292+
},
293+
} as any)
294+
.mockRejectedValue(new Error('Second page error'));
295+
296+
const { result, waitForNextUpdate } = renderHook(() =>
297+
useDatasetsList(mockDb, 'public'),
298+
);
299+
300+
await waitForNextUpdate();
301+
302+
// Should have first page data, then stop on error
270303
expect(getSpy).toHaveBeenCalledTimes(2);
304+
expect(result.current.datasets).toHaveLength(1);
305+
expect(result.current.datasets[0].table_name).toBe('table1');
306+
expect(result.current.datasetNames).toEqual(['table1']);
307+
expect(mockAddDangerToast).toHaveBeenCalledWith(
308+
'There was an error fetching dataset',
309+
);
310+
expect(mockAddDangerToast).toHaveBeenCalledTimes(1);
271311
});
272312

273313
test('useDatasetsList skips fetching when schema is null or undefined', () => {
@@ -291,6 +331,34 @@ test('useDatasetsList skips fetching when schema is null or undefined', () => {
291331
expect(resultNull.current.datasetNames).toEqual([]);
292332
});
293333

334+
test('useDatasetsList skips fetching when db is undefined', () => {
335+
const getSpy = jest.spyOn(SupersetClient, 'get');
336+
337+
const { result } = renderHook(() => useDatasetsList(undefined, 'public'));
338+
339+
// db is undefined - should NOT call API
340+
expect(getSpy).not.toHaveBeenCalled();
341+
expect(result.current.datasets).toEqual([]);
342+
expect(result.current.datasetNames).toEqual([]);
343+
});
344+
345+
test('useDatasetsList skips fetching when db.id is undefined', () => {
346+
const getSpy = jest.spyOn(SupersetClient, 'get');
347+
348+
// Create db object without id property
349+
const dbWithoutId = {
350+
database_name: 'test_db',
351+
owners: [1] as [number],
352+
} as any;
353+
354+
const { result } = renderHook(() => useDatasetsList(dbWithoutId, 'public'));
355+
356+
// db.id is undefined - should NOT call API
357+
expect(getSpy).not.toHaveBeenCalled();
358+
expect(result.current.datasets).toEqual([]);
359+
expect(result.current.datasetNames).toEqual([]);
360+
});
361+
294362
test('useDatasetsList encodes schemas with spaces and special characters in endpoint URL', async () => {
295363
const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({
296364
json: { count: 0, result: [] },

superset-frontend/src/features/datasets/hooks/useDatasetLists.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ const useDatasetsList = (
6565
} catch (error) {
6666
addDangerToast(t('There was an error fetching dataset'));
6767
logging.error(t('There was an error fetching dataset'), error);
68+
break;
6869
}
6970
}
7071

@@ -78,7 +79,7 @@ const useDatasetsList = (
7879
{ col: 'sql', opr: 'dataset_is_null_or_empty', value: true },
7980
];
8081

81-
if (schema) {
82+
if (schema && db?.id !== undefined) {
8283
getDatasetsList(filters);
8384
}
8485
}, [db?.id, schema, encodedSchema, getDatasetsList]);

superset-frontend/src/hooks/apiResources/datasets.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,4 +419,35 @@ describe('useDatasetDrillInfo - extension path', () => {
419419
expect(result.current.result).toEqual({ verbose_map: {} });
420420
expect(result.current.error).toBeNull();
421421
});
422+
423+
test('falls back to REST API when extension exists but formData is undefined', async () => {
424+
// Extension is registered (mockGetExtensionsRegistry returns it)
425+
// But formData is NOT provided (undefined)
426+
const mockDataset = {
427+
id: 123,
428+
columns: [{ column_name: 'col1', verbose_name: 'Column 1' }],
429+
metrics: [],
430+
};
431+
432+
mockedCachedSupersetGet.mockResolvedValue({
433+
json: { result: mockDataset },
434+
} as any);
435+
436+
const { result, waitForNextUpdate } = renderHook(
437+
() => useDatasetDrillInfo(123, 456, undefined), // formData is undefined
438+
);
439+
440+
await waitForNextUpdate();
441+
442+
// Should use REST API, NOT extension
443+
expect(mockedCachedSupersetGet).toHaveBeenCalledWith({
444+
endpoint: '/api/v1/dataset/123/drill_info/?q=(dashboard_id:456)',
445+
});
446+
expect(mockExtension).not.toHaveBeenCalled();
447+
expect(result.current.status).toBe('complete');
448+
expect(result.current.result).toEqual({
449+
...mockDataset,
450+
verbose_map: { col1: 'Column 1' },
451+
});
452+
});
422453
});

0 commit comments

Comments
 (0)