rusackas commented on code in PR #41346:
URL: https://github.com/apache/superset/pull/41346#discussion_r3462568937
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -1363,4 +1365,170 @@ describe('plugin-chart-ag-grid-table', () => {
expect(query.extras?.where || undefined).toBeUndefined();
});
});
+
+ 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,
+ }),
+ }),
+ );
Review Comment:
This test is pinning the page-size persistence on the capped last page, not
the caching hook. `setCachedChanges` is just a stub to satisfy the hook
signature, so asserting it here would muddy what the test is actually checking.
##########
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<TableChartFormData> = (
+export const buildQuery: BuildQuery<TableChartFormData> = (
Review Comment:
Agreed these two `buildQuery` implementations have drifted close, but
consolidating them into a shared util is a cross-plugin refactor well beyond
this row-limit fix. Better as its own PR.
--
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]
