codeant-ai-for-open-source[bot] commented on code in PR #36985:
URL: https://github.com/apache/superset/pull/36985#discussion_r2672972851
##########
superset/utils/core.py:
##########
@@ -1058,6 +1058,12 @@ def merge_extra_form_data(form_data: dict[str, Any]) ->
None: # noqa: C901
for fltr in append_filters
if fltr
)
+
+ if granularity_sqla_override := extra_form_data.get("granularity_sqla"):
Review Comment:
**Suggestion:** Truthiness check with the walrus operator will skip valid
but falsy overrides (e.g. empty string or 0). Use an explicit None check so
present but falsy values are applied. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
granularity_sqla_override = extra_form_data.get("granularity_sqla")
if granularity_sqla_override is not None:
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current walrus check uses truthiness, so an intentionally provided but
falsy value (e.g. empty string) would be ignored. Using an explicit
is not None check is safer if the intention is to accept empty-string
overrides. This fixes a real edge-case bug and is safe to change locally.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/core.py
**Line:** 1062:1062
**Comment:**
*Possible Bug: Truthiness check with the walrus operator will skip
valid but falsy overrides (e.g. empty string or 0). Use an explicit None check
so present but falsy values are applied.
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>
##########
tests/integration_tests/explore/api_tests.py:
##########
@@ -254,3 +254,72 @@ def test_get_url_params(test_client, login_as_admin,
chart_id):
"foo": "bar",
"slice_id": str(chart_id),
}
+
+
+def test_granularity_sqla_override_updates_temporal_range_filter_subject(
+ test_client, login_as_admin, chart_id, admin_id, dataset
+):
+ """
+ Test that extra_form_data.granularity_sqla overrides TEMPORAL_RANGE filter
subject.
+
+ The flow is:
+ 1. Chart has a TEMPORAL_RANGE adhoc filter on 'original_time_col'
+ 2. Dashboard applies Time Column native filter selecting 'year' via
extra_form_data
+ 3. Explore API processes form_data through
merge_extra_filters/merge_extra_form_data
+ 4. The TEMPORAL_RANGE filter's subject should be updated to 'year'
+ """
+
+ form_data_with_temporal_filter = {
+ "datasource": f"{dataset.id}__table",
+ "viz_type": "country_map",
+ "time_range": "Last week",
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "some_other_time_col",
+ }
+ ],
+ "extra_form_data": {
+ "granularity_sqla": "year",
+ "time_range": "Last month",
+ },
+ }
+
+ test_form_data_key = "test_granularity_override_key"
+ entry: TemporaryExploreState = {
+ "owner": admin_id,
+ "datasource_id": dataset.id,
+ "datasource_type": dataset.type,
+ "chart_id": chart_id,
+ "form_data": json.dumps(form_data_with_temporal_filter),
+ }
+ cache_manager.explore_form_data_cache.set(test_form_data_key, entry)
+
+ resp = test_client.get(
+ f"api/v1/explore/?form_data_key={test_form_data_key}"
+ f"&datasource_id={dataset.id}&datasource_type={dataset.type}"
+ )
+ assert resp.status_code == 200
+
+ data = json.loads(resp.data.decode("utf-8"))
+ result = data.get("result")
+ form_data = result["form_data"]
+
+ adhoc_filters = form_data.get("adhoc_filters", [])
+ temporal_range_filters = [
+ f for f in adhoc_filters if f.get("operator") == "TEMPORAL_RANGE"
Review Comment:
**Suggestion:** The list comprehension filters adhoc filters by exact
case-sensitive match on `"operator" == "TEMPORAL_RANGE"`. If the operator is
normalized differently (different case) after processing, the test will miss
the filter; compare case-insensitively or normalize the value before comparing
to make the check robust. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
f for f in adhoc_filters if f.get("operator", "").upper() ==
"TEMPORAL_RANGE"
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Normalizing the operator before comparison (e.g., .upper()) makes the test
more robust to incidental casing differences
introduced by processing, without changing the tested behavior. It's a
harmless defensive improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/explore/api_tests.py
**Line:** 313:313
**Comment:**
*Logic Error: The list comprehension filters adhoc filters by exact
case-sensitive match on `"operator" == "TEMPORAL_RANGE"`. If the operator is
normalized differently (different case) after processing, the test will miss
the filter; compare case-insensitively or normalize the value before comparing
to make the check robust.
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>
##########
tests/integration_tests/explore/api_tests.py:
##########
@@ -254,3 +254,72 @@ def test_get_url_params(test_client, login_as_admin,
chart_id):
"foo": "bar",
"slice_id": str(chart_id),
}
+
+
+def test_granularity_sqla_override_updates_temporal_range_filter_subject(
+ test_client, login_as_admin, chart_id, admin_id, dataset
+):
+ """
+ Test that extra_form_data.granularity_sqla overrides TEMPORAL_RANGE filter
subject.
+
+ The flow is:
+ 1. Chart has a TEMPORAL_RANGE adhoc filter on 'original_time_col'
+ 2. Dashboard applies Time Column native filter selecting 'year' via
extra_form_data
+ 3. Explore API processes form_data through
merge_extra_filters/merge_extra_form_data
+ 4. The TEMPORAL_RANGE filter's subject should be updated to 'year'
+ """
+
+ form_data_with_temporal_filter = {
+ "datasource": f"{dataset.id}__table",
+ "viz_type": "country_map",
+ "time_range": "Last week",
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "some_other_time_col",
+ }
+ ],
+ "extra_form_data": {
+ "granularity_sqla": "year",
+ "time_range": "Last month",
+ },
+ }
+
+ test_form_data_key = "test_granularity_override_key"
Review Comment:
**Suggestion:** The test uses a fixed cache key string
`"test_granularity_override_key"`, which can collide with other tests running
in the same process or leave stale state between test runs; make the key unique
per-test (for example incorporate `chart_id` or `dataset.id`) to avoid test
interference and flakiness. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
test_form_data_key =
f"test_granularity_override_key_{chart_id}_{dataset.id}"
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The test writes to a global cache. Reusing a constant key increases the risk
of collisions between tests
running in the same process and can produce flaky failures. Making the key
unique per-test (e.g., include chart_id/dataset.id)
is a simple defensive change that improves reliability.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/explore/api_tests.py
**Line:** 291:291
**Comment:**
*Possible Bug: The test uses a fixed cache key string
`"test_granularity_override_key"`, which can collide with other tests running
in the same process or leave stale state between test runs; make the key unique
per-test (for example incorporate `chart_id` or `dataset.id`) to avoid test
interference and flakiness.
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>
##########
tests/integration_tests/explore/api_tests.py:
##########
@@ -254,3 +254,72 @@ def test_get_url_params(test_client, login_as_admin,
chart_id):
"foo": "bar",
"slice_id": str(chart_id),
}
+
+
+def test_granularity_sqla_override_updates_temporal_range_filter_subject(
+ test_client, login_as_admin, chart_id, admin_id, dataset
+):
+ """
+ Test that extra_form_data.granularity_sqla overrides TEMPORAL_RANGE filter
subject.
+
+ The flow is:
+ 1. Chart has a TEMPORAL_RANGE adhoc filter on 'original_time_col'
+ 2. Dashboard applies Time Column native filter selecting 'year' via
extra_form_data
+ 3. Explore API processes form_data through
merge_extra_filters/merge_extra_form_data
+ 4. The TEMPORAL_RANGE filter's subject should be updated to 'year'
+ """
+
+ form_data_with_temporal_filter = {
+ "datasource": f"{dataset.id}__table",
Review Comment:
**Suggestion:** The `datasource` value is hardcoded to use "__table" but
other tests and code construct the datasource string using the dataset's `type`
property; if `dataset.type` is not "table" this will produce an incorrect
datasource identifier and cause the API to load the wrong dataset or fail to
find it. Use the actual `dataset.type` instead of the literal `"table"`.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
"datasource": f"{dataset.id}__{dataset.type}",
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Other tests in this file build the datasource using dataset.type (see
test_get_from_permalink).
Using the literal "__table" is brittle if dataset.type ever differs;
replacing with f"{dataset.id}__{dataset.type}"
makes the test consistent and less likely to break if dataset fixtures
change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/explore/api_tests.py
**Line:** 273:273
**Comment:**
*Possible Bug: The `datasource` value is hardcoded to use "__table" but
other tests and code construct the datasource string using the dataset's `type`
property; if `dataset.type` is not "table" this will produce an incorrect
datasource identifier and cause the API to load the wrong dataset or fail to
find it. Use the actual `dataset.type` instead of the literal `"table"`.
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>
--
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]
