codeant-ai-for-open-source[bot] commented on code in PR #40475:
URL: https://github.com/apache/superset/pull/40475#discussion_r3530970999
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -406,6 +409,21 @@ const FiltersConfigForm = (
const datasetId = getDatasetId();
+ const getDatasourceType = (): DatasourceType => {
+ if (formFilter?.datasourceType) {
+ return formFilter.datasourceType;
+ }
+ if (isChartCustomization) {
+ return (
+ customizationToEdit?.targets?.[0]?.datasourceType ||
+ DatasourceType.Table
+ );
+ }
+ return filterToEdit?.targets?.[0]?.datasourceType || DatasourceType.Table;
+ };
Review Comment:
**Suggestion:** Defaulting an absent target datasource type to `table`
changes behavior for legacy saved filters that intentionally had no type,
because they now behave as table-only instead of ID-only/wildcard. This can
alter scoping and chart matching immediately when editing old filters even
before users intentionally choose a datasource type. Preserve `undefined` for
legacy records unless the user explicitly picks a datasource kind. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard filter scoping excludes semantic-view charts unexpectedly.
- ⚠️ Legacy filters gain table-only type just by editing.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dashboard with a legacy native filter whose target has only
`datasetId` set
and no `datasourceType`, so the front-end `Filter` object for that filter
(superset-frontend/src/dashboard/types.ts:58-78) carries `targets: [{
datasetId: 7 }]` and
is passed as `filterToEdit` into `FiltersConfigForm`
(superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:290-307).
2. Open the native filters configuration modal for this filter; inside
`FiltersConfigForm`
the helper `getDatasetId()` (same file lines 393-400) resolves `datasetId`
from
`filterToEdit.targets[0].datasetId`, while `getDatasourceType()` (lines
412-423) sees
`formFilter.datasourceType` and `filterToEdit.targets[0].datasourceType`
both undefined
and therefore returns the fallback `DatasourceType.Table`.
3. The computed `datasourceType` (line 46 in the same block) is stored back
into the form
via the hidden field `['filters', filterId, 'datasourceType']`
(FiltersConfigForm.tsx:191-194), so a previously untyped legacy filter now
has
`datasourceType: DatasourceType.Table` purely as a side effect of opening
the modal,
before the user explicitly chooses a datasource kind.
4. When initial scoping defaults are computed, `initiallyExcludedCharts`
(FiltersConfigForm.tsx:879-899) calls
`doesChartMatchFilterDatasource(chart.form_data.datasource, loadedDatasets,
formFilter.dataset.value, datasourceType)` for each chart; for charts using
semantic-view
datasources like `'7__semantic_view'` (see test fixture in
utils.test.ts:22-27),
`doesChartMatchFilterDatasource`
(superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts:211-234)
compares `expectedType='table'` to `loadedType='semantic_view'` and returns
false,
pre-excluding those charts from the filter and breaking the wildcard ID-only
semantics
used elsewhere in `getRelatedCharts.ts:13-36` and
`getFormDataWithExtraFilters.ts:13-23`
where a missing `datasourceType` intentionally matches any type.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=91d500afc48d437c9eb97a880f0211f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=91d500afc48d437c9eb97a880f0211f2&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/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
**Line:** 412:423
**Comment:**
*Logic Error: Defaulting an absent target datasource type to `table`
changes behavior for legacy saved filters that intentionally had no type,
because they now behave as table-only instead of ID-only/wildcard. This can
alter scoping and chart matching immediately when editing old filters even
before users intentionally choose a datasource type. Preserve `undefined` for
legacy records unless the user explicitly picks a datasource kind.
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%2F40475&comment_hash=c99626786435daa24a6b14fd690fba8cbccae71f922b7462ae781dc94ab68e5a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40475&comment_hash=c99626786435daa24a6b14fd690fba8cbccae71f922b7462ae781dc94ab68e5a&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts:
##########
@@ -144,3 +188,48 @@ export const mostUsedDataset = (
return datasets[mostUsedDataset]?.id;
};
+
+const normalizeDatasourceType = (datasourceType?: string): string =>
+ datasourceType || DatasourceType.Table;
+
+const parseDatasourceUid = (
+ datasourceUid?: string,
+): { id?: number; type?: string } => {
+ if (!datasourceUid) {
+ return {};
+ }
+
+ const [rawId, type] = String(datasourceUid).split('__');
+ const id = Number(rawId);
+ if (Number.isNaN(id)) {
+ return {};
+ }
+
+ return { id, type };
+};
+
+export const doesChartMatchFilterDatasource = (
+ chartDatasourceUid: string | undefined,
+ loadedDatasets: DatasourcesState,
+ filterDatasetId: number,
+ filterDatasourceType?: DatasourceType,
+): boolean => {
+ const expectedType = normalizeDatasourceType(filterDatasourceType);
+ const loadedDataset = chartDatasourceUid
+ ? loadedDatasets[chartDatasourceUid]
+ : undefined;
+
+ if (loadedDataset) {
+ const loadedType = normalizeDatasourceType(
+ loadedDataset.datasource_type || loadedDataset.type,
+ );
+
+ return loadedDataset.id === filterDatasetId && loadedType === expectedType;
+ }
+
+ const parsed = parseDatasourceUid(chartDatasourceUid);
+ return (
+ parsed.id === filterDatasetId &&
+ normalizeDatasourceType(parsed.type) === expectedType
+ );
+};
Review Comment:
**Suggestion:** The datasource matching helper now coerces missing
datasource types to `table` and then requires strict type equality. This breaks
the existing legacy contract where missing `datasourceType` is treated as a
wildcard and only dataset ID is matched, so older filters/customizations can
silently stop matching semantic-view charts with the same ID. Keep `undefined`
as wildcard and only enforce type equality when both sides explicitly provide a
type. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Semantic-view charts not matched by legacy filter targets.
- ⚠️ Scoping UI misidentifies charts due to coerced type.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Set up a dashboard where `loadedDatasets` contains both `'7__table'` and
`'7__semantic_view'` entries, for example as in the test fixture at
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts:22-27,
representing a table and a semantic view that share numeric ID 7 but differ
in datasource
type.
2. In the native filters modal, configure a filter that targets dataset ID 7;
`FiltersConfigForm` resolves `datasetId` via `getDatasetId()`
(FiltersConfigForm.tsx:393-400) and `datasourceType` via
`getDatasourceType()`
(FiltersConfigForm.tsx:412-423), which returns `DatasourceType.Table` when
the form field
and `filterToEdit.targets[0].datasourceType` are undefined, and this value
is stored in
the form under `['filters', filterId, 'datasourceType']`
(FiltersConfigForm.tsx:191-194).
3. While computing default scoping exclusions, `initiallyExcludedCharts`
(FiltersConfigForm.tsx:879-899) iterates over `charts`, obtains each chart’s
`form_data.datasource` UID (e.g. `'7__table'` or `'7__semantic_view'`), and
calls
`doesChartMatchFilterDatasource(chartDatasetUid, loadedDatasets,
formFilter.dataset.value,
datasourceType)` with `filterDatasetId=7` and
`filterDatasourceType=DatasourceType.Table`.
4. Inside `doesChartMatchFilterDatasource` (utils.ts:211-234),
`normalizeDatasourceType`
(utils.ts:192-194) coerces the expected type and loaded dataset type,
defaulting any
missing type to `DatasourceType.Table`; for a semantic-view chart whose
loaded dataset has
`datasource_type: DatasourceType.SemanticView`, `loadedDataset.id === 7` but
`loadedType='semantic_view'` while `expectedType='table'`, so the helper
returns false and
the chart ID is added to `initiallyExcludedCharts`, contradicting the
documented wildcard
contract in `getFormDataWithExtraFilters.ts:16-23` and
`getRelatedCharts.ts:14-32` where
missing `datasourceType` on either side is supposed to allow ID-only matches
so legacy
filters and customizations continue to apply.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=391bd4131dbd4f5fbd6e51258d4caf86&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=391bd4131dbd4f5fbd6e51258d4caf86&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/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
**Line:** 192:235
**Comment:**
*Api Mismatch: The datasource matching helper now coerces missing
datasource types to `table` and then requires strict type equality. This breaks
the existing legacy contract where missing `datasourceType` is treated as a
wildcard and only dataset ID is matched, so older filters/customizations can
silently stop matching semantic-view charts with the same ID. Keep `undefined`
as wildcard and only enforce type equality when both sides explicitly provide a
type.
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%2F40475&comment_hash=b28228af9eb5da440c92f394567ed781018b48564a453b2dc6a3fdc3de5e424a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40475&comment_hash=b28228af9eb5da440c92f394567ed781018b48564a453b2dc6a3fdc3de5e424a&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]
