From c1d08bf27c063aa968a2c6e869abb28299e66ee2 Mon Sep 17 00:00:00 2001 From: innovark Date: Tue, 23 Jun 2026 22:17:12 +0300 Subject: [PATCH] fix(table): respect row limit with server pagination (#41024) Co-authored-by: Evan Rusackas Co-authored-by: Claude --- .../plugin-chart-table/src/buildQuery.ts | 46 ++++- .../test/buildQuery.test.ts | 170 +++++++++++++++++- 2 files changed, 208 insertions(+), 8 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts index c23a8a49c56..fe8278f1c0a 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts @@ -54,7 +54,7 @@ export function getQueryMode(formData: TableChartFormData) { return hasRawColumns ? QueryMode.Raw : QueryMode.Aggregate; } -const buildQuery: BuildQuery = ( +export const buildQuery: BuildQuery = ( formData: TableChartFormData, options, ) => { @@ -217,6 +217,17 @@ const buildQuery: BuildQuery = ( const moreProps: Partial = {}; const ownState = options?.ownState ?? {}; + // Server pagination sizing, shared between the per-page request below and + // the filter-change reset further down. + const pageSize = + Number(ownState.pageSize ?? formDataCopy.server_page_length) || 0; + const configuredRowLimit = Number(formDataCopy.row_limit) || 0; + // row_limit for the first page, capped by the configured row limit. Used + // when a filter change resets pagination back to page 0. + const firstPageRowLimit = + configuredRowLimit > 0 + ? Math.min(pageSize, configuredRowLimit) + : pageSize; // Build Query flag to check if its for either download as csv, excel or json const isDownloadQuery = ['csv', 'xlsx'].includes(formData?.result_format || '') || @@ -229,11 +240,24 @@ const buildQuery: BuildQuery = ( } if (!isDownloadQuery && formDataCopy.server_pagination) { - const pageSize = ownState.pageSize ?? formDataCopy.server_page_length; - const currentPage = ownState.currentPage ?? 0; + // Never page past the configured row limit. Clamping the page to the last + // one that still falls within the limit keeps the request inside the cap + // and avoids emitting row_limit: 0, which the backend treats as + // "no limit" rather than "no rows" (see helpers.py get_sqla_query). + const lastPage = + configuredRowLimit > 0 && pageSize > 0 + ? Math.max(Math.ceil(configuredRowLimit / pageSize) - 1, 0) + : Number(ownState.currentPage) || 0; + const currentPage = Math.min(Number(ownState.currentPage) || 0, lastPage); + const rowOffset = currentPage * pageSize; + const remainingRows = + configuredRowLimit > 0 + ? Math.max(configuredRowLimit - rowOffset, 0) + : pageSize; - moreProps.row_limit = pageSize; - moreProps.row_offset = currentPage * pageSize; + moreProps.row_limit = + configuredRowLimit > 0 ? Math.min(pageSize, remainingRows) : pageSize; + moreProps.row_offset = rowOffset; } // getting sort by in case of server pagination from own state @@ -263,11 +287,19 @@ const buildQuery: BuildQuery = ( JSON.stringify(options?.extras?.cachedChanges?.[formData.slice_id]) !== JSON.stringify(queryObject.filters) ) { - queryObject = { ...queryObject, row_offset: 0 }; + // Reset to the first page: restore the full first-page row_limit rather + // than carrying over the last page's capped value. + queryObject = { + ...queryObject, + row_offset: 0, + row_limit: firstPageRowLimit, + }; const modifiedOwnState = { ...options?.ownState, currentPage: 0, - pageSize: queryObject.row_limit ?? 0, + // Persist the user-selected page size, not the per-request row_limit, + // which may be capped to the remaining rows on the last page. + pageSize, }; updateTableOwnState(options?.hooks?.setDataMask, modifiedOwnState); } diff --git a/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts index aa6e1b83001..bbea811f594 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts @@ -17,7 +17,9 @@ * under the License. */ import { QueryMode, TimeGranularity, VizType } from '@superset-ui/core'; -import buildQuery from '../src/buildQuery'; +import buildQuery, { + buildQuery as buildQueryUncached, +} from '../src/buildQuery'; import { TableChartFormData } from '../src/types'; const basicFormData: TableChartFormData = { @@ -278,6 +280,172 @@ describe('plugin-chart-table', () => { expect(queries[0].filters?.some(f => f.op === 'ILIKE')).toBeFalsy(); }); + + test('uses user row limit when it is lower than server page size', () => { + const { queries } = buildQuery( + { + ...baseFormDataWithServerPagination, + row_limit: 10, + server_page_length: 20, + slice_id: 101, + }, + { + ownState: { + currentPage: 0, + pageSize: 20, + }, + }, + ); + + expect(queries[0]).toMatchObject({ + row_limit: 10, + row_offset: 0, + }); + }); + + test('limits server page size by remaining rows inside user row limit', () => { + const { queries } = buildQuery( + { + ...baseFormDataWithServerPagination, + row_limit: 120, + server_page_length: 50, + slice_id: 102, + }, + { + ownState: { + currentPage: 2, + pageSize: 50, + sortBy: [{ key: 'category', desc: true }], + }, + }, + ); + + expect(queries[0]).toMatchObject({ + orderby: [['category', false]], + row_limit: 20, + row_offset: 100, + }); + expect(queries[1]).toMatchObject({ + is_rowcount: true, + row_limit: 120, + row_offset: 0, + }); + }); + + test('clamps pages beyond the row limit instead of emitting row_limit: 0', () => { + const { queries } = buildQuery( + { + ...baseFormDataWithServerPagination, + row_limit: 120, + server_page_length: 50, + slice_id: 103, + }, + { + ownState: { + // Page 5 is well past the cap; offset would be 250 > 120, which + // previously made row_limit collapse to 0 ("no limit"). + currentPage: 5, + pageSize: 50, + }, + }, + ); + + expect(queries[0].row_limit).not.toBe(0); + expect(queries[0]).toMatchObject({ + row_limit: 20, + row_offset: 100, + }); + }); + + test('restores the full first-page row limit after a filter change reset', () => { + // Uncached export lets us seed cachedChanges directly; the default + // export overrides extras with its own closure. + const { queries } = buildQueryUncached( + { + ...baseFormDataWithServerPagination, + row_limit: 120, + server_page_length: 50, + slice_id: 104, + }, + { + // User was on the capped last page (row_limit would be 20)... + ownState: { + currentPage: 2, + pageSize: 50, + }, + // ...then an external filter changed, so the cached filters differ + // from the current ones and pagination resets to page 0. + extras: { + cachedChanges: { + 104: [{ col: 'category', op: '==', val: 'previous' }], + }, + }, + }, + ); + + expect(queries[0].row_limit).not.toBe(0); + expect(queries[0]).toMatchObject({ + row_limit: 50, + row_offset: 0, + }); + }); + + test('persists the user page size, not the capped limit, on filter reset', () => { + const setDataMask = jest.fn(); + buildQueryUncached( + { + ...baseFormDataWithServerPagination, + row_limit: 120, + server_page_length: 50, + slice_id: 106, + }, + { + // On the capped last page, the per-request row_limit is 20. + ownState: { + currentPage: 2, + pageSize: 50, + }, + extras: { + cachedChanges: { + 106: [{ col: 'category', op: '==', val: 'previous' }], + }, + }, + hooks: { setDataMask, setCachedChanges: jest.fn() }, + }, + ); + + // The persisted page size must stay 50, not collapse to the capped 20. + expect(setDataMask).toHaveBeenCalledWith( + expect.objectContaining({ + ownState: expect.objectContaining({ + currentPage: 0, + pageSize: 50, + }), + }), + ); + }); + + test('falls back to the page size when no row limit is configured', () => { + const { queries } = buildQuery( + { + ...baseFormDataWithServerPagination, + row_limit: undefined, + server_page_length: 50, + slice_id: 105, + }, + { + ownState: { + currentPage: 3, + pageSize: 50, + }, + }, + ); + + expect(queries[0]).toMatchObject({ + row_limit: 50, + row_offset: 150, + }); + }); }); }); });