codeant-ai-for-open-source[bot] commented on code in PR #38369:
URL: https://github.com/apache/superset/pull/38369#discussion_r2880470539
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -482,12 +482,76 @@ const buildQuery: BuildQuery<TableChartFormData> = (
};
}
- // Add AG Grid complex WHERE clause from ownState (non-metric filters)
+ // Map metric/column labels to SQL expressions for WHERE/HAVING
resolution
+ const sqlExpressionMap: Record<string, string> = {};
+ (metrics || []).forEach((m: QueryFormMetric) => {
+ if (typeof m === 'object' && 'expressionType' in m) {
+ const label = getMetricLabel(m);
+ if (m.expressionType === 'SQL' && m.sqlExpression) {
+ sqlExpressionMap[label] = m.sqlExpression;
+ } else if (
+ m.expressionType === 'SIMPLE' &&
+ m.aggregate &&
+ m.column?.column_name
+ ) {
+ sqlExpressionMap[label] =
+ `${m.aggregate}(${m.column.column_name})`;
+ }
+ }
+ });
+ // Map dimension columns with custom SQL expressions
+ (columns || []).forEach((col: QueryFormColumn) => {
+ if (typeof col === 'object' && 'sqlExpression' in col) {
+ const label = getColumnLabel(col);
+ if (col.sqlExpression) {
+ sqlExpressionMap[label] = col.sqlExpression;
+ }
+ }
+ });
+ // Merge datasource-level saved metrics and calculated columns
+ if (ownState.metricSqlExpressions) {
+ Object.entries(
+ ownState.metricSqlExpressions as Record<string, string>,
+ ).forEach(([label, expression]) => {
+ if (!sqlExpressionMap[label]) {
+ sqlExpressionMap[label] = expression;
+ }
+ });
+ }
+
+ const resolveLabelsToSQL = (clause: string): string => {
+ let resolved = clause;
+ // Sort by label length descending to prevent substring false positives
+ const sortedEntries = Object.entries(sqlExpressionMap).sort(
+ ([a], [b]) => b.length - a.length,
+ );
+ sortedEntries.forEach(([label, expression]) => {
+ if (resolved.includes(label)) {
+ const escapedLabel = label.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ // Wrap complex expressions in parentheses for valid SQL
+ const isExpression =
+ expression.includes('(') ||
+ expression.toUpperCase().includes('CASE') ||
+ expression.includes('\n');
+ const wrappedExpression = isExpression
+ ? `(${expression})`
+ : `"${expression}"`;
+ resolved = resolved.replace(
+ new RegExp(`\\b${escapedLabel}\\b`, 'g'),
+ wrappedExpression,
+ );
+ }
+ });
+ return resolved;
+ };
Review Comment:
**Suggestion:** The `resolveLabelsToSQL` helper wraps any SQL expression
that doesn't contain parentheses/CASE/newlines in double quotes, which turns
arbitrary expressions like `col1 + col2` into `"col1 + col2"` and causes SQL
errors because the database interprets them as identifiers rather than
expressions; instead, non-complex expressions should be used as-is while only
complex ones are wrapped in parentheses. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Server-side AG Grid metric filters can fail with SQL errors.
- ⚠️ Users cannot reliably filter on expression-based metrics.
```
</details>
```suggestion
const resolveLabelsToSQL = (clause: string): string => {
let resolved = clause;
// Sort by label length descending to prevent substring false
positives
const sortedEntries = Object.entries(sqlExpressionMap).sort(
([a], [b]) => b.length - a.length,
);
sortedEntries.forEach(([label, expression]) => {
if (resolved.includes(label)) {
const escapedLabel = label.replace(/[.*+?^${}()|[\]\\]/g,
'\\$&');
// Wrap complex expressions in parentheses for valid SQL
const isExpression =
expression.includes('(') ||
expression.toUpperCase().includes('CASE') ||
expression.includes('\n');
const wrappedExpression = isExpression
? `(${expression})`
: expression;
resolved = resolved.replace(
new RegExp(`\\b${escapedLabel}\\b`, 'g'),
wrappedExpression,
);
}
});
return resolved;
};
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an AG Grid Table chart on a dataset with `server_pagination:
true` and a SQL
metric defined as an ad-hoc metric in the Explore view whose expression
contains operators
but no parentheses/CASE, e.g. `col1 + col2` (this metric is sent to the
backend as a
`QueryFormMetric` with `expressionType: 'SQL'` and `sqlExpression: 'col1 +
col2'` and ends
up in `baseQueryObject.metrics` used in `buildQuery` at
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:90`
and lines
485–499 where `sqlExpressionMap` is built).
2. In the AG Grid UI, enable server-side filtering on this metric and create
a complex
filter such as `metric_label > 5`, where `metric_label` is the display label
of the SQL
metric; `AgGridTableChart` writes the resulting string filter into
`ownState.agGridComplexWhere` via `handleFilterChanged` at
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:28–43`
(see
`agGridComplexWhere: completeFilterState.complexWhere`).
3. When the chart reloads, `cachedBuildQuery` (exported from `buildQuery.ts`
at lines
689–710) is called with this `ownState`, and inside `buildQuery` with
`formData.server_pagination === true` the server-pagination block at
`buildQuery.ts:437–581` is executed; there `sqlExpressionMap` is populated
from `metrics`
(lines 485–499) so that `sqlExpressionMap[metric_label] = 'col1 + col2'`.
4. Still in the same block, `resolveLabelsToSQL` at `buildQuery.ts:522–545`
is called with
`ownState.agGridComplexWhere` (line 549–550); because the mapped expression
`'col1 +
col2'` contains no `(`, `CASE`, or newline, `isExpression` is false and
`wrappedExpression` becomes `"col1 + col2"` (line 536–538), so the final
`extras.where`
built at lines 549–554 contains `"col1 + col2" > 5` rather than `col1 + col2
> 5`, causing
the generated SQL to reference a non-existent identifier `"col1 + col2"` and
producing a
database error when the backend executes the query.
```
</details>
<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:** 522:546
**Comment:**
*Logic Error: The `resolveLabelsToSQL` helper wraps any SQL expression
that doesn't contain parentheses/CASE/newlines in double quotes, which turns
arbitrary expressions like `col1 + col2` into `"col1 + col2"` and causes SQL
errors because the database interprets them as identifiers rather than
expressions; instead, non-complex expressions should be used as-is while only
complex ones are wrapped in parentheses.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38369&comment_hash=0079ef80d8059af4406e7c9111a03dce6e7bd9795e2d146c606040bf27588e5b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38369&comment_hash=0079ef80d8059af4406e7c9111a03dce6e7bd9795e2d146c606040bf27588e5b&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -498,12 +562,13 @@ const buildQuery: BuildQuery<TableChartFormData> = (
};
}
Review Comment:
**Suggestion:** After resolving AG Grid filter labels to SQL expressions,
`extras.where` contains the resolved clause while `ownState.agGridComplexWhere`
still holds the original label-based string, so the later totals query logic
that tries to strip the AG Grid WHERE fragment using
`ownState.agGridComplexWhere` no longer matches and leaves the filter applied
to totals, causing totals to be incorrectly filtered; updating
`ownState.agGridComplexWhere` to the resolved value keeps the stripping logic
working. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Totals row in AG Grid table shows filtered aggregates.
- ⚠️ Dashboards using totals misrepresent overall metric values.
```
</details>
```suggestion
// Resolve and apply AG Grid WHERE clause
if (ownState.agGridComplexWhere && ownState.agGridComplexWhere.trim())
{
const resolvedWhere =
resolveLabelsToSQL(ownState.agGridComplexWhere);
// Keep ownState.agGridComplexWhere in sync with the resolved
expression
// so totals queries can correctly strip the AG Grid WHERE clause
(ownState as Record<string, unknown>).agGridComplexWhere =
resolvedWhere;
const existingWhere = queryObject.extras?.where;
const combinedWhere = existingWhere
? `${existingWhere} AND ${resolvedWhere}`
: resolvedWhere;
queryObject = {
...queryObject,
extras: {
...queryObject.extras,
where: combinedWhere,
},
};
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build an AG Grid Table chart with `server_pagination: true`, `query_mode:
QueryMode.Aggregate`, at least one metric, and `show_totals: true` so that
totals queries
are generated; this passes through `buildQuery` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:61–685`,
which
constructs both the main query and a totals query (see lines 617–663 where
`extraQueries`
includes a totals query).
2. In the AG Grid UI, define a metric or calculated column whose label
requires resolution
(e.g. SQL metric `my_metric` defined on a saved metric or ad-hoc expression)
and apply a
complex filter on it so that `completeFilterState.complexWhere` contains a
label-based
condition such as `(my_metric > 5)`; `AgGridTableChart.handleFilterChanged`
at
`src/AgGridTableChart.tsx:28–43` stores this into
`ownState.agGridComplexWhere` via
`agGridComplexWhere: completeFilterState.complexWhere`.
3. On the next query execution, `cachedBuildQuery` (exported at
`buildQuery.ts:689–710`)
calls `buildQuery` with this `ownState`; inside the server-pagination block
at
`buildQuery.ts:437–581`, `sqlExpressionMap` is built (lines 485–520) and
`resolveLabelsToSQL` at lines 522–545 is invoked in the WHERE-clause logic
at lines
548–554, producing a `resolvedWhere` string where occurrences of `my_metric`
are replaced
with the underlying SQL expression (e.g. `(col1 + col2)`), so
`queryObject.extras.where`
now contains the resolved clause, not the original
`ownState.agGridComplexWhere` string.
4. Later in the same `buildQuery` invocation, the totals query is
constructed at
`buildQuery.ts:617–663`; the code copies `totalsExtras = {
...queryObject.extras }` (line
627), then attempts to strip the AG Grid filter by doing string replacements
using
`ownState.agGridComplexWhere` as `agGridWhere` (lines 628–641), but since
`ownState.agGridComplexWhere` still holds the original label-based clause
(e.g.
`(my_metric > 5)`) and `totalsExtras.where` contains the resolved SQL clause
(e.g. `(col1
+ col2 > 5)`), none of the `replace` calls match and the AG Grid filter
remains in
`totalsExtras.where`, causing the totals query pushed into `extraQueries` at
lines 653–662
to include the AG Grid filter and return filtered totals instead of overall
totals.
```
</details>
<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:** 548:563
**Comment:**
*Logic Error: After resolving AG Grid filter labels to SQL expressions,
`extras.where` contains the resolved clause while `ownState.agGridComplexWhere`
still holds the original label-based string, so the later totals query logic
that tries to strip the AG Grid WHERE fragment using
`ownState.agGridComplexWhere` no longer matches and leaves the filter applied
to totals, causing totals to be incorrectly filtered; updating
`ownState.agGridComplexWhere` to the resolved value keeps the stripping logic
working.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38369&comment_hash=c7339b24c321a63769b66e8bd73cd6bbcde2bdd5b6737175456e6e7102baf10a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38369&comment_hash=c7339b24c321a63769b66e8bd73cd6bbcde2bdd5b6737175456e6e7102baf10a&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]
