codeant-ai-for-open-source[bot] commented on code in PR #41131:
URL: https://github.com/apache/superset/pull/41131#discussion_r3425340398
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -314,3 +314,39 @@ def get_dashboard_filter_context(
)
return context
+
+
+def apply_dashboard_filter_context(
+ query_context: dict[str, Any],
+ extra_form_data: dict[str, Any],
+) -> None:
+ """
+ Apply dashboard filter context.
+
+ Filters are removed from `extra_form_data` to avoid duplicated values when
+ using `filter_values()` macro.
+
+ :param query_context: The chart's query context (mutated in place)
+ :param extra_form_data: The dashboard's merged extra_form_data to apply.
It's
+ also mutated in place.
+ """
+ extra_filters = extra_form_data.pop("filters", [])
+ for query in query_context.get("queries", []):
+ if extra_filters:
+ existing_filters = query.get("filters") or []
+ query["filters"] = existing_filters + [
+ {**flt, "isExtra": True} for flt in extra_filters
+ ]
Review Comment:
**Suggestion:** The new helper only moves `extra_form_data["filters"]` into
`query["filters"]` and applies override keys, but it never applies append-style
keys like `adhoc_filters` from dashboard `extra_form_data`. As a result,
dashboard defaults that rely on `adhoc_filters` are silently dropped and won't
affect the generated query. Extend this function to merge all supported append
keys (at least `adhoc_filters`) the same way existing extra-form-data merge
logic does. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ GET /api/v1/chart data drops dashboard adhoc defaults.
- ⚠️ Dashboard-native filters behave inconsistently across filter types.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a native dashboard filter whose `defaultDataMask.extraFormData`
contains only
`adhoc_filters` (no plain `filters`), so its defaults are expressed as adhoc
predicates;
these defaults are merged into the dashboard context via
`_merge_extra_form_data` at
`superset/charts/data/dashboard_filter_context.py:130-177`, which explicitly
documents
appending list-type keys like `filters` and `adhoc_filters`.
2. Load a chart from that dashboard by calling the GET endpoint
`ChartDataRestApi.get_data` at `superset/charts/data/api.py:86-103` with a
`filters_dashboard_id` query parameter pointing to the dashboard, causing
`get_dashboard_filter_context(...).extra_form_data` to be computed and, if
non-empty,
passed as `efd` into `apply_dashboard_filter_context(json_body, efd)` at
`superset/charts/data/api.py:55-56`.
3. Inside `apply_dashboard_filter_context` at
`superset/charts/data/dashboard_filter_context.py:319-352`, the function
pops only
`extra_form_data["filters"]` (line 333) into `query["filters"]` (lines
335-339) and then
applies only override-style keys (`EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS` and
`EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS`) to `query["extras"]` and
top-level query
fields, never merging append-style keys such as `adhoc_filters` into the
query's
filter/where/having clauses.
4. When the mutated `json_body` is later loaded into a `QueryContext` via
`ChartDataQueryContextSchema().load(json_body)` (see
`superset/charts/schemas.py:1468-1481` and `QueryContextFactory.create` at
`superset/common/query_context_factory.py:45-56`), the resulting
`QueryObject` sees only
the original `query["filters"]` plus the extra dashboard simple filters, but
not any
appended `adhoc_filters` from the dashboard extra_form_data, so those
dashboard-default
adhoc predicates never affect the generated SQL for
`/api/v1/chart/<pk>/data?filters_dashboard_id=...`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2461a244c3e9433bae761659a17b97d2&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=2461a244c3e9433bae761659a17b97d2&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/charts/data/dashboard_filter_context.py
**Line:** 333:339
**Comment:**
*Incomplete Implementation: The new helper only moves
`extra_form_data["filters"]` into `query["filters"]` and applies override keys,
but it never applies append-style keys like `adhoc_filters` from dashboard
`extra_form_data`. As a result, dashboard defaults that rely on `adhoc_filters`
are silently dropped and won't affect the generated query. Extend this function
to merge all supported append keys (at least `adhoc_filters`) the same way
existing extra-form-data merge logic does.
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%2F41131&comment_hash=e023deb2a2daf501b1fc58962912773f6f8285e06a3b87319c2d2b77e05f67f6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41131&comment_hash=e023deb2a2daf501b1fc58962912773f6f8285e06a3b87319c2d2b77e05f67f6&reaction=dislike'>👎</a>
##########
tests/unit_tests/charts/test_chart_data_api.py:
##########
@@ -73,6 +78,36 @@ def
test_get_data_sets_g_form_data_without_dashboard_filter() -> None:
assert g.form_data["queries"][0]["columns"] == ["col1"]
+def test_apply_dashboard_filter_context_does_not_duplicate_filters() -> None:
+ """
+ Regression test for the ``filters_dashboard_id`` parameter.
+
+ A dashboard's filters must not be present in both query["filters"] and
+ query["extra_form_data"]["filters"]. Previously the same filter existed in
both,
+ so Jinja's filter_values() read each value twice and produced SQL such as
+ ``country in ('USA', 'USA')``.
+ """
+ query_context_json: dict[str, Any] = {
+ "datasource": {"id": 1, "type": "table"},
+ "queries": [{"filters": [{"col": "year", "op": "IN", "val": [2004]}]}],
+ }
+ extra_form_data = {"filters": [{"col": "country", "op": "IN", "val":
["USA"]}]}
+
+ apply_dashboard_filter_context(query_context_json, extra_form_data)
+
+ query = query_context_json["queries"][0]
+ assert query["filters"] == [
+ {"col": "year", "op": "IN", "val": [2004]},
+ {"col": "country", "op": "IN", "val": ["USA"], "isExtra": True},
+ ]
+ assert "filters" not in query["extra_form_data"]
+
+ # filter_values() therefore returns the dashboard value exactly once.
+ with current_app.test_request_context("/api/v1/chart/1/data/"):
+ g.form_data = query_context_json
+ assert ExtraCache().filter_values("country") == ["USA"]
Review Comment:
**Suggestion:** This test uses `current_app` without creating/pushing an
application context first, which can raise "Working outside of application
context" when the test is run in isolation. Instantiate a Flask app (as done in
the other tests in this file) and use that app's `test_request_context` to make
the test deterministic. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit test can raise RuntimeError outside app context.
- ⚠️ Regression coverage for filters_dashboard_id becomes flaky.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the single test
`test_apply_dashboard_filter_context_does_not_duplicate_filters` in
`tests/unit_tests/charts/test_chart_data_api.py:81-109` in isolation (for
example via
`pytest
tests/unit_tests/charts/test_chart_data_api.py::test_apply_dashboard_filter_context_does_not_duplicate_filters`),
without any global fixture that pushes a Flask application context.
2. The test constructs `query_context_json` and calls
`apply_dashboard_filter_context`,
then reaches the block starting at line 106: `with
current_app.test_request_context("/api/v1/chart/1/data/"):` which uses
`current_app`
imported from `flask` at line 22.
3. Because no Flask application has been created or pushed for `current_app`
in this
module (unlike the preceding test, which uses a local `app =
Flask(__name__)` and `with
app.test_request_context(...)` at lines 52-55), entering
`current_app.test_request_context(...)` raises `RuntimeError: Working
outside of
application context` before `g.form_data` is set or
`ExtraCache().filter_values("country")` is evaluated.
4. The regression test for `filters_dashboard_id` therefore becomes
environment-dependent:
it passes only when some external test harness has already created and
pushed a Flask app
context, but fails when executed in isolation or under tools that do not
preconfigure
`current_app`, reducing test determinism and making failures harder to
diagnose.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8facaf2ca343434989d678a355c1ee12&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=8facaf2ca343434989d678a355c1ee12&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:** tests/unit_tests/charts/test_chart_data_api.py
**Line:** 106:108
**Comment:**
*Possible Bug: This test uses `current_app` without creating/pushing an
application context first, which can raise "Working outside of application
context" when the test is run in isolation. Instantiate a Flask app (as done in
the other tests in this file) and use that app's `test_request_context` to make
the test deterministic.
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%2F41131&comment_hash=c6c93b14f538ab79de2034f59afb044fe06714ba552f25312c5edfe6d76abce8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41131&comment_hash=c6c93b14f538ab79de2034f59afb044fe06714ba552f25312c5edfe6d76abce8&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]
