codeant-ai-for-open-source[bot] commented on code in PR #41218:
URL: https://github.com/apache/superset/pull/41218#discussion_r3440313434
##########
superset/security/manager.py:
##########
@@ -520,42 +520,87 @@ def _native_filter_request_modified(query_context:
"QueryContext") -> bool:
)
-def query_context_modified(query_context: "QueryContext") -> bool:
+def _collect_sortable_identifiers(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
"""
- Check if a query context has been modified.
-
- This is used to ensure guest users don't modify the payload and fetch data
- different from what was shared with them in dashboards.
+ Frozen column names and metric labels/definitions a guest may legitimately
+ sort by: every column or metric the stored chart already references.
+
+ Order-by only changes the ordering of the result, not which data is read,
so
+ any column or metric already part of the chart is a safe sort target. A
term
+ that is not present in the stored chart (for example a free-form
``random()``
+ expression) cannot be validated and must be rejected. Order-by entries are
+ ``(column_or_metric, ascending)`` pairs, so only their first element is
+ collected.
"""
- form_data = query_context.form_data
- stored_chart = query_context.slice_
+ allowed: set[str] = set()
- # Native-filter data requests have no associated chart (no slice_id).
Rather
- # than accepting any payload, constrain them to the column(s) the
dashboard's
- # native filter is allowed to target; other chartless paths keep prior
- # behavior (see _native_filter_request_modified).
- if stored_chart is None:
- return _native_filter_request_modified(query_context)
+ def add(values: Any) -> None:
+ for value in values or []:
+ allowed.add(freeze_value(value))
Review Comment:
**Suggestion:** Add a short docstring to this newly introduced helper
function so the intent and expected input shape are documented inline.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced Python function in a changed hunk, and it does not
include a docstring. The custom rule requires new Python functions and
classes
to be documented inline, so the suggestion correctly identifies a real
violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6c5da182ed2f4c4b80c5014604738ecd&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=6c5da182ed2f4c4b80c5014604738ecd&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/security/manager.py
**Line:** 540:542
**Comment:**
*Custom Rule: Add a short docstring to this newly introduced helper
function so the intent and expected input shape are documented inline.
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%2F41218&comment_hash=218d9e87f1f2894777c20c2ecabe5d2e2722d6dbc43eaf1d33b517dc00967188&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41218&comment_hash=218d9e87f1f2894777c20c2ecabe5d2e2722d6dbc43eaf1d33b517dc00967188&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -520,42 +520,87 @@ def _native_filter_request_modified(query_context:
"QueryContext") -> bool:
)
-def query_context_modified(query_context: "QueryContext") -> bool:
+def _collect_sortable_identifiers(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
"""
- Check if a query context has been modified.
-
- This is used to ensure guest users don't modify the payload and fetch data
- different from what was shared with them in dashboards.
+ Frozen column names and metric labels/definitions a guest may legitimately
+ sort by: every column or metric the stored chart already references.
+
+ Order-by only changes the ordering of the result, not which data is read,
so
+ any column or metric already part of the chart is a safe sort target. A
term
+ that is not present in the stored chart (for example a free-form
``random()``
+ expression) cannot be validated and must be rejected. Order-by entries are
+ ``(column_or_metric, ascending)`` pairs, so only their first element is
+ collected.
"""
- form_data = query_context.form_data
- stored_chart = query_context.slice_
+ allowed: set[str] = set()
- # Native-filter data requests have no associated chart (no slice_id).
Rather
- # than accepting any payload, constrain them to the column(s) the
dashboard's
- # native filter is allowed to target; other chartless paths keep prior
- # behavior (see _native_filter_request_modified).
- if stored_chart is None:
- return _native_filter_request_modified(query_context)
+ def add(values: Any) -> None:
+ for value in values or []:
+ allowed.add(freeze_value(value))
- if form_data is None:
- return False
+ def add_orderby(entries: Any) -> None:
+ for entry in entries or []:
+ if isinstance(entry, (list, tuple)) and entry:
+ allowed.add(freeze_value(entry[0]))
Review Comment:
**Suggestion:** Add a concise docstring to this new helper function to
describe how order-by entries are interpreted and collected. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This helper function was newly added in the PR and has no docstring. Since
the
rule requires newly added Python functions and classes to include docstrings,
this is a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5aa79b6879204bf1a421bd7def0f2d5e&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=5aa79b6879204bf1a421bd7def0f2d5e&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/security/manager.py
**Line:** 544:547
**Comment:**
*Custom Rule: Add a concise docstring to this new helper function to
describe how order-by entries are interpreted and collected.
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%2F41218&comment_hash=b4c4f0f53fce9a8cc2081786996f88f4857c59513b0084de43c3800c984a4598&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41218&comment_hash=b4c4f0f53fce9a8cc2081786996f88f4857c59513b0084de43c3800c984a4598&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]
