From 77533a50adf2a5247d0244ba80e4e364cddbbaef Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 23 Jun 2026 10:20:33 -0700 Subject: [PATCH] fix(ag-grid-table): respect row limit with server pagination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the server-pagination row-limit fix from plugin-chart-table (#41024) to the ag-grid table, which shared the same buildQuery logic and therefore the same bugs: - Paging past the configured row limit produced `row_limit: 0`, which the backend treats as "no limit" rather than "no rows" — silently removing the cap and returning more rows than configured. The page is now clamped to the last page that still falls within the limit, and per-page `row_limit` is capped to the remaining rows. - On a filter-change reset the per-request (last-page-capped) `row_limit` was persisted into `ownState.pageSize`, shrinking the page size. The reset now restores the full first-page `row_limit` and persists the user-selected page size. Adds unit tests mirroring the plugin-chart-table coverage. Co-Authored-By: Claude Opus 4.8 --- .../src/buildQuery.ts | 46 ++++- .../test/buildQuery.test.ts | 170 +++++++++++++++++- 2 files changed, 208 insertions(+), 8 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts index 9231a1f38d6..d2d5794026d 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts @@ -58,7 +58,7 @@ export function getQueryMode(formData: TableChartFormData) { return hasRawColumns ? QueryMode.Raw : QueryMode.Aggregate; } -const buildQuery: BuildQuery = ( +export const buildQuery: BuildQuery = ( formData: TableChartFormData, options, ) => { @@ -209,6 +209,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 = @@ -222,11 +233,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; } let sortByFromOwnState: QueryFormOrderBy[] | undefined; @@ -378,11 +402,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, lastFilteredColumn: undefined, lastFilteredInputPosition: undefined, }; diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts index 6333fd57445..7e9002a4b62 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts @@ -17,7 +17,9 @@ * under the License. */ import { AdhocColumn, QueryMode, 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 = { @@ -1561,4 +1563,170 @@ describe('plugin-chart-ag-grid-table', () => { expect(queries[1].metrics).toEqual(['count']); }); }); + + describe('buildQuery - server pagination row limit', () => { + const serverPaginationFormData: TableChartFormData = { + ...basicFormData, + server_pagination: true, + }; + + test('uses user row limit when it is lower than server page size', () => { + const { queries } = buildQuery( + { + ...serverPaginationFormData, + 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( + { + ...serverPaginationFormData, + row_limit: 120, + server_page_length: 50, + slice_id: 102, + }, + { + ownState: { + currentPage: 2, + pageSize: 50, + }, + }, + ); + + expect(queries[0]).toMatchObject({ + row_limit: 20, + row_offset: 100, + }); + }); + + test('clamps pages beyond the row limit instead of emitting row_limit: 0', () => { + const { queries } = buildQuery( + { + ...serverPaginationFormData, + 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( + { + ...serverPaginationFormData, + 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: 'state', 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( + { + ...serverPaginationFormData, + 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: 'state', 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( + { + ...serverPaginationFormData, + 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, + }); + }); + }); });