sha174n commented on code in PR #40607:
URL: https://github.com/apache/superset/pull/40607#discussion_r3341280290
##########
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:
Copilot's right. Hoisting `IN_RANGE` out of the `&& filterTo !== undefined`
guard and rejecting `undefined / null / ''` explicitly before coercion:
```suggestion
if (type === FILTER_OPERATORS.IN_RANGE) {
// Range bounds are interpolated unquoted, so they must be finite
// numbers. Number('') and Number(null) coerce to 0 and pass
// Number.isFinite, so reject the cleared/null/undefined cases
// explicitly before coercion.
if (
value === undefined || value === null || value === '' ||
filterTo === undefined || filterTo === null || filterTo === ''
) {
return '';
}
const lowerBound = Number(value);
const upperBound = Number(filterTo);
if (!Number.isFinite(lowerBound) || !Number.isFinite(upperBound)) {
return '';
}
return `${columnName} ${SQL_OPERATORS.BETWEEN} ${lowerBound} AND
${upperBound}`;
}
```
##########
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:
Agreed, worth a couple more regressions. Three cases to cover: `filter: ''`,
`filter: null`, `filterTo: undefined` — each should produce no clause. Up to
you on naming/structure.
--
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]
