codeant-ai-for-open-source[bot] commented on code in PR #40818: URL: https://github.com/apache/superset/pull/40818#discussion_r3375851449
########## superset-frontend/playwright/tests/dashboard/mixed-chart-dashboard-filters.spec.ts: ########## @@ -0,0 +1,200 @@ +/** + * 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. + */ + +/** + * Regression for #29519: a dashboard-level filter that is in scope for a Mixed + * (mixed_timeseries) chart should apply to BOTH of the chart's queries — Query + * A and Query B — not just Query A. + * + * A Mixed chart issues a single query context with two queries + * (queries[0] = A, queries[1] = B). This test creates a Mixed chart, puts it on + * a dashboard behind a native filter scoped to the chart, loads the dashboard, + * and inspects the outgoing POST /api/v1/chart/data payload to assert the filter + * is present in both queries. + * + * CI green => both queries inherit the dashboard filter (contract holds); + * merging closes #29519 and guards against regressions. + * CI red => Query B dropped the filter; the bug is live in the Mixed chart + * query-building path (plugin-chart-echarts/src/MixedTimeseries). + */ +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiPost, apiPut } from '../../helpers/api/requests'; +import { apiPostDashboard } from '../../helpers/api/dashboard'; +import { getDatasetByName } from '../../helpers/api/dataset'; +import { DashboardPage } from '../../pages/DashboardPage'; + +const DATASET_NAME = 'birth_names'; +const FILTER_COLUMN = 'gender'; +const FILTER_VALUE = 'boy'; + +testWithAssets( + 'Mixed chart applies dashboard filter to both queries (#29519)', + async ({ page, testAssets }) => { + const dataset = await getDatasetByName(page, DATASET_NAME); + if (!dataset) { + throw new Error( + `Dataset ${DATASET_NAME} not found — run Superset with --load-examples`, + ); + } + const datasetId = dataset.id; + + const chartParams = { + datasource: `${datasetId}__table`, + viz_type: 'mixed_timeseries', + x_axis: 'ds', + time_grain_sqla: 'P1Y', + metrics: ['count'], + groupby: [], + adhoc_filters: [], + metrics_b: ['count'], + groupby_b: [], + adhoc_filters_b: [], + row_limit: 100, + row_limit_b: 100, + truncate_metric: true, + truncate_metric_b: true, + comparison_type: 'values', + color_scheme: 'supersetColors', + }; + const chartResp = await apiPost(page, 'api/v1/chart/', { + slice_name: `mixed_filter_repro_${Date.now()}`, + viz_type: 'mixed_timeseries', + datasource_id: datasetId, + datasource_type: 'table', + params: JSON.stringify(chartParams), + }); + expect(chartResp.ok()).toBe(true); + const chartId: number = (await chartResp.json()).id; + testAssets.trackChart(chartId); + + const chartLayoutKey = `CHART-${chartId}`; + const filterId = `NATIVE_FILTER-${Math.random().toString(36).slice(2, 10)}`; + const positionJson = { + DASHBOARD_VERSION_KEY: 'v2', + ROOT_ID: { type: 'ROOT', id: 'ROOT_ID', children: ['GRID_ID'] }, + GRID_ID: { + type: 'GRID', + id: 'GRID_ID', + children: ['ROW-1'], + parents: ['ROOT_ID'], + }, + 'ROW-1': { + type: 'ROW', + id: 'ROW-1', + children: [chartLayoutKey], + parents: ['ROOT_ID', 'GRID_ID'], + meta: { background: 'BACKGROUND_TRANSPARENT' }, + }, + [chartLayoutKey]: { + type: 'CHART', + id: chartLayoutKey, + children: [], + parents: ['ROOT_ID', 'GRID_ID', 'ROW-1'], + meta: { chartId, width: 8, height: 60, sliceName: 'mixed_filter_repro' }, + }, + }; + const jsonMetadata = { + native_filter_configuration: [ + { + id: filterId, + name: 'Gender', + filterType: 'filter_select', + type: 'NATIVE_FILTER', + targets: [{ datasetId, column: { name: FILTER_COLUMN } }], + controlValues: { + multiSelect: false, + enableEmptyFilter: false, + defaultToFirstItem: false, + inverseSelection: false, + searchAllOptions: false, + }, + defaultDataMask: { + filterState: { value: [FILTER_VALUE] }, + extraFormData: { + filters: [ + { col: FILTER_COLUMN, op: 'IN', val: [FILTER_VALUE] }, + ], + }, + }, + cascadeParentIds: [], + scope: { rootPath: ['ROOT_ID'], excluded: [] }, + chartsInScope: [chartId], + }, + ], + chart_configuration: {}, + cross_filters_enabled: false, + global_chart_configuration: { + scope: { rootPath: ['ROOT_ID'], excluded: [] }, + chartsInScope: [chartId], + }, + }; + const dashResp = await apiPostDashboard(page, { + dashboard_title: `mixed_filter_repro_${Date.now()}`, + published: true, + position_json: JSON.stringify(positionJson), + json_metadata: JSON.stringify(jsonMetadata), + }); + expect(dashResp.ok()).toBe(true); + const dashBody = await dashResp.json(); + const dashboardId: number = dashBody.result?.id ?? dashBody.id; + testAssets.trackDashboard(dashboardId); + + await apiPut(page, `api/v1/chart/${chartId}`, { dashboards: [dashboardId] }); + + // Capture the Mixed chart's data request (the one with two queries). + const twoQueryPayloads: any[] = []; + page.on('request', req => { + if ( + req.url().includes('/api/v1/chart/data') && + req.method() === 'POST' + ) { + try { + const body = req.postDataJSON(); + if (body?.queries?.length === 2) { + twoQueryPayloads.push(body); + } + } catch { + // ignore non-JSON bodies + } + } + }); + + const dashboardPage = new DashboardPage(page); + await dashboardPage.gotoById(dashboardId); + await dashboardPage.waitForLoad(); + await dashboardPage.waitForChartsToLoad(); + + await expect + .poll(() => twoQueryPayloads.length, { timeout: 15_000 }) + .toBeGreaterThan(0); + + const payload = twoQueryPayloads[twoQueryPayloads.length - 1]; + const filtersA = JSON.stringify(payload.queries[0].filters || []); + const filtersB = JSON.stringify(payload.queries[1].filters || []); + + expect( + filtersA.includes(FILTER_COLUMN), + 'Query A should inherit the dashboard filter', + ).toBe(true); + expect( + filtersB.includes(FILTER_COLUMN), + 'Query B should inherit the dashboard filter (see #29519)', + ).toBe(true); Review Comment: **Suggestion:** The assertion only checks whether the filter column name string appears in serialized filters, which can pass even when the actual filter payload is wrong (wrong operator/value or unrelated structure containing the same text). Assert against structured filter objects (`col`, `op`, and `val`) in both queries so the regression test actually verifies correct filter propagation semantics. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Mixed chart filter test may miss semantic regressions. - ⚠️ Query operator/value bugs could ship undetected for Mixed charts. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. After collecting POST `/api/v1/chart/data` payloads with two queries in `superset-frontend/playwright/tests/dashboard/mixed-chart-dashboard-filters.spec.ts:160-176`, the test selects the last payload at line 187 and derives `filtersA` and `filtersB` as JSON strings from `payload.queries[0].filters` and `payload.queries[1].filters` at lines 188-189. 2. The assertions at lines 191-198 use `filtersA.includes(FILTER_COLUMN)` and `filtersB.includes(FILTER_COLUMN)` to decide whether each query "inherits the dashboard filter", so they only check that the substring `'gender'` (from `FILTER_COLUMN` at line 43) appears somewhere in the serialized filters. 3. The intended filter semantics are defined in the dashboard's native filter configuration at lines 112-135, where `defaultDataMask.extraFormData.filters` is explicitly set to `[ { col: FILTER_COLUMN, op: 'IN', val: [FILTER_VALUE] } ]`, meaning both Mixed chart queries should receive a structured filter object with the correct `col`, `op`, and `val`. 4. If the MixedTimeseries query-building path (referenced in the test comment at lines 31-34 as `plugin-chart-echarts/src/MixedTimeseries`) regresses to emitting an incorrect filter payload—for example, using a wrong operator, omitting `val`, or embedding the column name in an unrelated structure—while still including the string `"gender"` in `queries[*].filters`, these substring-based checks at lines 191-198 would still pass, so the regression test would not catch the broken filter semantics for one or both queries. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9498f66cb2ae433cb6ec28106503de35&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9498f66cb2ae433cb6ec28106503de35&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/dashboard/mixed-chart-dashboard-filters.spec.ts **Line:** 188:198 **Comment:** *Incomplete Implementation: The assertion only checks whether the filter column name string appears in serialized filters, which can pass even when the actual filter payload is wrong (wrong operator/value or unrelated structure containing the same text). Assert against structured filter objects (`col`, `op`, and `val`) in both queries so the regression test actually verifies correct filter propagation semantics. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40818&comment_hash=98858ea6ef584284f714857bb66481a4643bcb7ffe7bc8f26fb0ea9c44f8f9d0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40818&comment_hash=98858ea6ef584284f714857bb66481a4643bcb7ffe7bc8f26fb0ea9c44f8f9d0&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/dashboard/mixed-chart-dashboard-filters.spec.ts: ########## @@ -0,0 +1,200 @@ +/** + * 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. + */ + +/** + * Regression for #29519: a dashboard-level filter that is in scope for a Mixed + * (mixed_timeseries) chart should apply to BOTH of the chart's queries — Query + * A and Query B — not just Query A. + * + * A Mixed chart issues a single query context with two queries + * (queries[0] = A, queries[1] = B). This test creates a Mixed chart, puts it on + * a dashboard behind a native filter scoped to the chart, loads the dashboard, + * and inspects the outgoing POST /api/v1/chart/data payload to assert the filter + * is present in both queries. + * + * CI green => both queries inherit the dashboard filter (contract holds); + * merging closes #29519 and guards against regressions. + * CI red => Query B dropped the filter; the bug is live in the Mixed chart + * query-building path (plugin-chart-echarts/src/MixedTimeseries). + */ +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiPost, apiPut } from '../../helpers/api/requests'; +import { apiPostDashboard } from '../../helpers/api/dashboard'; +import { getDatasetByName } from '../../helpers/api/dataset'; +import { DashboardPage } from '../../pages/DashboardPage'; + +const DATASET_NAME = 'birth_names'; +const FILTER_COLUMN = 'gender'; +const FILTER_VALUE = 'boy'; + +testWithAssets( + 'Mixed chart applies dashboard filter to both queries (#29519)', + async ({ page, testAssets }) => { + const dataset = await getDatasetByName(page, DATASET_NAME); + if (!dataset) { + throw new Error( + `Dataset ${DATASET_NAME} not found — run Superset with --load-examples`, + ); + } + const datasetId = dataset.id; + + const chartParams = { + datasource: `${datasetId}__table`, + viz_type: 'mixed_timeseries', + x_axis: 'ds', + time_grain_sqla: 'P1Y', + metrics: ['count'], + groupby: [], + adhoc_filters: [], + metrics_b: ['count'], + groupby_b: [], + adhoc_filters_b: [], + row_limit: 100, + row_limit_b: 100, + truncate_metric: true, + truncate_metric_b: true, + comparison_type: 'values', + color_scheme: 'supersetColors', + }; + const chartResp = await apiPost(page, 'api/v1/chart/', { + slice_name: `mixed_filter_repro_${Date.now()}`, + viz_type: 'mixed_timeseries', + datasource_id: datasetId, + datasource_type: 'table', + params: JSON.stringify(chartParams), + }); + expect(chartResp.ok()).toBe(true); + const chartId: number = (await chartResp.json()).id; Review Comment: **Suggestion:** The chart creation response is parsed as if `id` is always top-level, but this API can return `{ result: { id } }` in some environments. When that shape is returned, the computed chart ID becomes `undefined`, which then breaks dashboard linkage and navigation with hard-to-diagnose failures. Parse both response shapes before using the ID. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Mixed-chart dashboard filter regression test fails on some APIs. - ⚠️ CI for issue #29519 may be flaky across deployments. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `superset-frontend/playwright/tests/dashboard/mixed-chart-dashboard-filters.spec.ts:46-84`, the test creates a Mixed chart by POSTing to `api/v1/chart/` via `apiPost` at lines 75-81 and then parses the response as `const chartId: number = (await chartResp.json()).id;` at line 83. 2. The same chart creation endpoint is wrapped by `apiPostChart` in `superset-frontend/playwright/helpers/api/chart.ts:5-9`, and its response is parsed in `superset-frontend/playwright/tests/chart/chart-test-helpers.ts:29-35` using `const id = body.result?.id ?? body.id;` with the comment "Handle both response shapes: { id } or { result: { id } }", demonstrating that in real environments the API can return either `{ id }` or `{ result: { id } }`. 3. A similar dual-shape handling exists for datasets in `superset-frontend/playwright/helpers/api/dataset.ts:1-3`, where the helper returns `body.result?.id ?? body.id ?? null`, further confirming that Superset APIs sometimes wrap created resource IDs under `result`. 4. When the `api/v1/chart/` POST used in the mixed-chart test returns the `{ result: { id } }` shape, `(await chartResp.json()).id` at line 83 is `undefined`, so `chartId` becomes `undefined`; this propagates into `chartLayoutKey` (line 86) and the dashboard linkage update `await apiPut(page, \`api/v1/chart/${chartId}\`, { dashboards: [dashboardId] });` at line 158, causing the created chart not to be correctly linked to the dashboard and leading to no two-query `/api/v1/chart/data` requests and eventual timeout in the `expect.poll` at lines 183-185. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3440c28b860a42c59285443a4be0f05c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3440c28b860a42c59285443a4be0f05c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/dashboard/mixed-chart-dashboard-filters.spec.ts **Line:** 83:83 **Comment:** *Api Mismatch: The chart creation response is parsed as if `id` is always top-level, but this API can return `{ result: { id } }` in some environments. When that shape is returned, the computed chart ID becomes `undefined`, which then breaks dashboard linkage and navigation with hard-to-diagnose failures. Parse both response shapes before using the ID. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40818&comment_hash=f5d6dfb2bcd9cb8907310dda912cfcb70373d91fbf655add1fb6f847a0474164&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40818&comment_hash=f5d6dfb2bcd9cb8907310dda912cfcb70373d91fbf655add1fb6f847a0474164&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]
