Repository navigation
Conversation
Code Review Agent Run #9c3488Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44080 +/- ##
=======================================
Coverage 82.85% 82.85%
=======================================
Files 3017 3017
Lines 192923 192958 +35
Branches 44976 44985 +9
=======================================
+ Hits 159845 159882 +37
+ Misses 30038 30037 -1
+ Partials 3040 3039 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Good catch on the same-database repoint gap, and the tests are solid. One thing before this merges, though: bito's right that the fallback here doesn't match what actually gets applied. update_from_object (connectors/sqla/models.py:788) does setattr(self, attr, obj.get(attr)) with no default, so an omitted table_name/schema/catalog key gets set to None. But the check here falls back to the current value when the key is missing, so table_changed reads False and the new access check never runs, even though the dataset is about to get repointed (to None, in this case). Left a suggestion inline.
|
Heya, checking back in on this one — the suggestion above is still open. Should be a quick one whenever you get a chance. |
|
@rusackas you and bito were right — Removing the fallback surfaced two cases the stricter comparison would have caught wrongly, fixed in df17cab: a virtual dataset's |
|
@rusackas one more in 96e2c48, from a self-review pass on the same block: the virtual-dataset skip needed a companion clause for the conversion case. Dropping Rest of the commit is cleanup: the source-binding comparison is field-by-field rather than |
Code Review Agent Run #ef0dceActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #eeb642Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas one tidy-up pass on top in 755e76d, no behaviour change to the rule you flagged: the two conversion clauses fold into a single condition, the requested schema/catalog are normalised at construction so the same value is compared and authorised, and the virtual test reuses the existing |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks for the omitted-key fix, that reads right the way update_from_object actually applies it now.
One thing before this merges, though: the follow-up moved table_changed's computation above any type check, and it reads orm_datasource.table_name/.is_virtual unconditionally. Query, SavedQuery and SemanticView datasources don't have those attributes (Query only has tmp_table_name), so saving any non-table datasource through this endpoint now 500s, even one that never touches the database. Might be worth scoping the block to orm_datasource.type == DatasourceType.TABLE?
|
@rusackas quality pass in 4449a33, no change to the rule you flagged: the requested and current targets are now two |
Code Review Agent Run #edbae6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas right, thanks — a query has |
Code Review Agent Run #463827Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas both points are in already: the omitted-key read in d16ddd8, and the non-dataset guard in 5ce70d6 ( One follow-up in b05b995 from a tidy-up pass, no behaviour change: the "result is still virtual" skip moved out of the helper and into the caller's The red CI here is not from this branch: |
|
@rusackas both of your points were already in before your last pass, so nothing new was needed for them: the omitted-key read in d16ddd8, and the non-dataset guard in 5ce70d6, where One docs-only commit on top in 7ecbbe4, no behaviour change: On the red CI: |
Code Review Agent Run #53995aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas no change to either rule you raised, but the guard you asked about moved in 0da560b: the datasource-type and still-virtual checks now live inside Unrelated to this diff: the current red CI is |
Code Review Agent Run #85c0b7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #e92ffbActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas both points you raised are already in and unchanged: the omitted-key read in d16ddd8, and the non-dataset guard in 5ce70d6 ( Correction on my earlier CI note: the One cleanup commit on top in 8448ff6, no behaviour change: the target database is resolved inside the branch that reads it rather than eagerly in an |
Code Review Agent Run #f4a14cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas one commit on top in e9d4ad8, no behaviour change: comments trimmed where Both points you raised are unchanged and still in: the omitted-key read in d16ddd8, the non-dataset guard in 5ce70d6. On the red CI, all four failures are one cause and it is not from this branch: |
Code Review Agent Run #be25f0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas correcting my last CI note: the Both points you raised remain in and unchanged: the omitted-key read in d16ddd8, the non-dataset guard in 5ce70d6. One cleanup commit on top in a5aef64, no behaviour change: three mock stubs that set |
Code Review Agent Run #8480ccActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Move the datasource-type and still-virtual guards into _repoints_table so the predicate is self-contained and no longer relies on a precondition the caller has to enforce. Behaviour is unchanged: the type guard still short-circuits ahead of table_name/is_virtual. Also route the two remaining save tests through the shared _run_save helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
``_repoints_table`` took the whole request payload to read one key. Pass the requested ``sql`` directly so the helper is not coupled to the payload's key names, and trim the prose around it to the two points that are not obvious from the code. Also drops the function-local ``from flask import Flask`` lines in the samples tests, redundant since the module-level import was added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment-only in the view: the helper docstring owns the label-vs-pointer rule, so the call site no longer restates it. Drops the test helper's unused return value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…trized case Quality pass, no behaviour change: the two "repoint allowed" tests differed only in the requested table and the expected target, so they fold into a single parametrized test; the module-level request app is built once; and the comment above ``requested_table`` reads as a sentence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… save tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The view comment on `requested_table` and three test parameter comments restated points already made in `update_from_object`'s docstring and in `_repoints_table`'s. Point at those instead. Also drops a clause on the conversion case that claimed more than the check delivers: `raise_for_access` resolves the requested table through `query_datasources_by_name` and accepts any match the caller can edit, so an unchanged label resolves to the dataset under edit. The clause still covers a conversion that changes the label in the same save, which is what the parameter now says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The save path only checked the table pointer, so a request supplying ``sql`` skipped the check entirely even though ``update_from_object`` applies it and ``is_virtual``/``kind`` derive from whether ``sql`` is set. Ports ``UpdateDatasetCommand._validate_sql_access`` as a sibling predicate, covering new SQL and unchanged SQL whose connection, catalog or schema moves. The two checks need separate ``raise_for_access`` calls, since passing ``sql`` builds an ephemeral query that supersedes ``table``. Also folds the cross-database leg into ``_repoints_table`` so one function owns the whole predicate, and restores the guard on the ``database_id`` write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The save body is free-form JSON, so the target fields can arrive as any type. The access checks parse `sql` and render `table_name`/`schema`/ `catalog`, neither of which survives a non-string, so validate the four up front and answer 422 rather than failing inside the check. Extracts the target parse and the access check into helpers, which keeps `save` under the complexity limit, and drops a docstring clause that described an attribute the function does not read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 422 for a non-string target field built its detail with an f-string interpolated after `_()`, so only the outer sentence reached the catalog and "must be a string" stayed English in every language. That outer sentence also read "Dataset schema is invalid", which points at the dataset's column schema rather than at the request body field that actually failed the type check. One msgid now carries the whole sentence with a `%(field)s` placeholder, and the extraction template is regenerated so the string is translatable. Same pass drops the dict `_requested_target` built only to read back by key, and asserts the response names the offending field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0fc4493 to
5b3b211
Compare
github-actions
Bot
removed
i18n:spanish
CodeAnt PR Risk: Low Risk
Assessed commit: |
github-actions
Bot
removed
the
requires:rebase
This branch has not been deployed
2 participants
SUMMARY
The legacy
Datasource.saveview (POST /superset/datasource/save/) ransecurity_manager.raise_for_access(table=...)only inside theif database_id != orm_datasource.database_id:branch. A request that keeps the samedatabase.idbut changestable_name/schema/catalogalso repoints the dataset (update_from_objectapplies whatever the request supplies), but skipped the target-table check, so it ran only for cross-database repoints.This aligns the legacy view with the create/update paths: resolve the target database and table up front, and run the access check whenever either changes. A plain save that changes neither the database nor the table (a column/metric edit) still needs no recheck, since editorship already gates it.
TESTING INSTRUCTIONS
pytest tests/unit_tests/views/datasource/views_test.pyAdds
test_save_rejects_same_database_repoint_to_table_without_access(same-DB repoint is now checked) andtest_save_allows_unchanged_datasource_without_access_recheck(no behavior change for plain edits). Existing cross-database repoint tests still pass.ADDITIONAL INFORMATION