bito-code-review[bot] commented on code in PR #35683: URL: https://github.com/apache/superset/pull/35683#discussion_r2670225536
########## superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts: ########## @@ -0,0 +1,863 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { convertAgGridFiltersToSQL } from '../../src/utils/agGridFilterConverter'; +import type { + AgGridFilterModel, + AgGridSimpleFilter, + AgGridCompoundFilter, + AgGridSetFilter, +} from '../../src/utils/agGridFilterConverter'; + +describe('agGridFilterConverter', () => { + describe('Empty and invalid inputs', () => { + it('should handle empty filter model', () => { + const result = convertAgGridFiltersToSQL({}); + + expect(result.simpleFilters).toEqual([]); + expect(result.complexWhere).toBeUndefined(); + expect(result.havingClause).toBeUndefined(); + }); + + it('should handle null filter model', () => { + const result = convertAgGridFiltersToSQL(null as any); + + expect(result.simpleFilters).toEqual([]); + expect(result.complexWhere).toBeUndefined(); + expect(result.havingClause).toBeUndefined(); + }); + + it('should skip invalid column names', () => { + const filterModel: AgGridFilterModel = { + valid_column: { + filterType: 'text', + type: 'equals', + filter: 'test', + }, + 'invalid; DROP TABLE users--': { + filterType: 'text', + type: 'equals', + filter: 'malicious', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0].col).toBe('valid_column'); + }); + + it('should skip filters with invalid objects', () => { + const filterModel = { + column1: null, + column2: 'invalid string', + column3: { + filterType: 'text', + type: 'equals', + filter: 'valid', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel as any); + + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0].col).toBe('column3'); + }); + }); + + describe('Simple text filters', () => { + it('should convert equals filter', () => { + const filterModel: AgGridFilterModel = { + name: { + filterType: 'text', + type: 'equals', + filter: 'John', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0]).toEqual({ + col: 'name', + op: '==', + val: 'John', + }); + }); + + it('should convert notEqual filter', () => { + const filterModel: AgGridFilterModel = { + status: { + filterType: 'text', + type: 'notEqual', + filter: 'inactive', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'status', + op: '!=', + val: 'inactive', + }); + }); + + it('should convert contains filter with wildcard', () => { + const filterModel: AgGridFilterModel = { + description: { + filterType: 'text', + type: 'contains', + filter: 'urgent', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'description', + op: 'ILIKE', + val: '%urgent%', + }); + }); + + it('should convert notContains filter with wildcard', () => { + const filterModel: AgGridFilterModel = { + description: { + filterType: 'text', + type: 'notContains', + filter: 'spam', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'description', + op: 'NOT ILIKE', + val: '%spam%', + }); + }); + + it('should convert startsWith filter with trailing wildcard', () => { + const filterModel: AgGridFilterModel = { + email: { + filterType: 'text', + type: 'startsWith', + filter: 'admin', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'email', + op: 'ILIKE', + val: 'admin%', + }); + }); + + it('should convert endsWith filter with leading wildcard', () => { + const filterModel: AgGridFilterModel = { + email: { + filterType: 'text', + type: 'endsWith', + filter: '@example.com', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'email', + op: 'ILIKE', + val: '%@example.com', + }); + }); + }); + + describe('Numeric filters', () => { + it('should convert lessThan filter', () => { + const filterModel: AgGridFilterModel = { + age: { + filterType: 'number', + type: 'lessThan', + filter: 30, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'age', + op: '<', + val: 30, + }); + }); + + it('should convert lessThanOrEqual filter', () => { + const filterModel: AgGridFilterModel = { + price: { + filterType: 'number', + type: 'lessThanOrEqual', + filter: 100, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'price', + op: '<=', + val: 100, + }); + }); + + it('should convert greaterThan filter', () => { + const filterModel: AgGridFilterModel = { + score: { + filterType: 'number', + type: 'greaterThan', + filter: 50, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'score', + op: '>', + val: 50, + }); + }); + + it('should convert greaterThanOrEqual filter', () => { + const filterModel: AgGridFilterModel = { + rating: { + filterType: 'number', + type: 'greaterThanOrEqual', + filter: 4, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'rating', + op: '>=', + val: 4, + }); + }); + + it('should convert inRange filter to BETWEEN', () => { + const filterModel: AgGridFilterModel = { + age: { + filterType: 'number', + type: 'inRange', + filter: 18, + filterTo: 65, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + // inRange creates a simple filter with BETWEEN operator + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0]).toEqual({ + col: 'age', + op: 'BETWEEN', + val: 18, + }); + }); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect BETWEEN test expectation</b></div> <div id="fix"> The inRange test expects val: 18 for BETWEEN, but BETWEEN requires two values. Implementation doesn't handle filterTo for simple filters, causing incorrect val. Update to val: [18, 65] or fix implementation to use complex where for ranges. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts @@ -279,7 +279,7 @@ expect(result.simpleFilters[0]).toEqual({ col: 'age', op: 'BETWEEN', - val: 18, + val: [18, 65], }); }); ``` </div> </details> </div> <small><i>Code Review Run #562fb9</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts: ########## @@ -0,0 +1,737 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +export type AgGridFilterType = 'text' | 'number' | 'date' | 'set' | 'boolean'; + +export type AgGridFilterOperator = + | 'equals' + | 'notEqual' + | 'contains' + | 'notContains' + | 'startsWith' + | 'endsWith' + | 'lessThan' + | 'lessThanOrEqual' + | 'greaterThan' + | 'greaterThanOrEqual' + | 'inRange' + | 'blank' + | 'notBlank' + // Custom server-side date filter operators (always pass client-side filtering) + | 'serverEquals' + | 'serverNotEqual' + | 'serverBefore' + | 'serverAfter' + | 'serverInRange' + | 'serverBlank' + | 'serverNotBlank'; + +export type AgGridLogicalOperator = 'AND' | 'OR'; + +export const FILTER_OPERATORS = { + EQUALS: 'equals' as const, + NOT_EQUAL: 'notEqual' as const, + CONTAINS: 'contains' as const, + NOT_CONTAINS: 'notContains' as const, + STARTS_WITH: 'startsWith' as const, + ENDS_WITH: 'endsWith' as const, + LESS_THAN: 'lessThan' as const, + LESS_THAN_OR_EQUAL: 'lessThanOrEqual' as const, + GREATER_THAN: 'greaterThan' as const, + GREATER_THAN_OR_EQUAL: 'greaterThanOrEqual' as const, + IN_RANGE: 'inRange' as const, + BLANK: 'blank' as const, + NOT_BLANK: 'notBlank' as const, + // Custom server-side date filter operators + SERVER_EQUALS: 'serverEquals' as const, + SERVER_NOT_EQUAL: 'serverNotEqual' as const, + SERVER_BEFORE: 'serverBefore' as const, + SERVER_AFTER: 'serverAfter' as const, + SERVER_IN_RANGE: 'serverInRange' as const, + SERVER_BLANK: 'serverBlank' as const, + SERVER_NOT_BLANK: 'serverNotBlank' as const, +} as const; + +export const SQL_OPERATORS = { + EQUALS: '==', + NOT_EQUALS: '!=', + ILIKE: 'ILIKE', + NOT_ILIKE: 'NOT ILIKE', + LESS_THAN: '<', + LESS_THAN_OR_EQUAL: '<=', + GREATER_THAN: '>', + GREATER_THAN_OR_EQUAL: '>=', + BETWEEN: 'BETWEEN', + IS_NULL: 'IS NULL', + IS_NOT_NULL: 'IS NOT NULL', + IN: 'IN', + TEMPORAL_RANGE: 'TEMPORAL_RANGE', +} as const; + +export type FilterValue = string | number | boolean | Date | null; + +const COLUMN_NAME_REGEX = /^[a-zA-Z0-9_. ()%*+\-/]+$/; + +export interface AgGridSimpleFilter { + filterType: AgGridFilterType; + type: AgGridFilterOperator; + filter?: FilterValue; + filterTo?: FilterValue; + // Date filter properties + dateFrom?: string | null; + dateTo?: string | null; +} + +export interface AgGridCompoundFilter { + filterType: AgGridFilterType; + operator: AgGridLogicalOperator; + condition1: AgGridSimpleFilter; + condition2: AgGridSimpleFilter; + conditions?: AgGridSimpleFilter[]; +} + +export interface AgGridSetFilter { + filterType: 'set'; + values: FilterValue[]; +} + +export type AgGridFilterModel = Record< + string, + AgGridSimpleFilter | AgGridCompoundFilter | AgGridSetFilter +>; + +export interface SQLAlchemyFilter { + col: string; + op: string; + val: FilterValue | FilterValue[]; +} + +export interface ConvertedFilter { + simpleFilters: SQLAlchemyFilter[]; + complexWhere?: string; + havingClause?: string; +} + +const AG_GRID_TO_SQLA_OPERATOR_MAP: Record<AgGridFilterOperator, string> = { + [FILTER_OPERATORS.EQUALS]: SQL_OPERATORS.EQUALS, + [FILTER_OPERATORS.NOT_EQUAL]: SQL_OPERATORS.NOT_EQUALS, + [FILTER_OPERATORS.CONTAINS]: SQL_OPERATORS.ILIKE, + [FILTER_OPERATORS.NOT_CONTAINS]: SQL_OPERATORS.NOT_ILIKE, + [FILTER_OPERATORS.STARTS_WITH]: SQL_OPERATORS.ILIKE, + [FILTER_OPERATORS.ENDS_WITH]: SQL_OPERATORS.ILIKE, + [FILTER_OPERATORS.LESS_THAN]: SQL_OPERATORS.LESS_THAN, + [FILTER_OPERATORS.LESS_THAN_OR_EQUAL]: SQL_OPERATORS.LESS_THAN_OR_EQUAL, + [FILTER_OPERATORS.GREATER_THAN]: SQL_OPERATORS.GREATER_THAN, + [FILTER_OPERATORS.GREATER_THAN_OR_EQUAL]: SQL_OPERATORS.GREATER_THAN_OR_EQUAL, + [FILTER_OPERATORS.IN_RANGE]: SQL_OPERATORS.BETWEEN, + [FILTER_OPERATORS.BLANK]: SQL_OPERATORS.IS_NULL, + [FILTER_OPERATORS.NOT_BLANK]: SQL_OPERATORS.IS_NOT_NULL, + // Server-side date filter operators (map to same SQL operators as standard ones) + [FILTER_OPERATORS.SERVER_EQUALS]: SQL_OPERATORS.EQUALS, + [FILTER_OPERATORS.SERVER_NOT_EQUAL]: SQL_OPERATORS.NOT_EQUALS, + [FILTER_OPERATORS.SERVER_BEFORE]: SQL_OPERATORS.LESS_THAN, + [FILTER_OPERATORS.SERVER_AFTER]: SQL_OPERATORS.GREATER_THAN, + [FILTER_OPERATORS.SERVER_IN_RANGE]: SQL_OPERATORS.BETWEEN, + [FILTER_OPERATORS.SERVER_BLANK]: SQL_OPERATORS.IS_NULL, + [FILTER_OPERATORS.SERVER_NOT_BLANK]: SQL_OPERATORS.IS_NOT_NULL, +}; + +/** + * Escapes single quotes in SQL strings to prevent SQL injection + * @param value - String value to escape + * @returns Escaped string safe for SQL queries + */ +function escapeSQLString(value: string): string { + return value.replace(/'/g, "''"); +} + +/** + * Validates a column name to prevent SQL injection + * Checks for: non-empty string, length limit, allowed characters + * @param columnName - Column name to validate + * @returns True if the column name is valid, false otherwise + */ +function validateColumnName(columnName: string): boolean { + if (!columnName || typeof columnName !== 'string') { + return false; + } + + if (columnName.length > 255) { + return false; + } + + if (!COLUMN_NAME_REGEX.test(columnName)) { + return false; + } + + return true; +} + +/** + * Validates a filter value for a given operator + * BLANK and NOT_BLANK operators don't require values + * @param value - Filter value to validate + * @param operator - AG Grid filter operator + * @returns True if the value is valid for the operator, false otherwise + */ +function validateFilterValue( + value: FilterValue | undefined, + operator: AgGridFilterOperator, +): boolean { + if ( + operator === FILTER_OPERATORS.BLANK || + operator === FILTER_OPERATORS.NOT_BLANK + ) { + return true; + } + + if (value === undefined) { + return false; + } + + const valueType = typeof value; + if ( + value !== null && + valueType !== 'string' && + valueType !== 'number' && + valueType !== 'boolean' && + !(value instanceof Date) + ) { + return false; + } + + return true; +} + +function formatValueForOperator( + operator: AgGridFilterOperator, + value: FilterValue, +): FilterValue { + if (typeof value === 'string') { + if ( + operator === FILTER_OPERATORS.CONTAINS || + operator === FILTER_OPERATORS.NOT_CONTAINS + ) { + return `%${value}%`; + } + if (operator === FILTER_OPERATORS.STARTS_WITH) { + return `${value}%`; + } + if (operator === FILTER_OPERATORS.ENDS_WITH) { + return `%${value}`; + } + } + return value; +} + +/** + * Convert a date filter to a WHERE clause + * @param columnName - Column name + * @param filter - AG Grid date filter + * @returns WHERE clause string for date filter + */ +function dateFilterToWhereClause( + columnName: string, + filter: AgGridSimpleFilter, +): string { + const { type, dateFrom, dateTo, filter: filterValue, filterTo } = filter; + + // Support both dateFrom/dateTo and filter/filterTo + const fromDate = dateFrom || (filterValue as string); + const toDate = dateTo || (filterTo as string); + + // Convert based on operator type + switch (type) { + case FILTER_OPERATORS.EQUALS: + if (!fromDate) return ''; + // For equals, check if date is within the full day range + return `(${columnName} >= '${getStartOfDay(fromDate)}' AND ${columnName} <= '${getEndOfDay(fromDate)}')`; + + case FILTER_OPERATORS.NOT_EQUAL: + if (!fromDate) return ''; + // For not equals, exclude the full day range + return `(${columnName} < '${getStartOfDay(fromDate)}' OR ${columnName} > '${getEndOfDay(fromDate)}')`; + + case FILTER_OPERATORS.LESS_THAN: + if (!fromDate) return ''; + return `${columnName} < '${getStartOfDay(fromDate)}'`; + + case FILTER_OPERATORS.LESS_THAN_OR_EQUAL: + if (!fromDate) return ''; + return `${columnName} <= '${getEndOfDay(fromDate)}'`; + + case FILTER_OPERATORS.GREATER_THAN: + if (!fromDate) return ''; + return `${columnName} > '${getEndOfDay(fromDate)}'`; + + case FILTER_OPERATORS.GREATER_THAN_OR_EQUAL: + if (!fromDate) return ''; + return `${columnName} >= '${getStartOfDay(fromDate)}'`; + + case FILTER_OPERATORS.IN_RANGE: + if (!fromDate || !toDate) return ''; + return `${columnName} ${SQL_OPERATORS.BETWEEN} '${getStartOfDay(fromDate)}' AND '${getEndOfDay(toDate)}'`; + + case FILTER_OPERATORS.BLANK: + return `${columnName} ${SQL_OPERATORS.IS_NULL}`; + + case FILTER_OPERATORS.NOT_BLANK: + return `${columnName} ${SQL_OPERATORS.IS_NOT_NULL}`; + + default: + return ''; + } +} + +function simpleFilterToWhereClause( + columnName: string, + filter: AgGridSimpleFilter, +): string { + // Check if this is a date filter and handle it specially + if (filter.filterType === 'date') { + return dateFilterToWhereClause(columnName, filter); + } + + const { type, filter: value, filterTo } = filter; + + const operator = AG_GRID_TO_SQLA_OPERATOR_MAP[type]; + if (!operator) { + return ''; + } + + if (!validateFilterValue(value, type)) { + return ''; + } + + if (type === FILTER_OPERATORS.BLANK) { + return `${columnName} ${SQL_OPERATORS.IS_NULL}`; + } + + if (type === FILTER_OPERATORS.NOT_BLANK) { + return `${columnName} ${SQL_OPERATORS.IS_NOT_NULL}`; + } + + if (value === null || value === undefined) { + return ''; + } + + if (type === FILTER_OPERATORS.IN_RANGE && filterTo !== undefined) { + return `${columnName} ${SQL_OPERATORS.BETWEEN} ${value} AND ${filterTo}`; + } + + const formattedValue = formatValueForOperator(type, value!); + + if ( + operator === SQL_OPERATORS.ILIKE || + operator === SQL_OPERATORS.NOT_ILIKE + ) { + return `${columnName} ${operator} '${escapeSQLString(String(formattedValue))}'`; + } + + if (typeof formattedValue === 'string') { + return `${columnName} ${operator} '${escapeSQLString(formattedValue)}'`; + } + + return `${columnName} ${operator} ${formattedValue}`; +} + +function isCompoundFilter( + filter: AgGridSimpleFilter | AgGridCompoundFilter | AgGridSetFilter, +): filter is AgGridCompoundFilter { + return ( + 'operator' in filter && ('condition1' in filter || 'conditions' in filter) + ); +} + +function isSetFilter( Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing main conversion function</b></div> <div id="fix"> The file defines utility functions and types but lacks a main conversion function (e.g., convertAgGridFilters). Without it, the module is incomplete for use in filter processing. </div> </div> <div id="suggestion"> <div id="issue"><b>Unquoted column names in SQL</b></div> <div id="fix"> Column names are used unquoted in raw SQL construction (e.g., `${columnName} >= ...`), risking SQL injection if column names contain special characters or spaces. Although validateColumnName restricts characters, proper quoting is essential for security and correctness. </div> </div> <div id="suggestion"> <div id="issue"><b>Missing validation for filterTo in BETWEEN</b></div> <div id="fix"> For IN_RANGE (BETWEEN), filterTo is not validated or escaped, unlike value. If filterTo is a string, it could cause SQL injection or errors. Call validateFilterValue on filterTo and escape if string. </div> </div> <small><i>Code Review Run #562fb9</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts: ########## @@ -0,0 +1,863 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { convertAgGridFiltersToSQL } from '../../src/utils/agGridFilterConverter'; +import type { + AgGridFilterModel, + AgGridSimpleFilter, + AgGridCompoundFilter, + AgGridSetFilter, +} from '../../src/utils/agGridFilterConverter'; + +describe('agGridFilterConverter', () => { + describe('Empty and invalid inputs', () => { + it('should handle empty filter model', () => { + const result = convertAgGridFiltersToSQL({}); + + expect(result.simpleFilters).toEqual([]); + expect(result.complexWhere).toBeUndefined(); + expect(result.havingClause).toBeUndefined(); + }); + + it('should handle null filter model', () => { + const result = convertAgGridFiltersToSQL(null as any); + + expect(result.simpleFilters).toEqual([]); + expect(result.complexWhere).toBeUndefined(); + expect(result.havingClause).toBeUndefined(); + }); + + it('should skip invalid column names', () => { + const filterModel: AgGridFilterModel = { + valid_column: { + filterType: 'text', + type: 'equals', + filter: 'test', + }, + 'invalid; DROP TABLE users--': { + filterType: 'text', + type: 'equals', + filter: 'malicious', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0].col).toBe('valid_column'); + }); + + it('should skip filters with invalid objects', () => { + const filterModel = { + column1: null, + column2: 'invalid string', + column3: { + filterType: 'text', + type: 'equals', + filter: 'valid', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel as any); + + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0].col).toBe('column3'); + }); + }); + + describe('Simple text filters', () => { + it('should convert equals filter', () => { + const filterModel: AgGridFilterModel = { + name: { + filterType: 'text', + type: 'equals', + filter: 'John', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0]).toEqual({ + col: 'name', + op: '==', + val: 'John', + }); + }); + + it('should convert notEqual filter', () => { + const filterModel: AgGridFilterModel = { + status: { + filterType: 'text', + type: 'notEqual', + filter: 'inactive', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'status', + op: '!=', + val: 'inactive', + }); + }); + + it('should convert contains filter with wildcard', () => { + const filterModel: AgGridFilterModel = { + description: { + filterType: 'text', + type: 'contains', + filter: 'urgent', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'description', + op: 'ILIKE', + val: '%urgent%', + }); + }); + + it('should convert notContains filter with wildcard', () => { + const filterModel: AgGridFilterModel = { + description: { + filterType: 'text', + type: 'notContains', + filter: 'spam', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'description', + op: 'NOT ILIKE', + val: '%spam%', + }); + }); + + it('should convert startsWith filter with trailing wildcard', () => { + const filterModel: AgGridFilterModel = { + email: { + filterType: 'text', + type: 'startsWith', + filter: 'admin', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'email', + op: 'ILIKE', + val: 'admin%', + }); + }); + + it('should convert endsWith filter with leading wildcard', () => { + const filterModel: AgGridFilterModel = { + email: { + filterType: 'text', + type: 'endsWith', + filter: '@example.com', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'email', + op: 'ILIKE', + val: '%@example.com', + }); + }); + }); + + describe('Numeric filters', () => { + it('should convert lessThan filter', () => { + const filterModel: AgGridFilterModel = { + age: { + filterType: 'number', + type: 'lessThan', + filter: 30, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'age', + op: '<', + val: 30, + }); + }); + + it('should convert lessThanOrEqual filter', () => { + const filterModel: AgGridFilterModel = { + price: { + filterType: 'number', + type: 'lessThanOrEqual', + filter: 100, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'price', + op: '<=', + val: 100, + }); + }); + + it('should convert greaterThan filter', () => { + const filterModel: AgGridFilterModel = { + score: { + filterType: 'number', + type: 'greaterThan', + filter: 50, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'score', + op: '>', + val: 50, + }); + }); + + it('should convert greaterThanOrEqual filter', () => { + const filterModel: AgGridFilterModel = { + rating: { + filterType: 'number', + type: 'greaterThanOrEqual', + filter: 4, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'rating', + op: '>=', + val: 4, + }); + }); + + it('should convert inRange filter to BETWEEN', () => { + const filterModel: AgGridFilterModel = { + age: { + filterType: 'number', + type: 'inRange', + filter: 18, + filterTo: 65, + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + // inRange creates a simple filter with BETWEEN operator + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0]).toEqual({ + col: 'age', + op: 'BETWEEN', + val: 18, + }); + }); + }); + + describe('Null/blank filters', () => { + it('should convert blank filter to IS NULL', () => { + const filterModel: AgGridFilterModel = { + optional_field: { + filterType: 'text', + type: 'blank', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'optional_field', + op: 'IS NULL', + val: null, + }); + }); + + it('should convert notBlank filter to IS NOT NULL', () => { + const filterModel: AgGridFilterModel = { + required_field: { + filterType: 'text', + type: 'notBlank', + }, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'required_field', + op: 'IS NOT NULL', + val: null, + }); + }); + }); + + describe('Set filters', () => { + it('should convert set filter to IN operator', () => { + const filterModel: AgGridFilterModel = { + status: { + filterType: 'set', + values: ['active', 'pending', 'approved'], + } as AgGridSetFilter, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'status', + op: 'IN', + val: ['active', 'pending', 'approved'], + }); + }); + + it('should handle set filter with numeric values', () => { + const filterModel: AgGridFilterModel = { + priority: { + filterType: 'set', + values: [1, 2, 3], + } as AgGridSetFilter, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters[0]).toEqual({ + col: 'priority', + op: 'IN', + val: [1, 2, 3], + }); + }); + + it('should skip empty set filters', () => { + const filterModel: AgGridFilterModel = { + status: { + filterType: 'set', + values: [], + } as AgGridSetFilter, + }; + + const result = convertAgGridFiltersToSQL(filterModel); + + expect(result.simpleFilters).toHaveLength(0); + }); + }); + + describe('Compound filters', () => { + it('should combine conditions with AND operator', () => { + const filterModel: AgGridFilterModel = { + age: { + filterType: 'number', + operator: 'AND', Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete compound filter test</b></div> <div id="fix"> Compound filter test lacks assertions. Add checks for result.complexWhere to match 'age > 18 AND age < 65' or similar, ensuring AND operator works. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts @@ -374,6 +374,10 @@ const result = convertAgGridFiltersToSQL(filterModel); - // TODO: Add assertions + expect(result.simpleFilters).toHaveLength(0); + expect(result.complexWhere).toBe('(age > 18 AND age < 65)'); + expect(result.havingClause).toBeUndefined(); }); }); ``` </div> </details> </div> <small><i>Code Review Run #562fb9</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/filterStateManager.test.ts: ########## @@ -0,0 +1,658 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { getCompleteFilterState } from '../../src/utils/filterStateManager'; +import type { RefObject } from 'react'; +import type { AgGridReact } from '@superset-ui/core/components/ThemedAgGridReact'; +import { FILTER_INPUT_POSITIONS } from '../../src/consts'; + +describe('filterStateManager', () => { + describe('getCompleteFilterState', () => { + it('should return empty state when gridRef.current is null', async () => { + const gridRef = { current: null } as RefObject<AgGridReact>; + + const result = await getCompleteFilterState(gridRef, []); + + expect(result).toEqual({ + originalFilterModel: {}, + simpleFilters: [], + complexWhere: undefined, + havingClause: undefined, + lastFilteredColumn: undefined, + inputPosition: FILTER_INPUT_POSITIONS.UNKNOWN, + }); + }); + + it('should return empty state when gridRef.current.api is undefined', async () => { + const gridRef = { + current: { api: undefined } as any, + } as RefObject<AgGridReact>; + + const result = await getCompleteFilterState(gridRef, []); + + expect(result).toEqual({ + originalFilterModel: {}, + simpleFilters: [], + complexWhere: undefined, + havingClause: undefined, + lastFilteredColumn: undefined, + inputPosition: FILTER_INPUT_POSITIONS.UNKNOWN, + }); + }); + + it('should convert simple filters correctly', async () => { + const filterModel = { + name: { filterType: 'text', type: 'equals', filter: 'John' }, + age: { filterType: 'number', type: 'greaterThan', filter: 25 }, + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => Promise.resolve(null)), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + const result = await getCompleteFilterState(gridRef, []); + + expect(result.originalFilterModel).toEqual(filterModel); + expect(result.simpleFilters).toHaveLength(2); + expect(result.simpleFilters[0]).toEqual({ + col: 'name', + op: '==', + val: 'John', + }); + expect(result.simpleFilters[1]).toEqual({ + col: 'age', + op: '>', + val: 25, + }); + }); + + it('should separate dimension and metric filters', async () => { + const filterModel = { + state: { filterType: 'text', type: 'equals', filter: 'CA' }, + 'SUM(revenue)': { + filterType: 'number', + type: 'greaterThan', + filter: 1000, + }, + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => Promise.resolve(null)), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + const result = await getCompleteFilterState(gridRef, ['SUM(revenue)']); + + // Dimension filter goes to simpleFilters + expect(result.simpleFilters).toHaveLength(1); + expect(result.simpleFilters[0].col).toBe('state'); + + // Metric filter goes to havingClause + expect(result.havingClause).toBe('SUM(revenue) > 1000'); + }); + + it('should detect first input position when active element is in first condition body', async () => { + const filterModel = { + name: { filterType: 'text', type: 'equals', filter: 'test' }, + }; + + const mockInput = document.createElement('input'); + const mockConditionBody1 = document.createElement('div'); + mockConditionBody1.appendChild(mockInput); + + const mockFilterInstance = { + eGui: document.createElement('div'), + eConditionBodies: [mockConditionBody1], + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => + Promise.resolve(mockFilterInstance), + ), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + // Mock activeElement + Object.defineProperty(document, 'activeElement', { + writable: true, + configurable: true, + value: mockInput, + }); + + const result = await getCompleteFilterState(gridRef, []); + + expect(result.lastFilteredColumn).toBe('name'); + expect(result.inputPosition).toBe(FILTER_INPUT_POSITIONS.FIRST); + }); + + it('should detect second input position when active element is in second condition body', async () => { + const filterModel = { + age: { + filterType: 'number', + operator: 'AND', + condition1: { filterType: 'number', type: 'greaterThan', filter: 18 }, + condition2: { filterType: 'number', type: 'lessThan', filter: 65 }, + }, + }; + + const mockInput = document.createElement('input'); + const mockConditionBody1 = document.createElement('div'); + const mockConditionBody2 = document.createElement('div'); + mockConditionBody2.appendChild(mockInput); + + const mockFilterInstance = { + eGui: document.createElement('div'), + eConditionBodies: [mockConditionBody1, mockConditionBody2], + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => + Promise.resolve(mockFilterInstance), + ), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + // Mock activeElement + Object.defineProperty(document, 'activeElement', { + writable: true, + configurable: true, + value: mockInput, + }); + + const result = await getCompleteFilterState(gridRef, []); + + expect(result.lastFilteredColumn).toBe('age'); + expect(result.inputPosition).toBe(FILTER_INPUT_POSITIONS.SECOND); + }); + + it('should return unknown position when active element is not in any condition body', async () => { + const filterModel = { + name: { filterType: 'text', type: 'equals', filter: 'test' }, + }; + + const mockConditionBody = document.createElement('div'); + const mockFilterInstance = { + eGui: document.createElement('div'), + eConditionBodies: [mockConditionBody], + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => + Promise.resolve(mockFilterInstance), + ), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + // Mock activeElement as something outside the filter + const outsideElement = document.createElement('div'); + Object.defineProperty(document, 'activeElement', { + writable: true, + configurable: true, + value: outsideElement, + }); + + const result = await getCompleteFilterState(gridRef, []); + + expect(result.inputPosition).toBe(FILTER_INPUT_POSITIONS.UNKNOWN); + expect(result.lastFilteredColumn).toBeUndefined(); + }); + + it('should handle multiple filtered columns and detect the correct one', async () => { + const filterModel = { + name: { filterType: 'text', type: 'equals', filter: 'John' }, + age: { filterType: 'number', type: 'greaterThan', filter: 25 }, + status: { filterType: 'text', type: 'equals', filter: 'active' }, + }; + + const mockInput = document.createElement('input'); + const mockConditionBodyAge = document.createElement('div'); + mockConditionBodyAge.appendChild(mockInput); + + const mockFilterInstanceName = { + eGui: document.createElement('div'), + eConditionBodies: [document.createElement('div')], + }; + + const mockFilterInstanceAge = { + eGui: document.createElement('div'), + eConditionBodies: [mockConditionBodyAge], + }; + + const mockFilterInstanceStatus = { + eGui: document.createElement('div'), + eConditionBodies: [document.createElement('div')], + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn((colId: string) => { + if (colId === 'name') return Promise.resolve(mockFilterInstanceName); + if (colId === 'age') return Promise.resolve(mockFilterInstanceAge); + if (colId === 'status') + return Promise.resolve(mockFilterInstanceStatus); + return Promise.resolve(null); + }), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + // Mock activeElement in age filter + Object.defineProperty(document, 'activeElement', { + writable: true, + configurable: true, + value: mockInput, + }); + + const result = await getCompleteFilterState(gridRef, []); + + expect(result.lastFilteredColumn).toBe('age'); + expect(result.inputPosition).toBe(FILTER_INPUT_POSITIONS.FIRST); + }); + + it('should handle filter instance without eConditionBodies', async () => { + const filterModel = { + name: { filterType: 'text', type: 'equals', filter: 'test' }, + }; + + const mockFilterInstance = { + eGui: document.createElement('div'), + // No eConditionBodies property + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => + Promise.resolve(mockFilterInstance), + ), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + const result = await getCompleteFilterState(gridRef, []); + + expect(result.inputPosition).toBe(FILTER_INPUT_POSITIONS.UNKNOWN); + expect(result.lastFilteredColumn).toBeUndefined(); + }); + + it('should handle empty filter model', async () => { + const filterModel = {}; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => Promise.resolve(null)), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + + const result = await getCompleteFilterState(gridRef, []); + + expect(result.originalFilterModel).toEqual({}); + expect(result.simpleFilters).toEqual([]); + expect(result.complexWhere).toBeUndefined(); + expect(result.havingClause).toBeUndefined(); + expect(result.inputPosition).toBe(FILTER_INPUT_POSITIONS.UNKNOWN); + }); + + it('should handle compound filters correctly', async () => { + const filterModel = { + age: { + filterType: 'number', + operator: 'AND', + condition1: { + filterType: 'number', + type: 'greaterThanOrEqual', + filter: 18, + }, + condition2: { filterType: 'number', type: 'lessThan', filter: 65 }, + }, + }; + + const mockApi = { + getFilterModel: jest.fn(() => filterModel), + getColumnFilterInstance: jest.fn(() => Promise.resolve(null)), + }; + + const gridRef = { + current: { api: mockApi } as any, + } as RefObject<AgGridReact>; + Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete test assertions</b></div> <div id="fix"> The compound filters test case sets up mocks and calls the function but lacks any expect statements to verify the output. Based on the source code, compound filters generate a complexWhere clause like 'age >= 18 AND age < 65' since no metric columns are specified. Add assertions to ensure the test validates this behavior. </div> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/4cc0aa4/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run #562fb9</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
