gabotorresruiz commented on code in PR #45504:
URL: https://github.com/apache/superset/pull/45504#discussion_r4328593796
##########
superset/mcp_service/app.py:
##########
@@ -329,7 +329,7 @@ def get_default_instructions(
- list_dashboards(request={{"created_by_me": true, "edited_by_me": true}})
- list_datasets(request={{"created_by_me": true, "edited_by_me": true}})
-Use created_by_me for authorship, edited_by_me for edit access, or both
+For dashboards, edited_by_me means direct USER editor (owner), excluding
role/group access. Use created_by_me for authorship, edited_by_me for direct
editor membership, or both
Review Comment:
Two small things on this line.
The `editor` filter resolves through the same direct user subject in
`superset/daos/dataset.py`, `superset/daos/chart.py` and
`superset/daos/report.py` as it does in `superset/daos/dashboard.py`, all four
filtering `Subject.type == 1`. So the `For dashboards,` qualifier reads, in a
block that lists `list_datasets` two lines up, as if datasets resolved it
differently. Dropping the qualifier would make it true everywhere.
Related and not a blocker: the strictness is dashboard only.
`ListChartsRequest`, `ListDatasetsRequest`, `ListReportsRequest` and
`ListDatabasesRequest` carry the same `CreatedByMeMixin` and `EditedByMeMixin`
and still accept `owned_by_me` silently, so a caller that guesses it on
`list_datasets` gets the unscoped list back exactly as it did for
`list_dashboards` before this PR. Worth the same `model_config` in the same
pass, or is dashboards deliberately first?
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -212,6 +212,19 @@ class ListDashboardsRequest(
):
"""Request schema for list_dashboards with clear, unambiguous types."""
+ edited_by_me: bool = Field(
+ default=False,
+ description=(
+ "Only dashboards where the current user is a direct USER editor "
+ "(the dashboard ownership representation). Does not include access
"
+ "inherited from roles or groups. Use this for dashboards I own. "
+ "Combining with created_by_me returns creator OR owner, not both."
+ ),
+ )
+
+ # Never silently discard a misspelled scope filter (e.g. owned_by_me).
+ model_config = ConfigDict(extra="forbid")
Review Comment:
Would you consider pairing this with `UnknownFieldCheckMixin` from
`superset/mcp_service/chart/schemas.py`? `extra="forbid"` gets you
`additionalProperties: false` in the published schema, which is the part I care
about, but the runtime message is just `Extra inputs are not permitted` and
never points the caller at `edited_by_me`, which is the whole thing the PR is
trying to teach.
I tried adding it to the bases and it gives both:
```
Unknown field 'owned_by_me' — did you mean 'edited_by_me'?
Unknown field 'page_szie' — did you mean 'page_size'?
```
with `additionalProperties` still `false` and the `columns` alias still
resolving to `select_columns`. One catch: the before validator raises first, so
the error `type` becomes `value_error` instead of `extra_forbidden` and the
assertion in `test_list_dashboards_rejects_unknown_arguments` would need to
match on the message.
##########
superset/mcp_service/dashboard/tool/get_dashboard_data.py:
##########
@@ -132,6 +132,17 @@ def _summarize_chart_data(
chart_id=result.chart_id,
chart_name=result.chart_name,
chart_type=result.chart_type,
+ fetch_row_limit=request.fetch_row_limit,
+ sample_rows_limit=request.sample_rows,
+ completeness_note=(
+ "Histogram bins may be computed from input rows capped by
fetch_row_limit. "
+ "A single output row and truncated=false do not establish
full-population "
+ "coverage or a defect in the saved chart. Verify the saved
configuration "
+ "and source aggregates before making a distribution claim."
+ if result.chart_type in {"histogram", "histogram_v2"}
+ else "sample_data contains example rows only; use row_count,
total_rows "
Review Comment:
The histogram note is right, but `box_plot` collapses rows in post
processing the same way, so it falls into this `else` branch and gets told the
opposite of the truth.
On a 500 row source I get one output row with `row_count=1`, `total_rows=1`
and `truncated=false` at every limit, while the whisker statistics move with
the cap: `AVG(val)__count` is 3 at `fetch_row_limit=3` and 5 at
`fetch_row_limit=500`, and `__max` goes from 249.5 to 251.5. Same for a chart
with a saved `query_context`, since `request.limit` overwrites `row_limit` on
every saved query. A caller that follows this note and checks `row_count`,
`total_rows` and `truncated` concludes the retrieval was complete when it was
not.
Could the note key off whether the executed query actually ran row
collapsing post processing, rather than a `chart_type` set?
`executed_query_facts` already hands back the operations. Failing that,
`box_plot` wants to be in the first branch with the wording generalised:
```python
if result.chart_type in {"histogram", "histogram_v2", "box_plot"}
```
A `test_box_plot_sample_exposes_input_fetch_limit` alongside the histogram
one would lock it in. Or am I missing a reason histogram is the only shape
worth calling out?
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -292,8 +292,9 @@ async def duplicate_dashboard(
"""
Duplicate an existing dashboard under a new title.
- By default the copy references the same charts as the source.
- Set duplicate_slices=true to also deep-copy every chart into new
+ By default the copy deep-copies charts so subsequent edits are isolated.
+ Set duplicate_slices=false only to deliberately share source charts.
+ With duplicate_slices=true, every chart is copied into new
chart objects owned by you, so edits to the copies never affect
the originals.
Review Comment:
Just a small NIT on the wording an agent reads on every call: the third
sentence still introduces `duplicate_slices=true` as though it were the opt in,
one line after the first says it is the default.
```suggestion
By default the copy deep-copies charts so subsequent edits are isolated:
every chart becomes a new chart object owned by you, so edits to the
copies never affect the originals. Set duplicate_slices=false only to
deliberately share the source charts.
```
--
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]
