codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r3509779738
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1247,6 +1247,36 @@ def _table_sort_query_context(
return query_context
+def _series_limit_metric_query_context(
+ mocker: MockerFixture,
+ requested_metric: Any,
+ *,
+ stored_metrics: Optional[list[Any]] = None,
+ form_metric_key: str = "series_limit_metric",
+) -> Any:
+ """
+ Build a minimal chart query context with a series-limit metric selector.
+ """
+ metrics: list[Any] = stored_metrics if stored_metrics is not None else
["count"]
+ query_context = mocker.MagicMock()
+ query_context.queries = [
+ QueryObject(metrics=metrics, series_limit_metric=requested_metric),
+ ]
Review Comment:
**Suggestion:** The helper always injects `series_limit_metric` into
`QueryObject` regardless of `form_metric_key`, so the deprecated-key test does
not actually isolate `timeseries_limit_metric` handling and can pass even if
form-data support for that key regresses. Build the query object with the
matching metric key (or omit query-level metric in that test) so the test
verifies the intended contract. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Deprecated series-limit key coverage weaker than expected.
- ⚠️ Future regressions in timeseries_limit_metric may go undetected.
- ⚠️ Guest metric selector authorization tests give false sense of safety.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/security/manager_test.py` and locate
`_series_limit_metric_query_context` at lines 51–78 (in the read snapshot:
helper starts
around line 51, `query_context.queries` assignment at lines 63–65). Note
that it always
builds `QueryObject(metrics=metrics, series_limit_metric=requested_metric)`
regardless of
the `form_metric_key` argument.
2. Open `superset/security/manager.py` and inspect
`_series_limit_metric_modified` at
lines 720–31. Observe that it loops over both keys `("series_limit_metric",
"timeseries_limit_metric")` for values in `form_data` and for each `query`
via
`getattr(query, key, None)`.
3. Inspect the test
`test_query_context_modified_timeseries_limit_metric_stored_metric_allowed`
in
`tests/unit_tests/security/manager_test.py` at lines 747–760. The test passes
`form_metric_key="timeseries_limit_metric"` to the helper, so `form_data`
uses the
deprecated key, but `query_context.queries` still carries
`series_limit_metric=requested_metric` due to the helper’s hardcoded field.
4. Consider a regression where `_series_limit_metric_modified` stops checking
`form_data.get("timeseries_limit_metric")` but still checks
`series_limit_metric` on
queries (e.g., by narrowing the first loop to just `"series_limit_metric"`).
Running the
tests in this scenario would still make
`test_query_context_modified_timeseries_limit_metric_stored_metric_allowed`
pass, because
the query-level `series_limit_metric` remains valid, demonstrating that the
helper’s
hardcoding can hide regressions in deprecated `timeseries_limit_metric`
form-data
handling.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cef4e2a6b4f549799307b3d9fd3553b1&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=cef4e2a6b4f549799307b3d9fd3553b1&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/security/manager_test.py
**Line:** 1262:1264
**Comment:**
*Logic Error: The helper always injects `series_limit_metric` into
`QueryObject` regardless of `form_metric_key`, so the deprecated-key test does
not actually isolate `timeseries_limit_metric` handling and can pass even if
form-data support for that key regresses. Build the query object with the
matching metric key (or omit query-level metric in that test) so the test
verifies the intended contract.
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%2F37371&comment_hash=52242666de820c9623dfebda8393e577132a0ea5bbe375cbd7134f5fcb7933ec&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=52242666de820c9623dfebda8393e577132a0ea5bbe375cbd7134f5fcb7933ec&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -2166,3 +2682,204 @@ def
test_reset_password_self_service_pk_string_clears_flag(
# Coerced to int when clearing, regardless of the inbound id type.
mock_clear.assert_called_once_with(5)
+
+
+# -----------------------------------------------------------------------------
+# Tests for orderby with invalid formats - integration tests
+# -----------------------------------------------------------------------------
Review Comment:
**Suggestion:** This header is factually incorrect: these are unit tests
using mocks in a unit-test module, not integration tests, which misleads future
maintainers about test scope and guarantees; update the comment to reflect
unit-test coverage. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Comment misleads about invalid orderby test scope.
- ⚠️ Maintainers may overestimate integration coverage here.
- ⚠️ Suggestion addresses documentation, not runtime behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/security/manager_test.py` and navigate to the
section header
around lines 2687–2689, where the comment reads `# Tests for orderby with
invalid formats
- integration tests`.
2. Confirm from the file path (`tests/unit_tests/security/manager_test.py`)
and
surrounding code (all tests in this module use `mocker.MagicMock` and do not
hit real
services or databases) that these are unit tests, not integration tests.
3. Observe that the comment explicitly labels the group as “integration
tests,” which
misrepresents the scope and may mislead maintainers about the guarantees
these tests
provide.
4. Because this is purely a documentation mismatch and does not affect
runtime behavior or
test execution, the issue does not manifest as a functional bug but can
cause confusion
about coverage; updating the comment would align documentation with actual
test type.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bd50a9ba5dd74ad0a4d577531c3732e4&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=bd50a9ba5dd74ad0a4d577531c3732e4&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/security/manager_test.py
**Line:** 2687:2689
**Comment:**
*Comment Mismatch: This header is factually incorrect: these are unit
tests using mocks in a unit-test module, not integration tests, which misleads
future maintainers about test scope and guarantees; update the comment to
reflect unit-test coverage.
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%2F37371&comment_hash=8749de420eabd8076ad616d3f886d9fc2706aecc3cb3916b806a4743a34dbf3a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=8749de420eabd8076ad616d3f886d9fc2706aecc3cb3916b806a4743a34dbf3a&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]
