codeant-ai-for-open-source[bot] commented on code in PR #35683:
URL: https://github.com/apache/superset/pull/35683#discussion_r2626794009
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts:
##########
@@ -27,3 +27,18 @@ export const PAGE_SIZE_OPTIONS = [10, 20, 50, 100, 200];
export const CUSTOM_AGG_FUNCS = {
queryTotal: 'Metric total',
};
+
+export const FILTER_POPOVER_OPEN_DELAY = 200;
+export const FILTER_INPUT_SELECTOR = 'input[data-ref="eInput"]';
+export const NOOP_FILTER_COMPARATOR = () => 0;
+
+export const FILTER_INPUT_POSITIONS = {
+ FIRST: 'first' as const,
+ SECOND: 'second' as const,
+ UNKNOWN: 'unknown' as const,
+} as const;
+
Review Comment:
**Suggestion:** `FILTER_INPUT_POSITIONS` is defined as a const object but no
exported type alias is provided; callers that need the union type of the
allowed values will have to repeat type logic or may accidentally widen the
type, leading to inconsistent usage. Export an explicit type alias derived from
the const to ensure consistent, type-safe usage across the codebase. [type
error]
**Severity Level:** Minor ⚠️
```suggestion
export type FilterInputPosition = typeof FILTER_INPUT_POSITIONS[keyof typeof
FILTER_INPUT_POSITIONS];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Exporting a derived union type (e.g., FilterInputPosition) is a sensible,
non-breaking enhancement that makes downstream usage type-safe and avoids
repeated type derivations. It doesn't change runtime behavior and helps
maintainers.
</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/consts.ts
**Line:** 40:40
**Comment:**
*Type Error: `FILTER_INPUT_POSITIONS` is defined as a const object but
no exported type alias is provided; callers that need the union type of the
allowed values will have to repeat type logic or may accidentally widen the
type, leading to inconsistent usage. Export an explicit type alias derived from
the const to ensure consistent, type-safe usage across the codebase.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts:
##########
@@ -27,3 +27,18 @@ export const PAGE_SIZE_OPTIONS = [10, 20, 50, 100, 200];
export const CUSTOM_AGG_FUNCS = {
queryTotal: 'Metric total',
};
+
+export const FILTER_POPOVER_OPEN_DELAY = 200;
+export const FILTER_INPUT_SELECTOR = 'input[data-ref="eInput"]';
+export const NOOP_FILTER_COMPARATOR = () => 0;
Review Comment:
**Suggestion:** The no-op comparator is defined without parameters and
without an explicit comparator type: `() => 0`. Comparator consumers expect a
function of two arguments that returns a number. Defining it without parameters
can cause TypeScript signature mismatches and makes intent unclear; also, some
call-sites may rely on the parameters being present. Add two parameters with
appropriate types and an explicit return type to match comparator signatures.
[type error]
**Severity Level:** Minor ⚠️
```suggestion
export const NOOP_FILTER_COMPARATOR = (_a: unknown, _b: unknown): number =>
0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Making the noop comparator explicitly accept two arguments and return a
number improves its type compatibility with typical comparator signatures and
documents intent.
The current () => 0 is valid JS/TS but can lead to subtle typing mismatches
at call sites that expect (a, b) => number. The proposed change is small, safe,
and doesn't change runtime behavior.
</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/consts.ts
**Line:** 33:33
**Comment:**
*Type Error: The no-op comparator is defined without parameters and
without an explicit comparator type: `() => 0`. Comparator consumers expect a
function of two arguments that returns a number. Defining it without parameters
can cause TypeScript signature mismatches and makes intent unclear; also, some
call-sites may rely on the parameters being present. Add two parameters with
appropriate types and an explicit return type to match comparator signatures.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts:
##########
@@ -68,6 +95,101 @@ function getTextComparator(type: string, value: string):
string {
return value;
}
+/**
+ * Format a date string to ISO format for SQL queries, preserving local
timezone.
+ * @param dateStr - Date string from AG Grid filter
+ * @returns ISO formatted date string in local timezone
+ */
+function formatDateForSQL(dateStr: string): string {
+ const date = new Date(dateStr);
+ if (Number.isNaN(date.getTime())) {
+ return dateStr;
+ }
+
+ const year = date.getFullYear();
+ const month = String(date.getMonth() + 1).padStart(2, '0');
+ const day = String(date.getDate()).padStart(2, '0');
+ const hours = String(date.getHours()).padStart(2, '0');
+ const minutes = String(date.getMinutes()).padStart(2, '0');
+ const seconds = String(date.getSeconds()).padStart(2, '0');
+
+ return `${year}-${month}-${day}T${hours}:${minutes}:${seconds}`;
+}
+
+/**
+ * Get the start of day (00:00:00) for a given date string
+ */
+function getStartOfDay(dateStr: string): string {
+ const date = new Date(dateStr);
+ date.setHours(0, 0, 0, 0);
+ return formatDateForSQL(date.toISOString());
+}
+
+/**
+ * Get the end of day (23:59:59) for a given date string
+ */
+function getEndOfDay(dateStr: string): string {
+ const date = new Date(dateStr);
+ date.setHours(23, 59, 59, 999);
+ return formatDateForSQL(date.toISOString());
+}
+
+/**
+ * Converts a date filter to SQL clause.
+ * Handles both standard operators (equals, lessThan, etc.) and
+ * custom server-side operators (serverEquals, serverBefore, etc.).
+ *
+ * @param colId - Column identifier
+ * @param filter - AG Grid date filter object
+ * @returns SQL clause string or null if conversion not possible
+ */
+function convertDateFilterToSQL(
+ colId: string,
+ filter: AgGridFilter,
+): string | null {
+ const { type, dateFrom, dateTo } = filter;
+
+ if (!type) return null;
+
+ // Map custom server operators to standard ones
Review Comment:
**Suggestion:** Security/robustness: `convertDateFilterToSQL` calls
`getStartOfDay`/`getEndOfDay` with `dateFrom`/`dateTo` without validating they
are parseable dates; if an attacker provides a crafted string that doesn't
parse, the formatter currently returns the raw input and it will be
interpolated directly into SQL, enabling SQL injection or malformed queries;
validate `dateFrom`/`dateTo` (reject or return null) before using them.
[security]
**Severity Level:** Critical 🚨
```suggestion
// Validate date inputs early to avoid embedding unparsed user input into
SQL
if (dateFrom && Number.isNaN(new Date(dateFrom).getTime())) return null;
if (dateTo && Number.isNaN(new Date(dateTo).getTime())) return null;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Adding early validation of dateFrom/dateTo before formatting is a sensible,
practical hardening: it prevents invalid or attacker-controlled strings from
reaching the formatter/SQL layer and avoids unexpected runtime errors. The
suggested checks are small, correct, and apply directly to the shown code path.
</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/stateConversion.ts
**Line:** 154:154
**Comment:**
*Security: Security/robustness: `convertDateFilterToSQL` calls
`getStartOfDay`/`getEndOfDay` with `dateFrom`/`dateTo` without validating they
are parseable dates; if an attacker provides a crafted string that doesn't
parse, the formatter currently returns the raw input and it will be
interpolated directly into SQL, enabling SQL injection or malformed queries;
validate `dateFrom`/`dateTo` (reject or return null) before using them.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -476,27 +476,594 @@ describe('plugin-chart-ag-grid-table', () => {
});
});
- describe('buildQuery - metrics handling in different query modes', () => {
- test('should not include metrics in raw records mode', () => {
- const query = buildQuery({
- viz_type: VizType.Table,
- datasource: '11__table',
- query_mode: QueryMode.Raw,
- all_columns: ['state', 'city'],
- }).queries[0];
+ describe('buildQuery - AG Grid server-side filters', () => {
+ describe('Simple filters', () => {
+ it('should apply agGridSimpleFilters to query.filters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [
+ { col: 'state', op: '==', val: 'CA' },
+ { col: 'city', op: 'ILIKE', val: '%San%' },
+ ],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toContainEqual({
Review Comment:
**Suggestion:** Missing null/undefined guard before asserting on
`query.filters` may cause matcher errors if `filters` is undefined; assert
`query.filters` is defined first before calling `toContainEqual`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(query.filters).toBeDefined();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Calling toContainEqual on an undefined value would throw during the test run.
Adding an explicit expect(query.filters).toBeDefined() makes the failure
clearer
(filters missing vs. contents mismatch) and prevents a TypeError. This
improves
test robustness and is directly applicable to the PR's tests.
</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/test/buildQuery.test.ts
**Line:** 497:497
**Comment:**
*Possible Bug: Missing null/undefined guard before asserting on
`query.filters` may cause matcher errors if `filters` is undefined; assert
`query.filters` is defined first before calling `toContainEqual`.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -332,6 +351,56 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps>
= memo(
[onColumnStateChange],
);
+ const handleFilterChanged = useCallback(async () => {
+ filterOperationVersionRef.current += 1;
+ const currentVersion = filterOperationVersionRef.current;
+
+ const completeFilterState = await getCompleteFilterState(
+ gridRef,
+ metricColumns,
+ );
+
+ // Skip stale operations from rapid filter changes
+ if (currentVersion !== filterOperationVersionRef.current) {
+ return;
+ }
+
+ // Reject invalid filter states (e.g., text filter on numeric column)
+ if (completeFilterState.originalFilterModel) {
+ const filterModel = completeFilterState.originalFilterModel;
+ const hasInvalidFilterType = Object.entries(filterModel).some(
+ ([colId, filter]: [string, any]) => {
+ if (
+ filter?.filterType === 'text' &&
+ metricColumns?.includes(colId)
+ ) {
+ return true;
+ }
+ return false;
+ },
+ );
+
+ if (hasInvalidFilterType) {
+ return;
+ }
+ }
+
+ if (
+ !isEqual(
+ serverPaginationData?.agGridFilterModel,
+ completeFilterState.originalFilterModel,
+ )
+ ) {
+ if (onFilterChanged) {
+ onFilterChanged(completeFilterState);
+ }
+ }
Review Comment:
**Suggestion:** The async `handleFilterChanged` has no try/catch around the
await for `getCompleteFilterState` (or the subsequent logic). If
`getCompleteFilterState` rejects or any synchronous step throws, the promise
will be rejected with an unhandled exception from an event handler, which can
surface as an uncaught promise rejection and break filter change handling. Wrap
the async body in a try/catch and log or swallow errors as appropriate. [error
handling bug]
**Severity Level:** Minor ⚠️
```suggestion
try {
const completeFilterState = await getCompleteFilterState(
gridRef,
metricColumns,
);
// Skip stale operations from rapid filter changes
if (currentVersion !== filterOperationVersionRef.current) {
return;
}
// Reject invalid filter states (e.g., text filter on numeric column)
if (completeFilterState.originalFilterModel) {
const filterModel = completeFilterState.originalFilterModel;
const hasInvalidFilterType = Object.entries(filterModel).some(
([colId, filter]: [string, any]) => {
if (
filter?.filterType === 'text' &&
metricColumns?.includes(colId)
) {
return true;
}
return false;
},
);
if (hasInvalidFilterType) {
return;
}
}
if (
!isEqual(
serverPaginationData?.agGridFilterModel,
completeFilterState.originalFilterModel,
)
) {
if (onFilterChanged) {
onFilterChanged(completeFilterState);
}
}
} catch (error) {
// Prevent unhandled rejections from breaking the grid event loop
// Keep logging minimal to avoid noise in production
// eslint-disable-next-line no-console
console.warn('Error processing AG Grid filter change:', error);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid and practical improvement. getCompleteFilterState is awaited
inside an async callback wired to ag-Grid events — if it rejects, the rejected
promise bubbles up as an unhandled rejection. Wrapping the body in try/catch
avoids noisy uncaught rejections and matches patterns used elsewhere in the
file (e.g., state capture). The proposed change is minimal and fixes a real
error-handling gap.
</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/AgGridTable/index.tsx
**Line:** 358:397
**Comment:**
*Error Handling Bug: The async `handleFilterChanged` has no try/catch
around the await for `getCompleteFilterState` (or the subsequent logic). If
`getCompleteFilterState` rejects or any synchronous step throws, the promise
will be rejected with an unhandled exception from an event handler, which can
surface as an uncaught promise rejection and break filter change handling. Wrap
the async body in a try/catch and log or swallow errors as appropriate.
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>
##########
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,
+ });
Review Comment:
**Suggestion:** Assertions rely on the order of `simpleFilters` elements
(index-based checks); because object key iteration order or conversion logic
may not guarantee a stable order, make the assertion order-independent by
checking the array contains both expected filter objects. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
expect(result.simpleFilters).toEqual(
expect.arrayContaining([
{ col: 'name', op: '==', val: 'John' },
{ col: 'age', op: '>', val: 25 },
]),
);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Making assertions order-independent (e.g. expect.arrayContaining) avoids
brittle tests if the conversion
implementation changes iteration order or internal sorting. While object
insertion order in JS is usually stable,
the conversion function might reorder filters; making the test tolerant is a
sensible improvement.
</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/test/utils/filterStateManager.test.ts
**Line:** 77:86
**Comment:**
*Logic Error: Assertions rely on the order of `simpleFilters` elements
(index-based checks); because object key iteration order or conversion logic
may not guarantee a stable order, make the assertion order-independent by
checking the array contains both expected filter objects.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -476,27 +476,594 @@ describe('plugin-chart-ag-grid-table', () => {
});
});
- describe('buildQuery - metrics handling in different query modes', () => {
- test('should not include metrics in raw records mode', () => {
- const query = buildQuery({
- viz_type: VizType.Table,
- datasource: '11__table',
- query_mode: QueryMode.Raw,
- all_columns: ['state', 'city'],
- }).queries[0];
+ describe('buildQuery - AG Grid server-side filters', () => {
+ describe('Simple filters', () => {
+ it('should apply agGridSimpleFilters to query.filters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [
+ { col: 'state', op: '==', val: 'CA' },
+ { col: 'city', op: 'ILIKE', val: '%San%' },
+ ],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toContainEqual({
+ col: 'state',
+ op: '==',
+ val: 'CA',
+ });
+ expect(query.filters).toContainEqual({
+ col: 'city',
+ op: 'ILIKE',
+ val: '%San%',
+ });
+ });
+
+ it('should append simple filters to existing filters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ adhoc_filters: [
+ {
+ expressionType: 'SIMPLE',
+ subject: 'country',
+ operator: '==',
+ comparator: 'USA',
+ clause: 'WHERE',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [{ col: 'state', op: '==', val: 'CA' }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters?.length).toBeGreaterThan(1);
Review Comment:
**Suggestion:** Using optional chaining in
`expect(query.filters?.length).toBeGreaterThan(1)` can hide that `filters` is
undefined and will cause the matcher to receive `undefined`; check
`query.filters` exists, then assert on `.length`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(query.filters).toBeDefined();
expect(query.filters.length).toBeGreaterThan(1);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using optional chaining here may feed undefined into Jest's matcher which is
confusing and can
mask whether filters were produced. Verifying filters is defined first and
then asserting on
.length produces clearer, deterministic test failures and is a safe,
targeted change.
</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/test/buildQuery.test.ts
**Line:** 531:531
**Comment:**
*Possible Bug: Using optional chaining in
`expect(query.filters?.length).toBeGreaterThan(1)` can hide that `filters` is
undefined and will cause the matcher to receive `undefined`; check
`query.filters` exists, then assert on `.length`.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -476,27 +476,594 @@ describe('plugin-chart-ag-grid-table', () => {
});
});
- describe('buildQuery - metrics handling in different query modes', () => {
- test('should not include metrics in raw records mode', () => {
- const query = buildQuery({
- viz_type: VizType.Table,
- datasource: '11__table',
- query_mode: QueryMode.Raw,
- all_columns: ['state', 'city'],
- }).queries[0];
+ describe('buildQuery - AG Grid server-side filters', () => {
+ describe('Simple filters', () => {
+ it('should apply agGridSimpleFilters to query.filters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [
+ { col: 'state', op: '==', val: 'CA' },
+ { col: 'city', op: 'ILIKE', val: '%San%' },
+ ],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toContainEqual({
+ col: 'state',
+ op: '==',
+ val: 'CA',
+ });
+ expect(query.filters).toContainEqual({
+ col: 'city',
+ op: 'ILIKE',
+ val: '%San%',
+ });
+ });
+
+ it('should append simple filters to existing filters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ adhoc_filters: [
+ {
+ expressionType: 'SIMPLE',
+ subject: 'country',
+ operator: '==',
+ comparator: 'USA',
+ clause: 'WHERE',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [{ col: 'state', op: '==', val: 'CA' }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters?.length).toBeGreaterThan(1);
+ expect(query.filters).toContainEqual({
+ col: 'state',
+ op: '==',
+ val: 'CA',
+ });
+ });
+
+ it('should handle empty agGridSimpleFilters array', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toBeDefined();
+ });
+
+ it('should not apply simple filters when server pagination is disabled',
() => {
+ const query = buildQuery(basicFormData, {
+ ownState: {
+ agGridSimpleFilters: [{ col: 'state', op: '==', val: 'CA' }],
+ },
+ }).queries[0];
- expect(query.metrics).toBeUndefined();
+ expect(query.filters).not.toContainEqual({
+ col: 'state',
+ op: '==',
+ val: 'CA',
+ });
+ });
});
- test('should set metrics to empty array in aggregate mode when no metrics
specified', () => {
- const query = buildQuery({
- viz_type: VizType.Table,
- datasource: '11__table',
- query_mode: QueryMode.Aggregate,
- groupby: ['state'],
- }).queries[0];
+ describe('Complex WHERE clause', () => {
+ it('should apply agGridComplexWhere to query.extras.where', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridComplexWhere: '(age > 18 AND age < 65)',
+ },
+ },
+ ).queries[0];
+
+ expect(query.extras?.where).toBe('(age > 18 AND age < 65)');
+ });
+
+ it('should combine with existing WHERE clause using AND', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ adhoc_filters: [
+ {
+ expressionType: 'SQL',
+ clause: 'WHERE',
+ sqlExpression: 'country = "USA"',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridComplexWhere: '(age > 18 AND age < 65)',
+ },
+ },
+ ).queries[0];
+
+ expect(query.extras?.where).toContain('country = "USA"');
+ expect(query.extras?.where).toContain('(age > 18 AND age < 65)');
+ expect(query.extras?.where).toContain(' AND ');
+ });
+
+ it('should handle empty agGridComplexWhere', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridComplexWhere: '',
+ },
+ },
+ ).queries[0];
+
+ // Empty string should not set extras.where (undefined or empty string
both acceptable)
+ expect(query.extras?.where || undefined).toBeUndefined();
+ });
+
+ it('should not apply WHERE clause when server pagination is disabled',
() => {
+ const query = buildQuery(basicFormData, {
+ ownState: {
+ agGridComplexWhere: '(age > 18)',
+ },
+ }).queries[0];
+
+ // When server_pagination is disabled, AG Grid filters should not be
applied
+ expect(query.extras?.where || undefined).toBeUndefined();
+ });
+ });
+
+ describe('HAVING clause', () => {
+ it('should apply agGridHavingClause to query.extras.having', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ metrics: ['SUM(revenue)'],
+ },
+ {
+ ownState: {
+ agGridHavingClause: 'SUM(revenue) > 1000',
+ },
+ },
+ ).queries[0];
+
+ expect(query.extras?.having).toBe('SUM(revenue) > 1000');
+ });
+
+ it('should combine with existing HAVING clause using AND', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ metrics: ['SUM(revenue)', 'COUNT(*)'],
+ adhoc_filters: [
+ {
+ expressionType: 'SQL',
+ clause: 'HAVING',
+ sqlExpression: 'COUNT(*) > 10',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridHavingClause: 'SUM(revenue) > 1000',
+ },
+ },
+ ).queries[0];
+
+ expect(query.extras?.having).toContain('COUNT(*) > 10');
+ expect(query.extras?.having).toContain('SUM(revenue) > 1000');
+ expect(query.extras?.having).toContain(' AND ');
+ });
+
+ it('should handle metric filters correctly', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ metrics: ['AVG(score)', 'MAX(points)'],
+ },
+ {
+ ownState: {
+ agGridHavingClause: '(AVG(score) >= 90 AND MAX(points) < 100)',
+ },
+ },
+ ).queries[0];
+
+ expect(query.extras?.having).toBe(
+ '(AVG(score) >= 90 AND MAX(points) < 100)',
+ );
+ });
+
+ it('should not apply HAVING clause when server pagination is disabled',
() => {
+ const query = buildQuery(basicFormData, {
+ ownState: {
+ agGridHavingClause: 'SUM(revenue) > 1000',
+ },
+ }).queries[0];
+
+ // When server_pagination is disabled, AG Grid filters should not be
applied
+ expect(query.extras?.having || undefined).toBeUndefined();
+ });
+ });
+
+ describe('Totals query handling', () => {
+ it('should exclude AG Grid WHERE filters from totals query', () => {
+ const queries = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ show_totals: true,
+ query_mode: QueryMode.Aggregate,
+ },
+ {
+ ownState: {
+ agGridComplexWhere: 'age > 18',
+ },
+ },
+ ).queries;
+
+ const mainQuery = queries[0];
+ const totalsQuery = queries[2]; // queries[1] is rowcount, queries[2]
is totals
+
+ expect(mainQuery.extras?.where).toBe('age > 18');
+ expect(totalsQuery.extras?.where).toBeUndefined();
+ });
+
+ it('should preserve non-AG Grid WHERE clauses in totals', () => {
+ const queries = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ show_totals: true,
+ query_mode: QueryMode.Aggregate,
+ adhoc_filters: [
+ {
+ expressionType: 'SQL',
+ clause: 'WHERE',
+ sqlExpression: 'country = "USA"',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridComplexWhere: 'age > 18',
+ },
+ },
+ ).queries;
+
+ const mainQuery = queries[0];
+ const totalsQuery = queries[2]; // queries[1] is rowcount, queries[2]
is totals
+
+ expect(mainQuery.extras?.where).toContain('country = "USA"');
+ expect(mainQuery.extras?.where).toContain('age > 18');
+ expect(totalsQuery.extras?.where).toContain('country = "USA"');
+ expect(totalsQuery.extras?.where).not.toContain('age > 18');
+ });
+
+ it('should handle totals when AG Grid WHERE is only clause', () => {
+ const queries = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ show_totals: true,
+ query_mode: QueryMode.Aggregate,
+ },
+ {
+ ownState: {
+ agGridComplexWhere: 'status = "active"',
+ },
+ },
+ ).queries;
+
+ const totalsQuery = queries[2]; // queries[1] is rowcount, queries[2]
is totals
+
+ expect(totalsQuery.extras?.where).toBeUndefined();
+ });
+
+ it('should handle totals with empty WHERE clause after removal', () => {
+ const queries = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ show_totals: true,
+ query_mode: QueryMode.Aggregate,
+ adhoc_filters: [
+ {
+ expressionType: 'SQL',
+ clause: 'WHERE',
+ sqlExpression: 'country = "USA"',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridComplexWhere: 'country = "USA"',
+ },
+ },
+ ).queries;
+
+ const totalsQuery = queries[2]; // queries[1] is rowcount, queries[2]
is totals
+
+ // After removing AG Grid WHERE, totals should still have the adhoc
filter
+ expect(totalsQuery.extras).toBeDefined();
+ });
+
+ it('should not modify totals query when no AG Grid filters applied', ()
=> {
+ const queries = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ show_totals: true,
+ query_mode: QueryMode.Aggregate,
+ },
+ {
+ ownState: {},
+ },
+ ).queries;
+
+ const totalsQuery = queries[2]; // queries[1] is rowcount, queries[2]
is totals
+
+ expect(totalsQuery.columns).toEqual([]);
+ expect(totalsQuery.row_limit).toBe(0);
+ });
+ });
+
+ describe('Integration - all filter types together', () => {
+ it('should apply simple, WHERE, and HAVING filters simultaneously', ()
=> {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ metrics: ['SUM(revenue)', 'COUNT(*)'],
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [{ col: 'state', op: '==', val: 'CA' }],
+ agGridComplexWhere: '(age > 18 AND age < 65)',
+ agGridHavingClause: 'SUM(revenue) > 1000',
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toContainEqual({
+ col: 'state',
+ op: '==',
+ val: 'CA',
+ });
+ expect(query.extras?.where).toBe('(age > 18 AND age < 65)');
+ expect(query.extras?.having).toBe('SUM(revenue) > 1000');
+ });
+
+ it('should combine AG Grid filters with adhoc filters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ adhoc_filters: [
+ {
+ expressionType: 'SIMPLE',
+ subject: 'country',
+ operator: '==',
+ comparator: 'USA',
+ clause: 'WHERE',
+ },
+ {
+ expressionType: 'SQL',
+ clause: 'WHERE',
+ sqlExpression: 'region = "West"',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [
+ { col: 'state', op: 'IN', val: ['CA', 'OR', 'WA'] },
+ ],
+ agGridComplexWhere: "status = 'active'",
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toContainEqual({
+ col: 'state',
+ op: 'IN',
+ val: ['CA', 'OR', 'WA'],
+ });
+ expect(query.extras?.where).toContain('region = "West"');
+ expect(query.extras?.where).toContain("status = 'active'");
+ });
+
+ it('should reset currentPage to 0 when filtering', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ currentPage: 5,
+ agGridSimpleFilters: [{ col: 'state', op: '==', val: 'CA' }],
+ },
+ },
+ ).queries[0];
+
+ // The query itself doesn't have page info, but ownState should be
updated
+ expect(query.filters).toContainEqual({
+ col: 'state',
+ op: '==',
+ val: 'CA',
+ });
+ });
+
+ it('should include filter metadata in ownState', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridFilterModel: {
+ state: { filterType: 'text', type: 'equals', filter: 'CA' },
+ },
+ lastFilteredColumn: 'state',
+ lastFilteredInputPosition: 'first',
+ },
+ },
+ ).queries[0];
+
+ // Query should be generated with the filter model metadata
+ expect(query).toBeDefined();
+ });
+
+ it('should handle complex real-world scenario', () => {
+ const queries = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ show_totals: true,
+ query_mode: QueryMode.Aggregate,
+ groupby: ['state', 'city'],
+ metrics: ['SUM(revenue)', 'AVG(score)', 'COUNT(*)'],
+ adhoc_filters: [
+ {
+ expressionType: 'SIMPLE',
+ subject: 'country',
+ operator: '==',
+ comparator: 'USA',
+ clause: 'WHERE',
+ },
+ ],
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [
+ { col: 'state', op: 'IN', val: ['CA', 'NY', 'TX'] },
+ ],
+ agGridComplexWhere:
+ '(population > 100000 AND growth_rate > 0.05)',
+ agGridHavingClause:
+ '(SUM(revenue) > 1000000 AND AVG(score) >= 4.5)',
+ currentPage: 0,
+ pageSize: 50,
+ },
+ },
+ ).queries;
+
+ const mainQuery = queries[0];
+ const totalsQuery = queries[2]; // queries[1] is rowcount, queries[2]
is totals
+
+ // Main query should have all filters
+ expect(mainQuery.filters).toContainEqual({
+ col: 'state',
+ op: 'IN',
+ val: ['CA', 'NY', 'TX'],
+ });
+ expect(mainQuery.extras?.where).toContain('population > 100000');
+ expect(mainQuery.extras?.having).toContain('SUM(revenue) > 1000000');
+
+ // Totals query should exclude AG Grid WHERE (since it's the only
WHERE clause, it should be undefined)
+ expect(totalsQuery.extras?.where).toBeUndefined();
+ });
+ });
+
+ describe('Edge cases', () => {
+ it('should handle null ownState gracefully', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {},
+ ).queries[0];
+
+ expect(query).toBeDefined();
+ });
+
+ it('should handle ownState without filter properties', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ currentPage: 0,
+ pageSize: 20,
+ },
+ },
+ ).queries[0];
- expect(query.metrics).toEqual([]);
+ expect(query).toBeDefined();
+ });
+
+ it('should handle filters with special SQL characters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ agGridSimpleFilters: [{ col: 'name', op: '==', val: "O'Brien" }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toContainEqual({
Review Comment:
**Suggestion:** The test asserts `toContainEqual` on `query.filters` without
ensuring it's defined; this can produce a misleading failure or runtime error
if `filters` is undefined — add an explicit `toBeDefined()` check first.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(query.filters).toBeDefined();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same pattern as the other suggestions — asserting on a possibly undefined
array will throw.
Adding a toBeDefined() before toContainEqual yields clearer failures and
avoids runtime errors
in the test suite if buildQuery ever returns undefined filters.
</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/test/buildQuery.test.ts
**Line:** 1040:1040
**Comment:**
*Possible Bug: The test asserts `toContainEqual` on `query.filters`
without ensuring it's defined; this can produce a misleading failure or runtime
error if `filters` is undefined — add an explicit `toBeDefined()` check first.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts:
##########
@@ -0,0 +1,734 @@
+/**
+ * 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: '==',
Review Comment:
**Suggestion:** Logic error: SQL equality operator is incorrect —
`SQL_OPERATORS.EQUALS` is set to '==' (JavaScript equality) instead of SQL '=';
this will produce invalid SQL when equality comparisons are built. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
EQUALS: '=',
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a real logic/semantic bug: SQL uses '=' for equality, not '=='. The
PR maps AG Grid equals to SQL_OPERATORS.EQUALS and then emits that token into
generated SQL — producing invalid or nonstandard SQL. Replacing '==' with '='
fixes the generated SQL and is low risk.
</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/utils/agGridFilterConverter.ts
**Line:** 72:72
**Comment:**
*Logic Error: Logic error: SQL equality operator is incorrect —
`SQL_OPERATORS.EQUALS` is set to '==' (JavaScript equality) instead of SQL '=';
this will produce invalid SQL when equality comparisons are built.
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>
##########
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 () => {
Review Comment:
**Suggestion:** Tests mutate document.activeElement repeatedly without
restoring it, which can leak state between tests and cause flakiness; capture
the original activeElement in beforeEach and restore it in afterEach (use a
small helper to set activeElement). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
let originalActiveElement: Element | null = null;
const setActiveElement = (el: Element | null) => {
Object.defineProperty(document, 'activeElement', {
writable: true,
configurable: true,
value: el,
} as PropertyDescriptor);
};
beforeEach(() => {
originalActiveElement = document.activeElement;
});
afterEach(() => {
try {
setActiveElement(originalActiveElement);
} catch (e) {
// ignore environments where activeElement is not configurable
}
});
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The tests repeatedly mutate document.activeElement (Object.defineProperty)
and never restore it.
That can leak state between tests and cause flakiness in other tests that
rely on document.activeElement.
Adding a beforeEach/afterEach to capture and restore the original active
element is a targeted, correct fix
that reduces test coupling. The proposed improved code is applicable to the
PR and will make the suite more robust.
</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/test/utils/filterStateManager.test.ts
**Line:** 26:26
**Comment:**
*Possible Bug: Tests mutate document.activeElement repeatedly without
restoring it, which can leak state between tests and cause flakiness; capture
the original activeElement in beforeEach and restore it in afterEach (use a
small helper to set activeElement).
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:
##########
@@ -102,6 +104,73 @@ const getFilterType = (col: InputColumn) => {
}
};
+/**
+ * Filter value getter for temporal columns.
+ * Returns null for DateWithFormatter objects with null input,
+ * enabling AG Grid's blank filter to correctly identify null dates.
+ */
+const dateFilterValueGetter = (params: {
+ data: Record<string, unknown>;
+ colDef: { field?: string };
+}) => {
+ const value = params.data?.[params.colDef.field as string];
+ // Return null for DateWithFormatter with null input so AG Grid blank filter
works
+ if (value instanceof DateWithFormatter && value.input === null) {
+ return null;
Review Comment:
**Suggestion:** The current `dateFilterValueGetter` returns the
DateWithFormatter wrapper object for non-null values and uses `instanceof
DateWithFormatter` to detect it; this can cause AG Grid to receive a
non-primitive (wrapper) instead of the underlying Date/null and `instanceof`
can fail across realms or with plain objects. Return the underlying `input`
value when the wrapper is present and use a structural check to detect it.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
const field = params?.colDef?.field;
const value = field ? params.data?.[field] : undefined;
// If it's a wrapper-like object with `input`, return the underlying input
(Date or null)
if (value && typeof value === 'object' && 'input' in value) {
return (value as any).input === null ? null : (value as any).input;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This suggestion fixes a real interoperability issue: AG Grid date filters
expect a Date or null, not a wrapper object. The current code only returns null
for wrapper-with-null but returns the wrapper for non-null values, which can
break filtering or comparators and can fail `instanceof` across realms.
Returning the underlying `input` when present and using a structural check is
safer and directly addresses the bug.
</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/utils/useColDefs.ts
**Line:** 116:119
**Comment:**
*Logic Error: The current `dateFilterValueGetter` returns the
DateWithFormatter wrapper object for non-null values and uses `instanceof
DateWithFormatter` to detect it; this can cause AG Grid to receive a
non-primitive (wrapper) instead of the underlying Date/null and `instanceof`
can fail across realms or with plain objects. Return the underlying `input`
value when the wrapper is present and use a structural check to detect it.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts:
##########
@@ -0,0 +1,734 @@
+/**
+ * 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(
+ filter: AgGridSimpleFilter | AgGridCompoundFilter | AgGridSetFilter,
+): filter is AgGridSetFilter {
+ return filter.filterType === 'set' && 'values' in filter;
+}
+
+function compoundFilterToWhereClause(
+ columnName: string,
+ filter: AgGridCompoundFilter,
+): string {
+ const { operator, condition1, condition2, conditions } = filter;
+
+ if (conditions && conditions.length > 0) {
+ const clauses = conditions
+ .map(cond => {
+ const clause = simpleFilterToWhereClause(columnName, cond);
+
+ return clause;
+ })
+ .filter(clause => clause !== '');
+
+ if (clauses.length === 0) {
+ return '';
+ }
+
+ if (clauses.length === 1) {
+ return clauses[0];
+ }
+
+ const result = `(${clauses.join(` ${operator} `)})`;
+
+ return result;
+ }
+
+ const clause1 = simpleFilterToWhereClause(columnName, condition1);
+ const clause2 = simpleFilterToWhereClause(columnName, condition2);
+
+ if (!clause1 && !clause2) {
+ return '';
+ }
+
+ if (!clause1) {
+ return clause2;
+ }
+
+ if (!clause2) {
+ return clause1;
+ }
+
+ const result = `(${clause1} ${operator} ${clause2})`;
+ return result;
+}
+
+/**
+ * Format a date string to ISO format expected by Superset, preserving local
timezone
+ * @param dateStr - Date string from AG Grid filter
+ * @returns ISO formatted date string in local timezone
+ */
+function formatDateForSuperset(dateStr: string): string {
+ // AG Grid typically provides dates in format: "YYYY-MM-DD HH:MM:SS"
+ // Superset expects: "YYYY-MM-DDTHH:MM:SS" in local timezone (not UTC)
+ const date = new Date(dateStr);
+ if (Number.isNaN(date.getTime())) {
+ return dateStr; // Return as-is if invalid
+ }
+
+ // Format date in local timezone, not UTC
+ const year = date.getFullYear();
+ const month = String(date.getMonth() + 1).padStart(2, '0');
+ const day = String(date.getDate()).padStart(2, '0');
+ const hours = String(date.getHours()).padStart(2, '0');
+ const minutes = String(date.getMinutes()).padStart(2, '0');
+ const seconds = String(date.getSeconds()).padStart(2, '0');
+
+ const formatted = `${year}-${month}-${day}T${hours}:${minutes}:${seconds}`;
+ return formatted;
+}
+
+/**
+ * Get the start of day for a given date string
+ * @param dateStr - Date string from AG Grid filter
+ * @returns ISO formatted date string at 00:00:00
+ */
+function getStartOfDay(dateStr: string): string {
+ const date = new Date(dateStr);
+ date.setHours(0, 0, 0, 0);
+ return formatDateForSuperset(date.toISOString());
+}
+
+/**
+ * Get the end of day for a given date string
+ * @param dateStr - Date string from AG Grid filter
+ * @returns ISO formatted date string at 23:59:59
+ */
+function getEndOfDay(dateStr: string): string {
+ const date = new Date(dateStr);
+ date.setHours(23, 59, 59, 999);
+ return formatDateForSuperset(date.toISOString());
Review Comment:
**Suggestion:** Timezone/round-trip bug: `getStartOfDay` and `getEndOfDay`
build a Date, mutate it to local midnight/end, then pass date.toISOString()
into `formatDateForSuperset` which re-parses the UTC ISO string and converts
back to local time — this can shift the day and produce incorrect start/end
timestamps; format and return the local-date ISO directly from the Date object
instead. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const year = date.getFullYear();
const month = String(date.getMonth() + 1).padStart(2, '0');
const day = String(date.getDate()).padStart(2, '0');
const hours = '00';
const minutes = '00';
const seconds = '00';
return `${year}-${month}-${day}T${hours}:${minutes}:${seconds}`;
}
/**
* Get the end of day for a given date string
* @param dateStr - Date string from AG Grid filter
* @returns ISO formatted date string at 23:59:59
*/
function getEndOfDay(dateStr: string): string {
const date = new Date(dateStr);
date.setHours(23, 59, 59, 999);
const year = date.getFullYear();
const month = String(date.getMonth() + 1).padStart(2, '0');
const day = String(date.getDate()).padStart(2, '0');
const hours = '23';
const minutes = '59';
const seconds = '59';
return `${year}-${month}-${day}T${hours}:${minutes}:${seconds}`;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The code creates a Date, mutates it to local midnight/end, then calls
toISOString() and passes that back into formatDateForSuperset which re-parses
it and interprets it as UTC -> local conversion, producing shifted timestamps.
That's a real timezone/round-trip bug. Formatting the local Date components
directly (or changing formatDateForSuperset to accept a Date) prevents the
shift.
</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/utils/agGridFilterConverter.ts
**Line:** 448:459
**Comment:**
*Logic Error: Timezone/round-trip bug: `getStartOfDay` and
`getEndOfDay` build a Date, mutate it to local midnight/end, then pass
date.toISOString() into `formatDateForSuperset` which re-parses the UTC ISO
string and converts back to local time — this can shift the day and produce
incorrect start/end timestamps; format and return the local-date ISO directly
from the Date object instead.
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>
##########
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,
+ });
Review Comment:
**Suggestion:** Repeated direct Object.defineProperty calls are duplicated
and error-prone; use a helper function (added in the top-level describe) to set
the active element so all tests consistently mutate/restore
document.activeElement. [code duplication]
**Severity Level:** Minor ⚠️
```suggestion
setActiveElement(mockInput);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Wrapping the repeated Object.defineProperty logic in a small helper (e.g.,
setActiveElement)
reduces duplication and centralizes configurable/writable descriptor usage,
making maintenance easier.
This suggestion assumes adding that helper (and probably the
beforeEach/afterEach to restore), which is appropriate here.
</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/test/utils/filterStateManager.test.ts
**Line:** 145:148
**Comment:**
*Code Duplication: Repeated direct Object.defineProperty calls are
duplicated and error-prone; use a helper function (added in the top-level
describe) to set the active element so all tests consistently mutate/restore
document.activeElement.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts:
##########
@@ -0,0 +1,734 @@
+/**
+ * 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}`;
Review Comment:
**Suggestion:** Incorrect formatting for IN_RANGE values: the IN_RANGE
branch in `simpleFilterToWhereClause` inserts raw `value` and `filterTo`
without applying `formatValueForOperator` or escaping/quoting string values,
which can produce invalid SQL or allow injection-like issues; format both
endpoints, escape strings, and quote them when needed. [security]
**Severity Level:** Critical 🚨
```suggestion
const fromFormatted = formatValueForOperator(type, value!);
const toFormatted = formatValueForOperator(type, filterTo as
FilterValue);
const render = (v: FilterValue) =>
typeof v === 'string' ? `'${escapeSQLString(String(v))}'` : v;
return `${columnName} ${SQL_OPERATORS.BETWEEN} ${render(fromFormatted)}
AND ${render(toFormatted)}`;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code emits the raw value/filterTo into a BETWEEN clause without
applying formatValueForOperator, escaping single quotes, or quoting string
endpoints. That can produce invalid SQL (unquoted strings) and potential
injection-like issues. The suggested change to format, escape and quote string
endpoints addresses a real correctness/security concern.
</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/utils/agGridFilterConverter.ts
**Line:** 335:335
**Comment:**
*Security: Incorrect formatting for IN_RANGE values: the IN_RANGE
branch in `simpleFilterToWhereClause` inserts raw `value` and `filterTo`
without applying `formatValueForOperator` or escaping/quoting string values,
which can produce invalid SQL or allow injection-like issues; format both
endpoints, escape strings, and quote them when needed.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/filterStateManager.ts:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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 type { RefObject } from 'react';
+import { GridApi } from 'ag-grid-community';
+import { convertAgGridFiltersToSQL } from './agGridFilterConverter';
+import type {
+ AgGridFilterModel,
+ SQLAlchemyFilter,
+} from './agGridFilterConverter';
+import type { AgGridReact } from
'@superset-ui/core/components/ThemedAgGridReact';
+import type { FilterInputPosition, AGGridFilterInstance } from '../types';
+import { FILTER_INPUT_POSITIONS, FILTER_CONDITION_BODY_INDEX } from
'../consts';
+
+export interface FilterState {
+ originalFilterModel: AgGridFilterModel;
+ simpleFilters: SQLAlchemyFilter[];
+ complexWhere?: string;
+ havingClause?: string;
+ lastFilteredColumn?: string;
+ inputPosition?: FilterInputPosition;
+}
+
+/**
+ * Detects which input position (first or second) was last modified in a filter
+ * @param gridApi - AG Grid API reference
+ * @param filterModel - Current filter model
+ * @param activeElement - The currently focused DOM element
+ * @returns Object containing lastFilteredColumn and inputPosition
+ */
+async function detectLastFilteredInput(
+ gridApi: GridApi,
+ filterModel: AgGridFilterModel,
+ activeElement: HTMLElement,
+): Promise<{
+ lastFilteredColumn?: string;
+ inputPosition: FilterInputPosition;
+}> {
+ let inputPosition: FilterInputPosition = FILTER_INPUT_POSITIONS.UNKNOWN;
+ let lastFilteredColumn: string | undefined;
+
+ // Loop through filtered columns to find which one contains the active
element
+ for (const [colId] of Object.entries(filterModel)) {
+ const filterInstance = (await gridApi.getColumnFilterInstance(
+ colId,
+ )) as AGGridFilterInstance | null;
+
+ if (!filterInstance) {
+ continue;
+ }
+
+ if (filterInstance.eConditionBodies) {
+ const conditionBodies = filterInstance.eConditionBodies;
+
+ // Check first condition body
+ if (
+ conditionBodies[FILTER_CONDITION_BODY_INDEX.FIRST]?.contains(
+ activeElement,
+ )
+ ) {
+ inputPosition = FILTER_INPUT_POSITIONS.FIRST;
+ lastFilteredColumn = colId;
+ break;
+ }
+
+ // Check second condition body
+ if (
+ conditionBodies[FILTER_CONDITION_BODY_INDEX.SECOND]?.contains(
+ activeElement,
+ )
+ ) {
+ inputPosition = FILTER_INPUT_POSITIONS.SECOND;
+ lastFilteredColumn = colId;
+ break;
+ }
+ }
+
+ if (filterInstance.eJoinAnds) {
+ for (const joinAnd of filterInstance.eJoinAnds) {
+ if (joinAnd.eGui?.contains(activeElement)) {
+ inputPosition = FILTER_INPUT_POSITIONS.FIRST;
+ lastFilteredColumn = colId;
+ break;
+ }
+ }
+ if (lastFilteredColumn) break;
+ }
+
+ if (filterInstance.eJoinOrs) {
+ for (const joinOr of filterInstance.eJoinOrs) {
+ if (joinOr.eGui?.contains(activeElement)) {
+ inputPosition = FILTER_INPUT_POSITIONS.FIRST;
+ lastFilteredColumn = colId;
+ break;
+ }
+ }
+ if (lastFilteredColumn) break;
+ }
+ }
+
+ return { lastFilteredColumn, inputPosition };
+}
+
+/**
+ * Gets complete filter state including SQL conversion and input position
detection
+ * @param gridRef - React ref to AG Grid component
+ * @param metricColumns - Array of metric column names for SQL conversion
+ * @returns Complete filter state object
+ */
+export async function getCompleteFilterState(
+ gridRef: RefObject<AgGridReact>,
+ metricColumns: string[],
+): Promise<FilterState> {
+ const activeElement = document.activeElement as HTMLElement;
+
+ if (!gridRef.current?.api) {
+ return {
+ originalFilterModel: {},
+ simpleFilters: [],
+ complexWhere: undefined,
+ havingClause: undefined,
+ lastFilteredColumn: undefined,
+ inputPosition: FILTER_INPUT_POSITIONS.UNKNOWN,
+ };
+ }
+
+ const filterModel = gridRef.current.api.getFilterModel();
+
+ // Convert filters to SQL
+ const convertedFilters = convertAgGridFiltersToSQL(
+ filterModel,
+ metricColumns,
+ );
+
+ // Detect which input was last modified
+ const { lastFilteredColumn, inputPosition } = await detectLastFilteredInput(
+ gridRef.current.api,
+ filterModel,
+ activeElement,
+ );
Review Comment:
**Suggestion:** Runtime error when document.activeElement is null: the code
casts document.activeElement to HTMLElement and always passes it into
`detectLastFilteredInput`; if there is no active element this will be null and
cause `.contains` calls to throw. Fix by treating `document.activeElement` as
nullable, only calling `detectLastFilteredInput` when it is an HTMLElement, and
defaulting `lastFilteredColumn`/`inputPosition` otherwise. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
const activeElement = document.activeElement as Element | null;
if (!gridRef.current?.api) {
return {
originalFilterModel: {},
simpleFilters: [],
complexWhere: undefined,
havingClause: undefined,
lastFilteredColumn: undefined,
inputPosition: FILTER_INPUT_POSITIONS.UNKNOWN,
};
}
const filterModel = gridRef.current.api.getFilterModel();
// Convert filters to SQL
const convertedFilters = convertAgGridFiltersToSQL(
filterModel,
metricColumns,
);
// Detect which input was last modified (only if the active element is an
HTMLElement)
let lastFilteredColumn: string | undefined;
let inputPosition: FilterInputPosition = FILTER_INPUT_POSITIONS.UNKNOWN;
if (activeElement instanceof HTMLElement) {
const result = await detectLastFilteredInput(
gridRef.current.api,
filterModel,
activeElement,
);
lastFilteredColumn = result.lastFilteredColumn;
inputPosition = result.inputPosition;
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Defensive change — treating document.activeElement as nullable and only
calling the detector when it's an HTMLElement is safe and prevents a possible
runtime mismatch. In practice element.contains(null) usually returns false and
wouldn't throw, but the current code lies to TypeScript by force-casting to
HTMLElement and could hide real edge-cases (SVGElement vs HTMLElement, null).
The proposed change is sensible and low-risk.
</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/utils/filterStateManager.ts
**Line:** 130:156
**Comment:**
*Null Pointer: Runtime error when document.activeElement is null: the
code casts document.activeElement to HTMLElement and always passes it into
`detectLastFilteredInput`; if there is no active element this will be null and
cause `.contains` calls to throw. Fix by treating `document.activeElement` as
nullable, only calling `detectLastFilteredInput` when it is an HTMLElement, and
defaulting `lastFilteredColumn`/`inputPosition` otherwise.
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>
##########
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>;
+
+ const result = await getCompleteFilterState(gridRef, []);
+
+ expect(result.complexWhere).toBe('(age >= 18 AND age < 65)');
+ });
+
+ it('should handle set filters correctly', async () => {
+ const filterModel = {
+ status: {
+ filterType: 'set',
+ values: ['active', 'pending', 'approved'],
+ },
+ };
+
+ 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.simpleFilters).toHaveLength(1);
+ expect(result.simpleFilters[0]).toEqual({
+ col: 'status',
+ op: 'IN',
+ val: ['active', 'pending', 'approved'],
+ });
+ });
+
+ it('should break detection loop after finding active element', async () =>
{
+ const filterModel = {
+ col1: { filterType: 'text', type: 'equals', filter: 'a' },
+ col2: { filterType: 'text', type: 'equals', filter: 'b' },
+ col3: { filterType: 'text', type: 'equals', filter: 'c' },
+ };
+
+ const mockInput = document.createElement('input');
+ const mockConditionBody = document.createElement('div');
+ mockConditionBody.appendChild(mockInput);
+
+ let callCount = 0;
+ const mockApi = {
+ getFilterModel: jest.fn(() => filterModel),
+ getColumnFilterInstance: jest.fn((colId: string) => {
+ callCount++;
+ // Return match on col2
+ if (colId === 'col2') {
+ return Promise.resolve({
+ eGui: document.createElement('div'),
+ eConditionBodies: [mockConditionBody],
+ });
+ }
+ return Promise.resolve({
+ eGui: document.createElement('div'),
+ eConditionBodies: [document.createElement('div')],
+ });
+ }),
+ };
+
+ const gridRef = {
+ current: { api: mockApi } as any,
+ } as RefObject<AgGridReact>;
+
+ Object.defineProperty(document, 'activeElement', {
+ writable: true,
+ configurable: true,
+ value: mockInput,
+ });
+
+ const result = await getCompleteFilterState(gridRef, []);
+
+ expect(result.lastFilteredColumn).toBe('col2');
+ // Should not call getColumnFilterInstance for col3 after finding match
+ expect(callCount).toBeLessThanOrEqual(2);
Review Comment:
**Suggestion:** The test expects `getColumnFilterInstance` to be called at
most twice, but depending on iteration order the function might legitimately be
called fewer than three times but not exactly constrained to <=2; assert that
fewer than the total number of columns were called (i.e., strictly less than 3)
to express the intent more robustly. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
// Should stop early after finding the active element (fewer than
total columns)
expect(callCount).toBeLessThan(3);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The intent is to assert the search stops early; using a bound expressed
relative to the total columns
(expect(callCount).toBeLessThan(3)) is semantically equivalent to <=2 but
reads clearer about intent.
It's a minor readability improvement and is safe to apply.
</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/test/utils/filterStateManager.test.ts
**Line:** 436:437
**Comment:**
*Logic Error: The test expects `getColumnFilterInstance` to be called
at most twice, but depending on iteration order the function might legitimately
be called fewer than three times but not exactly constrained to <=2; assert
that fewer than the total number of columns were called (i.e., strictly less
than 3) to express the intent more robustly.
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>
--
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]
