codeant-ai-for-open-source[bot] commented on code in PR #41346:
URL: https://github.com/apache/superset/pull/41346#discussion_r3533560991
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -222,11 +233,24 @@ const buildQuery: BuildQuery<TableChartFormData> = (
}
if (!isDownloadQuery && formDataCopy.server_pagination) {
- const pageSize = ownState.pageSize ?? formDataCopy.server_page_length;
- const currentPage = ownState.currentPage ?? 0;
-
- moreProps.row_limit = pageSize;
- moreProps.row_offset = currentPage * pageSize;
+ // 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 =
+ configuredRowLimit > 0 ? Math.min(pageSize, remainingRows) : pageSize;
+ moreProps.row_offset = rowOffset;
Review Comment:
**Suggestion:** When `server_page_length` (or persisted `ownState.pageSize`)
is `0`—which this plugin’s control panel explicitly allows to mean “no
pagination”—the new pagination math computes `row_limit` as `0`. In Superset
backend semantics, `row_limit: 0` means “no limit”, so this bypasses the
configured row cap and can return unlimited rows again. Handle `pageSize <= 0`
as a special case by setting `row_offset` to `0` and using the configured row
limit (or leaving it undefined when no cap is configured), instead of feeding
`0` into the page/remaining-row math. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Server-paginated Ag Grid tables ignore configured row_limit cap.
⚠️ Dashboard views with no pagination can fetch unbounded rows.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open or create an Ag Grid table chart (viz type `table_ag_grid`
registered in
`superset-frontend/src/visualizations/presets/MainPreset.ts:25-27` and wired
up in
`superset-frontend/src/setup/setupPlugins.ts:10-18`) so that the frontend
uses
`buildQuery()` from
`plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:61-90` to
construct its queries.
2. In the Explore control panel for this chart, enable server pagination and
set
`server_page_length` to `0` and `row_limit` to a positive value (for example
1000), using
the controls defined in
`plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:12-23,
28-58`, where `server_page_length` is documented as "Rows per page, 0 means
no
pagination".
3. Trigger a data fetch (e.g., run or refresh the chart); the chart state is
converted to
backend `ownState` via `convertAgGridStateToOwnState` (registered in
`src/setup/setupPlugins.ts:16-18` and consumed by `buildQuery()`), so
`formData.server_pagination` is true, `formData.server_page_length` is `0`,
`formData.row_limit` is > 0, and `options.ownState.pageSize` is `0` or
undefined.
4. Observe in `plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:210-223,
235-253` that
`pageSize` is computed as `Number(ownState.pageSize ??
formDataCopy.server_page_length) ||
0` (yielding `0`), `configuredRowLimit` is the positive `row_limit`, and the
non-download
server pagination branch sets `moreProps.row_limit` to `Math.min(pageSize,
remainingRows)`
which evaluates to `0`, and `row_offset` to `0`; the constructed
`queryObject` at lines
118-138 therefore carries `row_limit: 0` to the backend, which according to
the API docs
(`superset/docs/static/resources/openapi.json:1559-1562` "Maximum row count
(0=disabled)")
and the inline comment in `buildQuery.ts:236-239` is treated as "no limit",
so the
configured row cap is bypassed and the backend can return unbounded rows
instead of being
limited by `row_limit`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1373dfe2145448708a5d1bcbcbfcc82d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1373dfe2145448708a5d1bcbcbfcc82d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts
**Line:** 215:253
**Comment:**
*Incorrect Condition Logic: When `server_page_length` (or persisted
`ownState.pageSize`) is `0`—which this plugin’s control panel explicitly allows
to mean “no pagination”—the new pagination math computes `row_limit` as `0`. In
Superset backend semantics, `row_limit: 0` means “no limit”, so this bypasses
the configured row cap and can return unlimited rows again. Handle `pageSize <=
0` as a special case by setting `row_offset` to `0` and using the configured
row limit (or leaving it undefined when no cap is configured), instead of
feeding `0` into the page/remaining-row math.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41346&comment_hash=9db693f04483c9303395e0b010d838f58fc718a0166f89b95f5419cbd40a3e2a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41346&comment_hash=9db693f04483c9303395e0b010d838f58fc718a0166f89b95f5419cbd40a3e2a&reaction=dislike'>👎</a>
--
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]
