Copilot commented on code in PR #40607:
URL: https://github.com/apache/superset/pull/40607#discussion_r3337852685
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts:
##########
@@ -284,6 +284,38 @@ describe('agGridFilterConverter', () => {
val: 18,
});
});
+
+ test('should emit a numeric BETWEEN clause for a metric range filter', ()
=> {
+ const filterModel: AgGridFilterModel = {
+ revenue: {
+ filterType: 'number',
+ type: 'inRange',
+ filter: 10,
+ filterTo: 20,
+ },
+ };
+
+ // revenue is a metric, so the range filter renders as a HAVING clause
+ const result = convertAgGridFiltersToSQL(filterModel, ['revenue']);
+
+ expect(result.havingClause).toContain('BETWEEN 10 AND 20');
+ });
+
+ test('should drop a metric range filter whose bounds are not numeric', ()
=> {
+ const filterModel = {
+ revenue: {
+ filterType: 'number',
+ type: 'inRange',
+ filter: '0',
+ filterTo: '100 OR 1=1',
+ },
+ } as unknown as AgGridFilterModel;
+
+ const result = convertAgGridFiltersToSQL(filterModel, ['revenue']);
+
+ // a non-numeric bound must never be interpolated into the clause
+ expect(result.havingClause).toBeUndefined();
+ });
Review Comment:
The negative test only covers an obviously non-numeric string. Because
`Number('')` and `Number(null)` become `0`, it’s worth adding a regression
assertion for an empty/cleared bound so this doesn’t silently generate `BETWEEN
... AND 0`.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts:
##########
@@ -379,7 +379,15 @@ function simpleFilterToWhereClause(
}
if (type === FILTER_OPERATORS.IN_RANGE && filterTo !== undefined) {
- return `${columnName} ${SQL_OPERATORS.BETWEEN} ${value} AND ${filterTo}`;
+ // Range bounds are interpolated into the clause without quoting, so they
+ // must be finite numbers. Coerce both ends and skip the clause entirely
+ // if either is not numeric.
+ const lowerBound = Number(value);
+ const upperBound = Number(filterTo);
+ if (!Number.isFinite(lowerBound) || !Number.isFinite(upperBound)) {
+ return '';
+ }
+ return `${columnName} ${SQL_OPERATORS.BETWEEN} ${lowerBound} AND
${upperBound}`;
}
Review Comment:
`Number()` treats `''` and `null` as `0`, and `Number.isFinite(0)` is true.
With the current logic, an empty/cleared bound (or null) can incorrectly
produce a `BETWEEN ... AND 0` clause instead of being dropped. Also, when `type
=== inRange` but `filterTo` is `undefined`, this function falls through and may
emit an invalid single-operand `BETWEEN` clause. Consider handling `IN_RANGE`
unconditionally and rejecting `undefined/null/empty-string` bounds before
coercion.
--
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]
