codeant-ai-for-open-source[bot] commented on code in PR #38035: URL: https://github.com/apache/superset/pull/38035#discussion_r2955419486
########## superset-frontend/plugins/plugin-chart-point-cluster-map/test/transformProps.test.ts: ########## @@ -0,0 +1,236 @@ +/* + * 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 { ChartProps } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/theme'; + +jest.mock('supercluster', () => { + const MockSupercluster = jest.fn().mockImplementation(() => ({ + load: jest.fn(), + getClusters: jest.fn().mockReturnValue([]), + })); + return { __esModule: true, default: MockSupercluster }; +}); + +// Import after mocking supercluster to avoid ESM parse error +// eslint-disable-next-line import/first +import transformProps from '../src/transformProps'; + +type TransformPropsResult = { + globalOpacity?: number; + onViewportChange?: (viewport: { + latitude: number; + longitude: number; + zoom: number; + }) => void; + viewportLongitude?: number; + viewportLatitude?: number; + viewportZoom?: number; + rgb?: string[] | null; +}; + +const baseFormData = { + all_columns_x: 'lon', + all_columns_y: 'lat', + clustering_radius: 60, + global_opacity: 0.8, + map_color: 'rgb(0, 139, 139)', + map_renderer: 'maplibre', + maplibre_style: 'https://tiles.openfreemap.org/styles/liberty', + pandas_aggfunc: 'sum', + point_radius_unit: 'Pixels', + render_while_dragging: true, + viewport_longitude: -73.935242, + viewport_latitude: 40.73061, + viewport_zoom: 9, +}; + +const baseQueriesData = [ + { + data: { + bounds: [ + [-74.0, 40.7], + [-73.9, 40.8], + ] as [[number, number], [number, number]], + geoJSON: { features: [] }, + hasCustomMetric: false, + }, + }, +]; + +function createChartProps(overrides: Record<string, unknown> = {}) { + return new ChartProps({ + formData: { ...baseFormData, ...overrides }, + width: 800, + height: 600, + queriesData: baseQueriesData, + theme: supersetTheme, + }); +} + +function getTransformPropsResult( + overrides: Record<string, unknown> = {}, +): TransformPropsResult { + return transformProps(createChartProps(overrides)) as TransformPropsResult; +} + +test('extracts globalOpacity from formData', () => { + const result = getTransformPropsResult({ global_opacity: 0.5 }); + expect(result.globalOpacity).toBe(0.5); +}); + +test('extracts viewport values from formData', () => { + const result = getTransformPropsResult({ + viewport_longitude: -122.4, + viewport_latitude: 37.8, + viewport_zoom: 12, + }); + expect(result).toEqual( + expect.objectContaining({ + viewportLongitude: -122.4, + viewportLatitude: 37.8, + viewportZoom: 12, + }), + ); +}); + +test('clamps viewport values to safe map ranges', () => { + const result = getTransformPropsResult({ + viewport_longitude: 190, + viewport_latitude: -100, + viewport_zoom: 99, + }); + expect(result).toEqual( + expect.objectContaining({ + viewportLongitude: 180, + viewportLatitude: -90, + viewportZoom: 16, + }), + ); Review Comment: **Suggestion:** These assertions expect `transformProps` to return `viewportLongitude`, `viewportLatitude`, and `viewportZoom`, but that function does not expose viewport fields at all. Keeping these expectations will make the test fail even when the transform output is correct. Update the test to validate the actual contract. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Jest suite fails on point-cluster transformProps tests. - ⚠️ CI validation blocked for plugin frontend changes. - ⚠️ False negatives hide unrelated regressions in same run. ``` </details> ```suggestion test('does not expose viewport values directly in transformed props', () => { const result = getTransformPropsResult({ viewport_longitude: -122.4, viewport_latitude: 37.8, viewport_zoom: 12, }); expect(result.viewportLongitude).toBeUndefined(); expect(result.viewportLatitude).toBeUndefined(); expect(result.viewportZoom).toBeUndefined(); }); test('does not clamp viewport form values inside transformProps', () => { const result = getTransformPropsResult({ viewport_longitude: 190, viewport_latitude: -100, viewport_zoom: 99, }); expect(result.viewportLongitude).toBeUndefined(); expect(result.viewportLatitude).toBeUndefined(); expect(result.viewportZoom).toBeUndefined(); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run Jest for `superset-frontend/plugins/plugin-chart-point-cluster-map/test/transformProps.test.ts`; this test file is part of frontend test execution. 2. In `transformProps.test.ts:87-91`, `getTransformPropsResult()` calls `transformProps()` from `../src/transformProps`. 3. `transformProps` return object at `src/transformProps.ts:213-243` includes `globalOpacity`, `onViewportChange`, etc., but no `viewportLongitude`, `viewportLatitude`, or `viewportZoom`. 4. Assertions at `transformProps.test.ts:105-109` and `120-124` require those missing fields, so tests fail deterministically despite current implementation contract. ``` </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-point-cluster-map/test/transformProps.test.ts **Line:** 98:125 **Comment:** *Logic Error: These assertions expect `transformProps` to return `viewportLongitude`, `viewportLatitude`, and `viewportZoom`, but that function does not expose viewport fields at all. Keeping these expectations will make the test fail even when the transform output is correct. Update the test to validate the actual contract. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=db93a3566e9cb16be8db1f98cba0e0b1947331e7043bf04fea25583593b7a3f2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=db93a3566e9cb16be8db1f98cba0e0b1947331e7043bf04fea25583593b7a3f2&reaction=dislike'>👎</a> ########## tests/unit_tests/models/slice_test.py: ########## @@ -85,3 +85,41 @@ def test_id_or_uuid_filter(self, test_name, input_value): """Test id_or_uuid_filter returns correct BinaryExpression.""" result = id_or_uuid_filter(input_value) assert result is not None + + def test_datasource_url_returns_none_when_datasource_lacks_explore_url(self): + """datasource_url() must not raise when the datasource has no explore_url. + + Charts whose datasource resolves to a Query (or any other type without + explore_url) used to raise AttributeError, which caused the entire chart + list API response to fail instead of just skipping that one chart. + """ + slc = Slice() + slc.id = 1 + + # Simulate a datasource object that does NOT have explore_url (e.g. Query) + mock_datasource = MagicMock(spec=[]) # spec=[] means no attributes at all + slc.table = mock_datasource + Review Comment: **Suggestion:** Assigning `MagicMock(spec=[])` directly to the SQLAlchemy relationship field `table` can raise runtime instrumentation/type errors (for example missing ORM state) before `datasource_url()` is exercised. Mock the class attribute instead so the test validates `datasource_url()` behavior without breaking on ORM relationship assignment. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Unit test may fail before assertion executes. - ⚠️ CI can fail on slice model tests. - ⚠️ datasource_url regression coverage becomes less reliable. ``` </details> ```suggestion mock_datasource = object() with patch.object(Slice, "table", new=mock_datasource): ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the unit test `tests/unit_tests/models/slice_test.py::TestSlice::test_datasource_url_returns_none_when_datasource_lacks_explore_url` (entry point is pytest collecting this test class). 2. Inside the test (`tests/unit_tests/models/slice_test.py:96-101`), a `Slice()` instance is created and `slc.table` is assigned `MagicMock(spec=[])`. 3. `Slice.table` is a SQLAlchemy relationship (`superset/models/slice.py:114-122`) targeting mapped class `"SqlaTable"`, so assigning a non-mapped mock can trigger ORM instrumentation/type-state errors before method logic. 4. The test can fail before `slc.datasource_url()` at `tests/unit_tests/models/slice_test.py:103`, meaning the test may not actually validate `datasource_url()` behavior (`superset/models/slice.py:166-173`). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/models/slice_test.py **Line:** 100:102 **Comment:** *Possible Bug: Assigning `MagicMock(spec=[])` directly to the SQLAlchemy relationship field `table` can raise runtime instrumentation/type errors (for example missing ORM state) before `datasource_url()` is exercised. Mock the class attribute instead so the test validates `datasource_url()` behavior without breaking on ORM relationship assignment. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=e529776d7971186b0af55671013a4c080e4ceeb287d9cf0624a5dfebc4815ba5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=e529776d7971186b0af55671013a4c080e4ceeb287d9cf0624a5dfebc4815ba5&reaction=dislike'>👎</a> ########## superset/mcp_service/sql_lab/tool/save_sql_query.py: ########## @@ -43,7 +43,16 @@ logger = logging.getLogger(__name__) -@tool(tags=["mutate"]) +@tool( + tags=["mutate"], + class_permission_name="SQLLab", + method_permission_name="write", Review Comment: **Suggestion:** The RBAC action is set to `write`, but SQL Lab permissions in Superset define `can_read` and `can_execute_sql_query` (not `can_write`), so this tool will be denied for users who otherwise can use SQL Lab. Use an existing SQL Lab action permission to avoid a permanent authorization failure. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ save_sql_query MCP tool always permission-denied. - ⚠️ SQL Lab query-saving automation unavailable to MCP clients. ``` </details> ```suggestion method_permission_name="execute_sql_query", ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start Superset with MCP enabled; startup path `superset/core/mcp/core_mcp_injection.py:36-67` replaces decorators and imports MCP app so tools register. 2. During registration, `save_sql_query` is decorated with `class_permission_name="SQLLab"` and `method_permission_name="write"` at `superset/mcp_service/sql_lab/tool/save_sql_query.py:46-50`. 3. Invoke MCP tool `save_sql_query` (listed in MCP instructions at `superset/mcp_service/app.py:71-74`), which is wrapped by auth hook in `core_mcp_injection.py:126-129`. 4. RBAC check in `superset/mcp_service/auth.py:92-113` (default `MCP_RBAC_ENABLED=True`) builds permission `can_write` and calls `security_manager.can_access("can_write", "SQLLab")`. 5. SQL Lab permission set in `superset/security/manager.py:50-61` includes `can_read`, `can_execute_sql_query`, etc., but not `can_write` for `SQLLab`; request is denied and tool returns permission failure. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/sql_lab/tool/save_sql_query.py **Line:** 49:49 **Comment:** *Logic Error: The RBAC action is set to `write`, but SQL Lab permissions in Superset define `can_read` and `can_execute_sql_query` (not `can_write`), so this tool will be denied for users who otherwise can use SQL Lab. Use an existing SQL Lab action permission to avoid a permanent authorization failure. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=0497e7935a5e45295c3d0351f0210dbe391775d32d4c6ddb2aa919dc0e62230c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=0497e7935a5e45295c3d0351f0210dbe391775d32d4c6ddb2aa919dc0e62230c&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/components/Select/controlPanel.ts: ########## @@ -32,8 +33,40 @@ const { defaultToFirstItem, searchAllOptions, sortAscending, + operatorType, } = DEFAULT_FORM_DATA; +type FilterSelectColumn = { + column_name: string; + type_generic?: GenericDataType | null; +}; + +export const getOperatorTypeChoices = (isStringColumn: boolean) => [ + [SelectFilterOperatorType.Exact, t('Exact match (IN)')], + ...(isStringColumn + ? [ + [SelectFilterOperatorType.Contains, t('Contains text (ILIKE %x%)')], + [SelectFilterOperatorType.StartsWith, t('Starts with (ILIKE x%)')], + [SelectFilterOperatorType.EndsWith, t('Ends with (ILIKE %x)')], + ] + : []), +]; + +export const isStringOperatorColumn = ( + selectedColumn: unknown, + columns?: FilterSelectColumn[], +) => { + const columnName = ensureIsArray(selectedColumn)[0]; + if (!columnName || !columns) { + return true; Review Comment: **Suggestion:** The column-type helper treats an unselected column as string, which exposes ILIKE operators before a valid column is chosen. If users pick a LIKE operator first and then switch to a non-string column, an incompatible operator can remain in form state and produce incorrect filter behavior. Default to non-string until a real column is resolved. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Select filter config may keep stale operatorType. - ⚠️ Non-string columns can receive unintended ILIKE predicates. - ⚠️ Filter behavior differs from visible operator choices. ``` </details> ```suggestion return false; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open Select filter plugin configuration path using chart plugin control panel (`superset-frontend/src/filters/components/Select/index.ts:37-40` wires `controlPanel.ts`), where `operatorType` choices are computed by `isStringOperatorColumn()` (`controlPanel.ts:108-115`). 2. With no column resolved yet, `isStringOperatorColumn()` returns `true` at `controlPanel.ts:60-62`, so ILIKE options are shown and can be chosen. 3. Change to a non-string column; choices are recomputed, but control value is not auto-sanitized by `SelectControl` (`explore/components/controls/SelectControl.tsx:183-191` updates options only; `279-292` keeps current value). 4. Save with stale non-Exact operator; runtime filter generation still receives `operatorType` and can emit ILIKE when string input exists (`filters/utils.ts:67-75`), causing mismatched semantics on non-string columns. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/Select/controlPanel.ts **Line:** 61:61 **Comment:** *Logic Error: The column-type helper treats an unselected column as string, which exposes ILIKE operators before a valid column is chosen. If users pick a LIKE operator first and then switch to a non-string column, an incompatible operator can remain in form state and produce incorrect filter behavior. Default to non-string until a real column is resolved. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=c31bb308bcacc03598bdc946db547df2686740e67992d9d80105db2b6941d094&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=c31bb308bcacc03598bdc946db547df2686740e67992d9d80105db2b6941d094&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx: ########## @@ -483,6 +506,52 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { setExcludeFilterValues(value === 'true'); }; + const updateDataMaskRef = useRef(updateDataMask); + useEffect(() => { + updateDataMaskRef.current = updateDataMask; + }, [updateDataMask]); + + const debouncedLikeChange = useMemo( + () => + debounce((text: string) => { + if (text) { + updateDataMaskRef.current([text]); + } else { + updateDataMaskRef.current(null); + } + }, Constants.SLOW_DEBOUNCE), + [], + ); + + useEffect(() => { + if (!isLikeOperator || clearAllTrigger) { + debouncedLikeChange.cancel(); + } + }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]); Review Comment: **Suggestion:** When the filter switches to a LIKE operator with an existing selected value, the component updates the text input but does not recompute `extraFormData`, so queries can keep using stale `IN`/old operator semantics until the user types again. Recompute the data mask when LIKE mode is active and the external filter value changes. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ LIKE mode may still apply old IN semantics. - ⚠️ Native filter results mismatch displayed operator selection. - ⚠️ Users must type again to refresh query predicate. ``` </details> ```suggestion return; } if (filterState.value !== undefined) { const text = filterState.value?.[0] != null ? String(filterState.value[0]) : ''; updateDataMaskRef.current(text ? [text] : null); } }, [ clearAllTrigger, debouncedLikeChange, filterState.value, isLikeOperator, ]); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a dashboard native Select filter (render path: `FilterBar/FilterControls/FilterValue.tsx:153-170` renders `SuperChart`, which renders `SelectFilterPlugin` via `filters/components/Select/transformProps.ts:64-85`). 2. Select a value while operator is Exact; `updateDataMask()` in `SelectFilterPlugin.tsx:216-245` writes `extraFormData` using `getSelectExtraFormData(..., operatorType)` and then publishes via `setDataMask` (`SelectFilterPlugin.tsx:460-461`), which flows to dashboard state (`FilterBar/index.tsx:225-256`, dispatch at `:268`). 3. Switch operator to a LIKE mode (Contains/StartsWith/EndsWith) while a value already exists; component flips to input UI (`isLikeOperator` at `SelectFilterPlugin.tsx:166-168`, conditional render at `:576-623`), and only syncs input text (`:186-190`) plus debounce cancellation (`:526-530`), but does not recompute `extraFormData`. 4. Run/Apply filters; downstream chart filtering still uses previously stored operator from `dataMask.extraFormData` merged in `dashboard/components/nativeFilters/utils.ts:16-27` and `:135-143`, so SQL clause semantics remain stale (e.g., old `IN` instead of expected `ILIKE`) until user types and triggers `handleLikeInputChange` (`SelectFilterPlugin.tsx:534-540`). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx **Line:** 529:530 **Comment:** *Logic Error: When the filter switches to a LIKE operator with an existing selected value, the component updates the text input but does not recompute `extraFormData`, so queries can keep using stale `IN`/old operator semantics until the user types again. Recompute the data mask when LIKE mode is active and the external filter value changes. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=052b08b7c87e5263913e7d4efa2ba6fbadf7178116ee6b0de924eefc8b7b07bf&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=052b08b7c87e5263913e7d4efa2ba6fbadf7178116ee6b0de924eefc8b7b07bf&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/tool/update_chart_preview.py: ########## @@ -44,7 +44,16 @@ logger = logging.getLogger(__name__) -@tool(tags=["mutate"], class_permission_name="Chart", method_permission_name="write") +@tool( + tags=["mutate"], + class_permission_name="Chart", + method_permission_name="write", + annotations=ToolAnnotations( + title="Update chart preview", + readOnlyHint=False, + destructiveHint=True, Review Comment: **Suggestion:** The tool is marked as destructive even though it only updates transient preview cache state and does not persist or delete user data. MCP clients can use `destructiveHint` to require extra confirmation or block autonomous execution, so this incorrect flag can cause valid preview-update calls to be unnecessarily gated or skipped. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ MCP automation may gate `update_chart_preview` unnecessarily. - ⚠️ Iterative chart preview editing becomes slower for users. ``` </details> ```suggestion destructiveHint=False, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the MCP service so tools are registered via imports in `superset/mcp_service/app.py:355-403`, which includes `update_chart_preview`. 2. Registration path passes tool annotations unchanged through `Tool.from_function(..., annotations=annotations)` in `superset/core/mcp/core_mcp_injection.py:135-141`. 3. `update_chart_preview` declares `destructiveHint=True` at `superset/mcp_service/chart/tool/update_chart_preview.py:47-55`, while its own docstring says it only updates cached preview state and does not save charts (`:61-66`). 4. MCP clients that read tool metadata (same API surface exercised by `mcp.list_tools()` in `tests/unit_tests/mcp_service/test_mcp_tool_registration.py:34-39`) will classify this tool as destructive and may require confirmation or skip autonomous execution, causing avoidable friction in preview-update flows. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/tool/update_chart_preview.py **Line:** 54:54 **Comment:** *Logic Error: The tool is marked as destructive even though it only updates transient preview cache state and does not persist or delete user data. MCP clients can use `destructiveHint` to require extra confirmation or block autonomous execution, so this incorrect flag can cause valid preview-update calls to be unnecessarily gated or skipped. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=d16da3e7cf55e42d058de2dac8227c8b52a9bc5c0e58ac1c63c891c526e51007&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=d16da3e7cf55e42d058de2dac8227c8b52a9bc5c0e58ac1c63c891c526e51007&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/test/transformProps.test.ts: ########## @@ -0,0 +1,236 @@ +/* + * 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 { ChartProps } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/theme'; + +jest.mock('supercluster', () => { + const MockSupercluster = jest.fn().mockImplementation(() => ({ + load: jest.fn(), + getClusters: jest.fn().mockReturnValue([]), + })); + return { __esModule: true, default: MockSupercluster }; +}); + +// Import after mocking supercluster to avoid ESM parse error +// eslint-disable-next-line import/first +import transformProps from '../src/transformProps'; + +type TransformPropsResult = { + globalOpacity?: number; + onViewportChange?: (viewport: { + latitude: number; + longitude: number; + zoom: number; + }) => void; + viewportLongitude?: number; + viewportLatitude?: number; + viewportZoom?: number; + rgb?: string[] | null; +}; + +const baseFormData = { + all_columns_x: 'lon', + all_columns_y: 'lat', + clustering_radius: 60, + global_opacity: 0.8, + map_color: 'rgb(0, 139, 139)', + map_renderer: 'maplibre', + maplibre_style: 'https://tiles.openfreemap.org/styles/liberty', + pandas_aggfunc: 'sum', + point_radius_unit: 'Pixels', + render_while_dragging: true, + viewport_longitude: -73.935242, + viewport_latitude: 40.73061, + viewport_zoom: 9, +}; + +const baseQueriesData = [ + { + data: { + bounds: [ + [-74.0, 40.7], + [-73.9, 40.8], + ] as [[number, number], [number, number]], + geoJSON: { features: [] }, + hasCustomMetric: false, + }, + }, +]; + +function createChartProps(overrides: Record<string, unknown> = {}) { + return new ChartProps({ + formData: { ...baseFormData, ...overrides }, + width: 800, + height: 600, + queriesData: baseQueriesData, + theme: supersetTheme, + }); +} + +function getTransformPropsResult( + overrides: Record<string, unknown> = {}, +): TransformPropsResult { + return transformProps(createChartProps(overrides)) as TransformPropsResult; +} + +test('extracts globalOpacity from formData', () => { + const result = getTransformPropsResult({ global_opacity: 0.5 }); + expect(result.globalOpacity).toBe(0.5); +}); + +test('extracts viewport values from formData', () => { + const result = getTransformPropsResult({ + viewport_longitude: -122.4, + viewport_latitude: 37.8, + viewport_zoom: 12, + }); + expect(result).toEqual( + expect.objectContaining({ + viewportLongitude: -122.4, + viewportLatitude: 37.8, + viewportZoom: 12, + }), + ); +}); + +test('clamps viewport values to safe map ranges', () => { + const result = getTransformPropsResult({ + viewport_longitude: 190, + viewport_latitude: -100, + viewport_zoom: 99, + }); + expect(result).toEqual( + expect.objectContaining({ + viewportLongitude: 180, + viewportLatitude: -90, + viewportZoom: 16, + }), + ); +}); + +test('provides onViewportChange callback that updates control values', () => { + const setControlValue = jest.fn(); + const chartProps = new ChartProps({ + formData: baseFormData, + width: 800, + height: 600, + queriesData: baseQueriesData, + hooks: { setControlValue }, + theme: supersetTheme, + }); + const result = transformProps(chartProps) as TransformPropsResult; + expect(result.onViewportChange).toBeDefined(); + + result.onViewportChange!({ + latitude: 51.5, + longitude: -0.12, + zoom: 10, + }); + + expect(setControlValue).toHaveBeenCalledWith('viewport_longitude', -0.12); + expect(setControlValue).toHaveBeenCalledWith('viewport_latitude', 51.5); + expect(setControlValue).toHaveBeenCalledWith('viewport_zoom', 10); +}); + +test('normalizes string viewport values to numbers', () => { + const result = getTransformPropsResult({ + viewport_longitude: '-122.4', + viewport_latitude: '37.8', + viewport_zoom: '12', + }); + expect(result.viewportLongitude).toBe(-122.4); + expect(result.viewportLatitude).toBe(37.8); + expect(result.viewportZoom).toBe(12); +}); + +test('normalizes empty viewport values to undefined', () => { + const result = getTransformPropsResult({ + viewport_longitude: '', + viewport_latitude: '', + viewport_zoom: '', + }); + expect(result.viewportLongitude).toBeUndefined(); + expect(result.viewportLatitude).toBeUndefined(); + expect(result.viewportZoom).toBeUndefined(); +}); + +test('normalizes whitespace-only viewport values to undefined', () => { + const result = getTransformPropsResult({ + viewport_longitude: ' ', + viewport_latitude: '\t', + viewport_zoom: ' \n ', + }); + expect(result.viewportLongitude).toBeUndefined(); + expect(result.viewportLatitude).toBeUndefined(); + expect(result.viewportZoom).toBeUndefined(); Review Comment: **Suggestion:** These tests assume viewport values are normalized by `transformProps`, but viewport values are not returned from this transformer, so the assertions are checking behavior that never exists in this layer. Replace them with assertions that match the current return contract. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Deterministic failure in transformProps viewport normalization tests. - ⚠️ Frontend CI cannot trust point-cluster test signal. - ⚠️ Developer time wasted investigating non-product failures. ``` </details> ```suggestion test('does not normalize viewport form values in transformProps', () => { const numericStringResult = getTransformPropsResult({ viewport_longitude: '-122.4', viewport_latitude: '37.8', viewport_zoom: '12', }); expect(numericStringResult.viewportLongitude).toBeUndefined(); expect(numericStringResult.viewportLatitude).toBeUndefined(); expect(numericStringResult.viewportZoom).toBeUndefined(); const emptyResult = getTransformPropsResult({ viewport_longitude: '', viewport_latitude: '', viewport_zoom: '', }); expect(emptyResult.viewportLongitude).toBeUndefined(); expect(emptyResult.viewportLatitude).toBeUndefined(); expect(emptyResult.viewportZoom).toBeUndefined(); const whitespaceResult = getTransformPropsResult({ viewport_longitude: ' ', viewport_latitude: '\t', viewport_zoom: ' \n ', }); expect(whitespaceResult.viewportLongitude).toBeUndefined(); expect(whitespaceResult.viewportLatitude).toBeUndefined(); expect(whitespaceResult.viewportZoom).toBeUndefined(); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Execute `transformProps.test.ts` in Jest; these tests invoke `getTransformPropsResult()` (`transformProps.test.ts:87-91`). 2. Pass string/empty/whitespace viewport values via formData in tests at `transformProps.test.ts:153-178`. 3. `src/transformProps.ts:115-129` does not even destructure `viewport_longitude`, `viewport_latitude`, or `viewport_zoom`; returned props (`213-243`) still omit viewport fields. 4. Assertions at `transformProps.test.ts:158-160`, `169-171`, `180-182` therefore check nonexistent outputs and fail regardless of runtime chart 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-point-cluster-map/test/transformProps.test.ts **Line:** 152:182 **Comment:** *Logic Error: These tests assume viewport values are normalized by `transformProps`, but viewport values are not returned from this transformer, so the assertions are checking behavior that never exists in this layer. Replace them with assertions that match the current return contract. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=ed5a2fe9769f7a0e41bd588ac0962c8722fae3571814484a14bfe0d3f8128c5c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=ed5a2fe9769f7a0e41bd588ac0962c8722fae3571814484a14bfe0d3f8128c5c&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/utils.ts: ########## @@ -46,13 +64,26 @@ export const getSelectExtraFormData = ( }, ]; } else if (value !== undefined && value !== null && value.length !== 0) { - extra.filters = [ - { - col, - op: shouldExcludeFilter ? ('NOT IN' as const) : ('IN' as const), - val: value, - }, - ]; + const isLikeOperator = operatorType !== SelectFilterOperatorType.Exact; + + if (isLikeOperator && typeof value[0] === 'string') { + const wildcardVal = applyWildcard(value[0] as string, operatorType); Review Comment: **Suggestion:** The LIKE branch only uses the first element (`value[0]`) and silently discards the rest of the selected values. Since this function accepts an array and can receive multi-value state (for example from previously saved exact-match selections), this causes incorrect filtering by applying only one value. Restrict LIKE handling to a single string value and fall back to the normal `IN` path otherwise so all selected values are preserved. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Native Select filter ignores values beyond first. - ⚠️ Dashboard filtering semantics become incorrect for saved defaults. ``` </details> ```suggestion if ( isLikeOperator && value.length === 1 && typeof value[0] === 'string' ) { const wildcardVal = applyWildcard(value[0], operatorType); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a Native Select filter with multi-select enabled (default `multiSelect: true` in `superset-frontend/src/filters/components/Select/types.ts:80-89`) and set multiple default values (type allows arrays via `defaultValue?: SelectValue` at `types.ts:42-53`). 2. Change that filter's match type to a LIKE operator in the config modal (`Contains/StartsWith/EndsWith` options at `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:1578-1607`); note `onOperatorTypeChanged` only updates `controlValues.operatorType` and `defaultDataMask` and does not clear multi-value defaults (`FiltersConfigForm.tsx:646-655`). 3. Load/render the dashboard: `SelectFilterPlugin` initialization calls `updateDataMask(formData.defaultValue)` when `filterState.value` is undefined (`superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:91-112`), passing the multi-value array into `getSelectExtraFormData` (`SelectFilterPlugin.tsx:95-101`). 4. In `getSelectExtraFormData`, LIKE mode uses only `value[0]` (`superset-frontend/src/filters/utils.ts:69-76`), so additional selected/default values are silently dropped, producing a single `ILIKE` predicate instead of preserving all values. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/utils.ts **Line:** 69:70 **Comment:** *Logic Error: The LIKE branch only uses the first element (`value[0]`) and silently discards the rest of the selected values. Since this function accepts an array and can receive multi-value state (for example from previously saved exact-match selections), this causes incorrect filtering by applying only one value. Restrict LIKE handling to a single string value and fall back to the normal `IN` path otherwise so all selected values are preserved. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=bf434958d21799e0ae23a6450231c90de76ee0ebadfe18dd53acc76fd008a424&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=bf434958d21799e0ae23a6450231c90de76ee0ebadfe18dd53acc76fd008a424&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/test/transformProps.test.ts: ########## @@ -0,0 +1,236 @@ +/* + * 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 { ChartProps } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/theme'; + +jest.mock('supercluster', () => { + const MockSupercluster = jest.fn().mockImplementation(() => ({ + load: jest.fn(), + getClusters: jest.fn().mockReturnValue([]), + })); + return { __esModule: true, default: MockSupercluster }; +}); + +// Import after mocking supercluster to avoid ESM parse error +// eslint-disable-next-line import/first +import transformProps from '../src/transformProps'; + +type TransformPropsResult = { + globalOpacity?: number; + onViewportChange?: (viewport: { + latitude: number; + longitude: number; + zoom: number; + }) => void; + viewportLongitude?: number; + viewportLatitude?: number; + viewportZoom?: number; + rgb?: string[] | null; +}; + +const baseFormData = { + all_columns_x: 'lon', + all_columns_y: 'lat', + clustering_radius: 60, + global_opacity: 0.8, + map_color: 'rgb(0, 139, 139)', + map_renderer: 'maplibre', + maplibre_style: 'https://tiles.openfreemap.org/styles/liberty', + pandas_aggfunc: 'sum', + point_radius_unit: 'Pixels', + render_while_dragging: true, + viewport_longitude: -73.935242, + viewport_latitude: 40.73061, + viewport_zoom: 9, +}; + +const baseQueriesData = [ + { + data: { + bounds: [ + [-74.0, 40.7], + [-73.9, 40.8], + ] as [[number, number], [number, number]], + geoJSON: { features: [] }, + hasCustomMetric: false, + }, + }, +]; + +function createChartProps(overrides: Record<string, unknown> = {}) { + return new ChartProps({ + formData: { ...baseFormData, ...overrides }, + width: 800, + height: 600, + queriesData: baseQueriesData, + theme: supersetTheme, + }); +} + +function getTransformPropsResult( + overrides: Record<string, unknown> = {}, +): TransformPropsResult { + return transformProps(createChartProps(overrides)) as TransformPropsResult; +} + +test('extracts globalOpacity from formData', () => { + const result = getTransformPropsResult({ global_opacity: 0.5 }); + expect(result.globalOpacity).toBe(0.5); +}); + +test('extracts viewport values from formData', () => { + const result = getTransformPropsResult({ + viewport_longitude: -122.4, + viewport_latitude: 37.8, + viewport_zoom: 12, + }); + expect(result).toEqual( + expect.objectContaining({ + viewportLongitude: -122.4, + viewportLatitude: 37.8, + viewportZoom: 12, + }), + ); +}); + +test('clamps viewport values to safe map ranges', () => { + const result = getTransformPropsResult({ + viewport_longitude: 190, + viewport_latitude: -100, + viewport_zoom: 99, + }); + expect(result).toEqual( + expect.objectContaining({ + viewportLongitude: 180, + viewportLatitude: -90, + viewportZoom: 16, + }), + ); +}); + +test('provides onViewportChange callback that updates control values', () => { + const setControlValue = jest.fn(); + const chartProps = new ChartProps({ + formData: baseFormData, + width: 800, + height: 600, + queriesData: baseQueriesData, + hooks: { setControlValue }, + theme: supersetTheme, + }); + const result = transformProps(chartProps) as TransformPropsResult; + expect(result.onViewportChange).toBeDefined(); + + result.onViewportChange!({ + latitude: 51.5, + longitude: -0.12, + zoom: 10, + }); + + expect(setControlValue).toHaveBeenCalledWith('viewport_longitude', -0.12); + expect(setControlValue).toHaveBeenCalledWith('viewport_latitude', 51.5); + expect(setControlValue).toHaveBeenCalledWith('viewport_zoom', 10); +}); + +test('normalizes string viewport values to numbers', () => { + const result = getTransformPropsResult({ + viewport_longitude: '-122.4', + viewport_latitude: '37.8', + viewport_zoom: '12', + }); + expect(result.viewportLongitude).toBe(-122.4); + expect(result.viewportLatitude).toBe(37.8); + expect(result.viewportZoom).toBe(12); +}); + +test('normalizes empty viewport values to undefined', () => { + const result = getTransformPropsResult({ + viewport_longitude: '', + viewport_latitude: '', + viewport_zoom: '', + }); + expect(result.viewportLongitude).toBeUndefined(); + expect(result.viewportLatitude).toBeUndefined(); + expect(result.viewportZoom).toBeUndefined(); +}); + +test('normalizes whitespace-only viewport values to undefined', () => { + const result = getTransformPropsResult({ + viewport_longitude: ' ', + viewport_latitude: '\t', + viewport_zoom: ' \n ', + }); + expect(result.viewportLongitude).toBeUndefined(); + expect(result.viewportLatitude).toBeUndefined(); + expect(result.viewportZoom).toBeUndefined(); +}); + +test('normalizes string opacity to number', () => { + const result = getTransformPropsResult({ global_opacity: '0.5' }); + expect(result.globalOpacity).toBe(0.5); +}); + +test('defaults empty opacity to 1', () => { + const result = getTransformPropsResult({ global_opacity: '' }); + expect(result.globalOpacity).toBe(1); +}); + +test('defaults whitespace-only opacity to 1', () => { + const result = getTransformPropsResult({ global_opacity: ' ' }); + expect(result.globalOpacity).toBe(1); +}); + +test('clamps opacity to [0, 1] range', () => { + expect( + getTransformPropsResult({ global_opacity: 5 }).globalOpacity, + ).toBe(1); + expect( + getTransformPropsResult({ global_opacity: -1 }).globalOpacity, + ).toBe(0); Review Comment: **Suggestion:** `transformProps` currently passes `global_opacity` through without coercion or clamping, so these tests assert behavior implemented elsewhere (or not implemented yet). As written, they produce deterministic failures in CI. Adjust expectations to the actual transform behavior. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Opacity-related transformProps tests fail every execution. - ⚠️ CI reports regression where implementation unchanged. - ⚠️ Noise obscures real defects in plugin modifications. ``` </details> ```suggestion test('passes through string opacity values unchanged', () => { const result = getTransformPropsResult({ global_opacity: '0.5' }); expect(result.globalOpacity).toBe('0.5'); }); test('passes through empty opacity values unchanged', () => { const result = getTransformPropsResult({ global_opacity: '' }); expect(result.globalOpacity).toBe(''); }); test('passes through whitespace-only opacity values unchanged', () => { const result = getTransformPropsResult({ global_opacity: ' ' }); expect(result.globalOpacity).toBe(' '); }); test('does not clamp opacity in transformProps', () => { expect( getTransformPropsResult({ global_opacity: 5 }).globalOpacity, ).toBe(5); expect( getTransformPropsResult({ global_opacity: -1 }).globalOpacity, ).toBe(-1); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `transformProps.test.ts`; opacity tests call `getTransformPropsResult()` with string/empty/out-of-range `global_opacity` (`lines 186, 191, 196, 202, 205`). 2. In `src/transformProps.ts:119`, `global_opacity` is assigned to `globalOpacity` and returned directly at `line 219` with no parsing/clamping. 3. Current tests expect coercion/clamping (`0.5`, `1`, `0`) at `transformProps.test.ts:187, 192, 197, 203, 206`, which contradicts actual implementation. 4. Failures are deterministic in CI although chart pipeline intentionally passes raw value through this transformer layer. ``` </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-point-cluster-map/test/transformProps.test.ts **Line:** 185:206 **Comment:** *Logic Error: `transformProps` currently passes `global_opacity` through without coercion or clamping, so these tests assert behavior implemented elsewhere (or not implemented yet). As written, they produce deterministic failures in CI. Adjust expectations to the actual transform behavior. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=fe713583a368eb4fc7a24b0496aa956f05f6b4cebddda734c2249cbaa350c498&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=fe713583a368eb4fc7a24b0496aa956f05f6b4cebddda734c2249cbaa350c498&reaction=dislike'>👎</a> -- 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]
