codeant-ai-for-open-source[bot] commented on code in PR #34785:
URL: https://github.com/apache/superset/pull/34785#discussion_r3656268115
##########
superset/datasets/schemas.py:
##########
@@ -174,6 +174,7 @@ class DatasetPutSchema(Schema):
columns = fields.List(fields.Nested(DatasetColumnsPutSchema))
metrics = fields.List(fields.Nested(DatasetMetricsPutSchema))
folders = fields.List(fields.Nested(FolderSchema), required=False)
+ drill_through_chart_id = fields.Integer(allow_none=True)
Review Comment:
**Suggestion:** The schema accepts any existing slice ID, but does not
validate that the selected chart uses this dataset as its datasource. A chart
from another dataset can therefore be configured successfully; when
drill-through renders it, the source dataset's drill filters may not match the
target chart's columns or datasource, producing incorrect results or a failed
query. Validate the chart's datasource before persisting the relationship.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Drill-to-details can render results from an unrelated dataset.
- ❌ Cross-dataset filters can cause target-chart query failures.
- ⚠️ Dataset configuration permits invalid chart relationships.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create dataset A and a chart backed by dataset B, then obtain the chart's
slice ID
through the existing chart configuration flow described by this PR.
2. Submit a dataset update for dataset A through the dataset API path that
deserializes
requests with `DatasetPutSchema` in `superset/datasets/schemas.py:156`; set
`drill_through_chart_id` to dataset B's chart ID.
3. `DatasetPutSchema.drill_through_chart_id` at
`superset/datasets/schemas.py:177` only
checks that the value is an integer or null; it performs no slice lookup and
no datasource
comparison, so the cross-dataset ID passes schema validation.
4. Open a dashboard containing a chart based on dataset A and invoke
drill-to-details. The
PR's configured-target flow renders the selected chart, causing filters
originating from
dataset A to be applied to a chart backed by dataset B; incompatible column
filters can
produce an incorrect result or a failed query.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90e32f2001644564a5d6c0a72772c220&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=90e32f2001644564a5d6c0a72772c220&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/datasets/schemas.py
**Line:** 177:177
**Comment:**
*Logic Error: The schema accepts any existing slice ID, but does not
validate that the selected chart uses this dataset as its datasource. A chart
from another dataset can therefore be configured successfully; when
drill-through renders it, the source dataset's drill filters may not match the
target chart's columns or datasource, producing incorrect results or a failed
query. Validate the chart's datasource before persisting the relationship.
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%2F34785&comment_hash=e3a69d648010476f46a955bcfda682fcc1876326a485be2b6a0923a746e11741&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=e3a69d648010476f46a955bcfda682fcc1876326a485be2b6a0923a746e11741&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/hooks/useDashboardFormData.test.tsx:
##########
@@ -0,0 +1,171 @@
+/**
+ * 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 { renderHook } from '@testing-library/react-hooks';
+import { Provider } from 'react-redux';
+import configureMockStore from 'redux-mock-store';
+import { useDashboardFormData } from './useDashboardFormData';
+
+const mockStore = configureMockStore([]);
+
+const createMockState = (overrides = {}) => ({
+ dashboardInfo: {
+ id: 123,
+ metadata: {
+ chart_configuration: {},
+ },
+ },
+ dashboardState: {
+ sliceIds: [1, 2, 3],
+ },
+ nativeFilters: {
+ filters: {},
+ },
+ dataMask: {},
+ ...overrides,
+});
+
+const renderUseDashboardFormData = (
+ chartId: number | null | undefined,
+ state = {},
+) => {
+ const store = mockStore(createMockState(state));
+ return renderHook(() => useDashboardFormData(chartId), {
+ wrapper: ({ children }) => <Provider store={store}>{children}</Provider>,
+ });
+};
+
+test('returns base dashboard context when chartId is null', () => {
+ const { result } = renderUseDashboardFormData(null);
+
+ expect(result.current).toEqual({ dashboardId: 123 });
+});
+
+test('returns base dashboard context when chartId is undefined', () => {
+ const { result } = renderUseDashboardFormData(undefined);
+
+ expect(result.current).toEqual({ dashboardId: 123 });
+});
+
+test('returns base dashboard context when required state is missing', () => {
+ const { result } = renderUseDashboardFormData(1, {
+ nativeFilters: null,
+ });
+
+ expect(result.current).toEqual({ dashboardId: 123 });
+});
+
+test('returns base dashboard context when no filters apply to chart', () => {
+ const { result } = renderUseDashboardFormData(1, {
+ nativeFilters: {
+ filters: {
+ 'filter-1': {
+ scope: [2, 3], // Doesn't include chartId 1
+ },
+ },
+ },
+ dashboardInfo: {
+ id: 123,
+ metadata: {
+ chart_configuration: {
+ 'filter-1': {
+ id: 'filter-1',
+ scope: [2, 3],
+ },
+ },
+ },
+ },
+ });
+
+ expect(result.current).toEqual({ dashboardId: 123 });
+});
+
+test('returns dashboard context with extra form data when filters apply', ()
=> {
+ const mockState = {
+ nativeFilters: {
+ filters: {
+ 'filter-1': {
+ scope: [1, 2, 3], // Includes chartId 1
+ },
+ },
+ },
+ dashboardInfo: {
+ id: 123,
+ metadata: {
+ chart_configuration: {
+ 'filter-1': {
+ id: 'filter-1',
+ scope: [1, 2, 3],
+ },
+ },
+ },
+ },
+ dataMask: {
+ 'filter-1': {
+ extraFormData: {
+ filters: [
+ {
+ col: 'country',
+ op: 'IN',
+ val: ['USA', 'Canada'],
+ },
+ ],
+ },
+ },
+ },
+ };
+
+ // Mock the external utility functions
+ jest.mock('../util/activeAllDashboardFilters', () => ({
+ getAllActiveFilters: () => ({
+ 'filter-1': {
+ scope: [1, 2, 3],
+ },
+ }),
+ }));
+
+ jest.mock('../components/nativeFilters/utils', () => ({
+ getExtraFormData: () => ({
+ filters: [
+ {
+ col: 'country',
+ op: 'IN',
+ val: ['USA', 'Canada'],
+ },
+ ],
+ }),
+ }));
Review Comment:
**Suggestion:** These `jest.mock` calls execute inside the test after
`useDashboardFormData` and its dependencies have already been imported and
evaluated, so they do not reliably replace the functions used by the hook. As a
result, this test does not actually isolate or verify the mocked filter
behavior and may pass without testing the intended extra-form-data path. Move
the mocks to module scope or use `jest.doMock` before dynamically importing the
hook. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ The extra-form-data test does not reliably isolate utility behavior.
- ⚠️ Regression tests may pass without exercising the intended mock path.
- ⚠️ Future utility changes can make this test flaky or misleading.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Jest test file
`superset-frontend/src/dashboard/hooks/useDashboardFormData.test.tsx`; the
hook is
statically imported at line 22 before any test body executes.
2. Jest evaluates `useDashboardFormData` and its imports, including
`../util/activeAllDashboardFilters` and `../components/nativeFilters/utils`,
before
entering the test at line 98.
3. During that test, the `jest.mock` calls at lines 134 and 142 execute only
after those
dependencies have already been imported and cached.
4. The test renders `useDashboardFormData(1, mockState)` at line 154, so the
hook can call
the real imported utilities rather than the intended mock implementations;
the assertion
at lines 156-157 may therefore pass or fail based on production utility
behavior instead
of the explicitly configured mock return values.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=50a05a8bbfe54a71ae718e3c1189a877&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=50a05a8bbfe54a71ae718e3c1189a877&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/hooks/useDashboardFormData.test.tsx
**Line:** 133:152
**Comment:**
*Possible Bug: These `jest.mock` calls execute inside the test after
`useDashboardFormData` and its dependencies have already been imported and
evaluated, so they do not reliably replace the functions used by the hook. As a
result, this test does not actually isolate or verify the mocked filter
behavior and may pass without testing the intended extra-form-data path. Move
the mocks to module scope or use `jest.doMock` before dynamically importing the
hook.
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%2F34785&comment_hash=b29db4e6c5a7425369b9301e22b10b3c44c744148c84ade70eb6c3d8302e58ad&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=b29db4e6c5a7425369b9301e22b10b3c44c744148c84ade70eb6c3d8302e58ad&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:
##########
@@ -233,6 +236,10 @@ export default function DrillDetailPane({
// Download page of results & trim cache if page not in cache
useEffect(() => {
+ // Skip table data fetching if we're using a drill-through chart
+ if (dataset?.drill_through_chart_id) {
+ return;
+ }
Review Comment:
**Suggestion:** The table fetch is skipped solely because
`dataset.drill_through_chart_id` is set, even when `drillThroughFormData` is
still null or failed to load. In that state the configured chart cannot be
rendered, but the fallback table also has no results, leaving the drill-detail
pane blank or empty. Only skip the table fetch once the drill-through form data
is available, or render an explicit loading/error state and restore the normal
fallback when it is unavailable. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Drill details can show blank content during chart configuration loading.
- ⚠️ Normal table fallback is disabled before chart data is ready.
- ⚠️ Drill-through failures lack an explicit recovery state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dataset with `drill_through_chart_id` and open a dashboard
chart that uses
this dataset, causing the drill-detail flow to render `DrillDetailPane` in
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:81`.
2. During the drill-through chart configuration/load flow, render
`DrillDetailPane` while
its optional `drillThroughFormData` prop at
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:85-90`
is still
`null` or unavailable.
3. The effect at
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:239-242`
returns
solely because `dataset?.drill_through_chart_id` is set, so
`getDatasourceSamples()` is
never called by the normal fallback path at lines 243-250.
4. The render branch at
`superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:299-311`
also
cannot create the `StatefulChart` because it requires
`drillThroughFormData`; the table
therefore has no fetched results and the pane can remain blank until the
chart data
becomes available, with no explicit loading or error fallback.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c24046218ea145d8ab52a354f454abf1&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=c24046218ea145d8ab52a354f454abf1&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/components/Chart/DrillDetail/DrillDetailPane.tsx
**Line:** 239:242
**Comment:**
*Possible Bug: The table fetch is skipped solely because
`dataset.drill_through_chart_id` is set, even when `drillThroughFormData` is
still null or failed to load. In that state the configured chart cannot be
rendered, but the fallback table also has no results, leaving the drill-detail
pane blank or empty. Only skip the table fetch once the drill-through form data
is available, or render an explicit loading/error state and restore the normal
fallback when it is unavailable.
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%2F34785&comment_hash=0eba7efcceaf066e136c9e0a5e548ab9372ac5e5eda94f61f5a0d7692678aceb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34785&comment_hash=0eba7efcceaf066e136c9e0a5e548ab9372ac5e5eda94f61f5a0d7692678aceb&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]
