Skip to content

Commit f1ebac6

Browse files
amaannawab923amaannawab923
committed
resolve_comments
1 parent 245aa0c commit f1ebac6

3 files changed

Lines changed: 95 additions & 70 deletions

File tree

‎superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
ensureIsArray,
2323
getColumnLabel,
2424
getMetricLabel,
25+
isDefined,
2526
isPhysicalColumn,
2627
QueryFormColumn,
2728
QueryFormMetric,
@@ -255,14 +256,16 @@ const buildQuery: BuildQuery<TableChartFormData> = (
255256
sortByFromOwnState = sortSource
256257
.map(
257258
(sortItem: {
258-
colId?: string;
259-
key?: string;
259+
colId?: string | number;
260+
key?: string | number;
260261
sort?: string;
261262
desc?: boolean;
262263
}) => {
263-
const colId = sortItem?.colId || sortItem?.key;
264-
if (!colId) return null;
265-
const sortKey = mapColIdToIdentifier(colId);
264+
const colId = isDefined(sortItem?.colId)
265+
? sortItem.colId
266+
: sortItem?.key;
267+
if (!isDefined(colId)) return null;
268+
const sortKey = mapColIdToIdentifier(String(colId));
266269
if (!sortKey) return null;
267270
const isDesc = sortItem?.sort === 'desc' || sortItem?.desc;
268271
return [sortKey, !isDesc] as QueryFormOrderBy;

‎superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts‎

Lines changed: 81 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -25,31 +25,13 @@ import {
2525
type AgGridFilterModel,
2626
type AgGridFilter,
2727
} from '@superset-ui/core';
28-
import { getStartOfDay, getEndOfDay } from './utils/agGridFilterConverter';
29-
30-
/**
31-
* AG Grid text filter type to backend operator mapping
32-
*/
33-
const TEXT_FILTER_OPERATORS: Record<string, string> = {
34-
equals: '=',
35-
notEqual: '!=',
36-
contains: 'ILIKE',
37-
notContains: 'NOT ILIKE',
38-
startsWith: 'ILIKE',
39-
endsWith: 'ILIKE',
40-
};
41-
42-
/**
43-
* AG Grid number filter type to backend operator mapping
44-
*/
45-
const NUMBER_FILTER_OPERATORS: Record<string, string> = {
46-
equals: '=',
47-
notEqual: '!=',
48-
lessThan: '<',
49-
lessThanOrEqual: '<=',
50-
greaterThan: '>',
51-
greaterThanOrEqual: '>=',
52-
};
28+
import {
29+
getStartOfDay,
30+
getEndOfDay,
31+
FILTER_OPERATORS,
32+
SQL_OPERATORS,
33+
validateColumnName,
34+
} from './utils/agGridFilterConverter';
5335

5436
/**
5537
* Maps custom server-side date filter operators to normalized operator names.
@@ -58,29 +40,29 @@ const NUMBER_FILTER_OPERATORS: Record<string, string> = {
5840
*/
5941
const DATE_FILTER_OPERATOR_MAP: Record<string, string> = {
6042
// Standard operators
61-
equals: 'equals',
62-
notEqual: 'notEqual',
63-
lessThan: 'lessThan',
64-
lessThanOrEqual: 'lessThanOrEqual',
65-
greaterThan: 'greaterThan',
66-
greaterThanOrEqual: 'greaterThanOrEqual',
67-
inRange: 'inRange',
43+
[FILTER_OPERATORS.EQUALS]: FILTER_OPERATORS.EQUALS,
44+
[FILTER_OPERATORS.NOT_EQUAL]: FILTER_OPERATORS.NOT_EQUAL,
45+
[FILTER_OPERATORS.LESS_THAN]: FILTER_OPERATORS.LESS_THAN,
46+
[FILTER_OPERATORS.LESS_THAN_OR_EQUAL]: FILTER_OPERATORS.LESS_THAN_OR_EQUAL,
47+
[FILTER_OPERATORS.GREATER_THAN]: FILTER_OPERATORS.GREATER_THAN,
48+
[FILTER_OPERATORS.GREATER_THAN_OR_EQUAL]: FILTER_OPERATORS.GREATER_THAN_OR_EQUAL,
49+
[FILTER_OPERATORS.IN_RANGE]: FILTER_OPERATORS.IN_RANGE,
6850
// Custom server-side operators (map to standard equivalents)
69-
serverEquals: 'equals',
70-
serverNotEqual: 'notEqual',
71-
serverBefore: 'lessThan',
72-
serverAfter: 'greaterThan',
73-
serverInRange: 'inRange',
51+
[FILTER_OPERATORS.SERVER_EQUALS]: FILTER_OPERATORS.EQUALS,
52+
[FILTER_OPERATORS.SERVER_NOT_EQUAL]: FILTER_OPERATORS.NOT_EQUAL,
53+
[FILTER_OPERATORS.SERVER_BEFORE]: FILTER_OPERATORS.LESS_THAN,
54+
[FILTER_OPERATORS.SERVER_AFTER]: FILTER_OPERATORS.GREATER_THAN,
55+
[FILTER_OPERATORS.SERVER_IN_RANGE]: FILTER_OPERATORS.IN_RANGE,
7456
};
7557

7658
/**
7759
* Blank filter operator types
7860
*/
7961
const BLANK_OPERATORS = new Set([
80-
'blank',
81-
'notBlank',
82-
'serverBlank',
83-
'serverNotBlank',
62+
FILTER_OPERATORS.BLANK,
63+
FILTER_OPERATORS.NOT_BLANK,
64+
FILTER_OPERATORS.SERVER_BLANK,
65+
FILTER_OPERATORS.SERVER_NOT_BLANK,
8466
]);
8567

8668
/** Escapes single quotes in SQL strings: O'Hara → O''Hara */
@@ -89,13 +71,13 @@ function escapeStringValue(value: string): string {
8971
}
9072

9173
function getTextComparator(type: string, value: string): string {
92-
if (type === 'contains' || type === 'notContains') {
74+
if (type === FILTER_OPERATORS.CONTAINS || type === FILTER_OPERATORS.NOT_CONTAINS) {
9375
return `%${value}%`;
9476
}
95-
if (type === 'startsWith') {
77+
if (type === FILTER_OPERATORS.STARTS_WITH) {
9678
return `${value}%`;
9779
}
98-
if (type === 'endsWith') {
80+
if (type === FILTER_OPERATORS.ENDS_WITH) {
9981
return `%${value}`;
10082
}
10183
return value;
@@ -122,33 +104,33 @@ function convertDateFilterToSQL(
122104
const normalizedType = DATE_FILTER_OPERATOR_MAP[type] || type;
123105

124106
switch (normalizedType) {
125-
case 'equals':
107+
case FILTER_OPERATORS.EQUALS:
126108
if (!dateFrom) return null;
127109
// Full day range for equals
128110
return `(${colId} >= '${getStartOfDay(dateFrom)}' AND ${colId} <= '${getEndOfDay(dateFrom)}')`;
129111

130-
case 'notEqual':
112+
case FILTER_OPERATORS.NOT_EQUAL:
131113
if (!dateFrom) return null;
132114
// Outside the full day range for not equals
133115
return `(${colId} < '${getStartOfDay(dateFrom)}' OR ${colId} > '${getEndOfDay(dateFrom)}')`;
134116

135-
case 'lessThan':
117+
case FILTER_OPERATORS.LESS_THAN:
136118
if (!dateFrom) return null;
137119
return `${colId} < '${getStartOfDay(dateFrom)}'`;
138120

139-
case 'lessThanOrEqual':
121+
case FILTER_OPERATORS.LESS_THAN_OR_EQUAL:
140122
if (!dateFrom) return null;
141123
return `${colId} <= '${getEndOfDay(dateFrom)}'`;
142124

143-
case 'greaterThan':
125+
case FILTER_OPERATORS.GREATER_THAN:
144126
if (!dateFrom) return null;
145127
return `${colId} > '${getEndOfDay(dateFrom)}'`;
146128

147-
case 'greaterThanOrEqual':
129+
case FILTER_OPERATORS.GREATER_THAN_OR_EQUAL:
148130
if (!dateFrom) return null;
149131
return `${colId} >= '${getStartOfDay(dateFrom)}'`;
150132

151-
case 'inRange':
133+
case FILTER_OPERATORS.IN_RANGE:
152134
if (!dateFrom || !dateTo) return null;
153135
return `${colId} BETWEEN '${getStartOfDay(dateFrom)}' AND '${getEndOfDay(dateTo)}'`;
154136

@@ -207,6 +189,11 @@ function convertFilterToSQL(
207189
colId: string,
208190
filter: AgGridFilter,
209191
): string | null {
192+
// Validate column name to prevent SQL injection and malformed queries
193+
if (!validateColumnName(colId)) {
194+
return null;
195+
}
196+
210197
// Complex filter: has operator and conditions
211198
if (
212199
filter.operator &&
@@ -231,31 +218,64 @@ function convertFilterToSQL(
231218
// Handle blank/notBlank operators for all filter types
232219
// These are special operators that check for NULL values
233220
if (filter.type && BLANK_OPERATORS.has(filter.type)) {
234-
if (filter.type === 'blank' || filter.type === 'serverBlank') {
235-
return `${colId} IS NULL`;
221+
if (
222+
filter.type === FILTER_OPERATORS.BLANK ||
223+
filter.type === FILTER_OPERATORS.SERVER_BLANK
224+
) {
225+
return `${colId} ${SQL_OPERATORS.IS_NULL}`;
236226
}
237-
if (filter.type === 'notBlank' || filter.type === 'serverNotBlank') {
238-
return `${colId} IS NOT NULL`;
227+
if (
228+
filter.type === FILTER_OPERATORS.NOT_BLANK ||
229+
filter.type === FILTER_OPERATORS.SERVER_NOT_BLANK
230+
) {
231+
return `${colId} ${SQL_OPERATORS.IS_NOT_NULL}`;
239232
}
240233
}
241234

242235
if (filter.filterType === 'text' && filter.filter && filter.type) {
243-
const op = TEXT_FILTER_OPERATORS[filter.type];
244236
const escapedFilter = escapeStringValue(String(filter.filter));
245237
const val = getTextComparator(filter.type, escapedFilter);
246238

247-
return op === 'ILIKE' || op === 'NOT ILIKE'
248-
? `${colId} ${op} '${val}'`
249-
: `${colId} ${op} '${escapedFilter}'`;
239+
// Map text filter types to SQL operators
240+
switch (filter.type) {
241+
case FILTER_OPERATORS.EQUALS:
242+
return `${colId} ${SQL_OPERATORS.EQUALS} '${escapedFilter}'`;
243+
case FILTER_OPERATORS.NOT_EQUAL:
244+
return `${colId} ${SQL_OPERATORS.NOT_EQUALS} '${escapedFilter}'`;
245+
case FILTER_OPERATORS.CONTAINS:
246+
return `${colId} ${SQL_OPERATORS.ILIKE} '${val}'`;
247+
case FILTER_OPERATORS.NOT_CONTAINS:
248+
return `${colId} ${SQL_OPERATORS.NOT_ILIKE} '${val}'`;
249+
case FILTER_OPERATORS.STARTS_WITH:
250+
case FILTER_OPERATORS.ENDS_WITH:
251+
return `${colId} ${SQL_OPERATORS.ILIKE} '${val}'`;
252+
default:
253+
return null;
254+
}
250255
}
251256

252257
if (
253258
filter.filterType === 'number' &&
254259
filter.filter !== undefined &&
255260
filter.type
256261
) {
257-
const op = NUMBER_FILTER_OPERATORS[filter.type];
258-
return `${colId} ${op} ${filter.filter}`;
262+
// Map number filter types to SQL operators
263+
switch (filter.type) {
264+
case FILTER_OPERATORS.EQUALS:
265+
return `${colId} ${SQL_OPERATORS.EQUALS} ${filter.filter}`;
266+
case FILTER_OPERATORS.NOT_EQUAL:
267+
return `${colId} ${SQL_OPERATORS.NOT_EQUALS} ${filter.filter}`;
268+
case FILTER_OPERATORS.LESS_THAN:
269+
return `${colId} ${SQL_OPERATORS.LESS_THAN} ${filter.filter}`;
270+
case FILTER_OPERATORS.LESS_THAN_OR_EQUAL:
271+
return `${colId} ${SQL_OPERATORS.LESS_THAN_OR_EQUAL} ${filter.filter}`;
272+
case FILTER_OPERATORS.GREATER_THAN:
273+
return `${colId} ${SQL_OPERATORS.GREATER_THAN} ${filter.filter}`;
274+
case FILTER_OPERATORS.GREATER_THAN_OR_EQUAL:
275+
return `${colId} ${SQL_OPERATORS.GREATER_THAN_OR_EQUAL} ${filter.filter}`;
276+
default:
277+
return null;
278+
}
259279
}
260280

261281
// Handle date filters with proper date formatting and custom server operators

‎superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,18 +165,20 @@ function escapeSQLString(value: string): string {
165165
return value.replace(/'/g, "''");
166166
}
167167

168+
// Maximum column name length - conservative upper bound that exceeds all common
169+
// database identifier limits (MySQL: 64, PostgreSQL: 63, SQL Server: 128, Oracle: 128)
170+
const MAX_COLUMN_NAME_LENGTH = 255;
171+
168172
/**
169173
* Validates a column name to prevent SQL injection
170174
* Checks for: non-empty string, length limit, allowed characters
171-
* @param columnName - Column name to validate
172-
* @returns True if the column name is valid, false otherwise
173175
*/
174-
function validateColumnName(columnName: string): boolean {
176+
export function validateColumnName(columnName: string): boolean {
175177
if (!columnName || typeof columnName !== 'string') {
176178
return false;
177179
}
178180

179-
if (columnName.length > 255) {
181+
if (columnName.length > MAX_COLUMN_NAME_LENGTH) {
180182
return false;
181183
}
182184

0 commit comments

Comments
 (0)