fix(sqllab): reject blank saved query and dataset names#41624
Conversation
dosubot
Bot
added
change:backend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| db.session.delete(model) | ||
| db.session.commit() | ||
|
|
||
| def test_create_saved_query_blank_label(self): |
There was a problem hiding this comment.
Suggestion: Add a return type hint to this new test method (for example, indicate it returns no value). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new test method omits a return type annotation, and the custom rule requires Python functions/methods in new or modified code to include type hints. This is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 680:680
**Comment:**
*Custom Rule: Add a return type hint to this new test method (for example, indicate it returns no value).
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
|
The suggestion to add a return type hint to the new test method is correct, as it aligns with standard TypeScript practices for React components and test suites. In TypeScript, test functions that do not return a value should be explicitly typed as To resolve this, you can update the test method signature in Would you like me to fetch all other comments on this PR to validate them and implement fixes for those as well? tests/integration_tests/queries/saved_queries/api_tests.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41624 +/- ##
=======================================
Coverage 64.60% 64.60%
=======================================
Files 2684 2684
Lines 148192 148200 +8
Branches 34138 34140 +2
=======================================
+ Hits 95738 95746 +8
+ Misses 50711 50710 -1
- Partials 1743 1744 +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:
|
| def validate_label(value: str) -> None: | ||
| """Reject blank or whitespace-only saved query labels.""" | ||
| if value is None or not value.strip(): | ||
| raise ValidationError(_("Label must not be empty.")) |
There was a problem hiding this comment.
Suggestion: The new blank-label validator is only defined here but not applied to ImportV1SavedQuerySchema.label, so imported saved queries can still use whitespace-only names and recreate the same broken nameless entries through the import path. Reuse this validator (or equivalent trimming/validation) in the import schema so all creation paths enforce the same rule. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Imported saved queries can still use whitespace-only labels.
- ⚠️ Saved queries list shows nameless rows from imports.
- ⚠️ Inconsistent validation between create and import saved queries.Steps of Reproduction ✅
1. Prepare a v1 import bundle ZIP containing a `queries/<uuid>.yaml` file whose contents
match `ImportV1SavedQuerySchema` (fields `catalog`, `schema`, `label`, `description`,
`sql`, `uuid`, `version`, `database_uuid` as defined in
`superset/queries/saved_queries/schemas.py:49-57`), but set `label` to a whitespace-only
string, e.g. `" "`.
2. Call the saved query import endpoint `POST /api/v1/saved_query/import/` with the ZIP
file as multipart form-data; this endpoint is implemented by `SavedQueryRestApi.import_`
in `superset/queries/saved_queries/api.py:201-210`, which builds `contents` and
instantiates `ImportSavedQueriesCommand(contents, ...)`.
3. The dispatcher command in `superset/commands/query/importers/dispatcher.py:35-52`
creates `v1.ImportSavedQueriesCommand`
(`superset/commands/query/importers/v1/__init__.py:33-42`), which wires `schemas =
{"databases/": ImportV1DatabaseSchema(), "queries/": ImportV1SavedQuerySchema()}`;
`ImportV1SavedQuerySchema.label` at `superset/queries/saved_queries/schemas.py:52` uses
only `allow_none=True, validate=Length(0, 256)` and does not call `validate_label`, so the
whitespace-only `label` passes marshmallow validation unchanged.
4. In `superset/commands/query/importers/v1/utils.py:24-31`, `import_saved_query(config,
overwrite=...)` calls `SavedQuery.import_from_dict(config, recursive=False)` without
trimming or validating `config["label"]`, persisting a `SavedQuery` row whose `label`
column (model at `superset/models/sql_lab.py:32-41`) contains only spaces. When listing
saved queries via `SavedQueryRestApi` list configuration (`list_columns` including
`"label"` at `superset/queries/saved_queries/api.py:108-120`), the frontend renders a
nameless, effectively blank Name cell for this imported query, recreating the original
nameless-entry behavior through the import path even though direct create/update now use
`validators_columns = {"label": validate_label}` and `pre_add` stripping at
`superset/queries/saved_queries/api.py:78-87`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/queries/saved_queries/schemas.py
**Line:** 22:25
**Comment:**
*Incomplete Implementation: The new blank-label validator is only defined here but not applied to `ImportV1SavedQuerySchema.label`, so imported saved queries can still use whitespace-only names and recreate the same broken nameless entries through the import path. Reuse this validator (or equivalent trimming/validation) in the import schema so all creation paths enforce the same rule.
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![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #a3fe20
Actionable Suggestions - 1
-
superset/queries/saved_queries/schemas.py - 1
- Missing unit test coverage · Line 22-25
Additional Suggestions - 1
-
superset/queries/saved_queries/schemas.py - 1
-
Imprecise type annotation · Line 22-22The parameter type annotation `value: str` is imprecise since the validator receives None values (label field allows None in schema). Change to `value: str | None` for accurate type documentation.
-
Review Details
-
Files reviewed - 7 · Commit Range:
c3ada02..c3ada02- superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx
- superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx
- superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
- superset/queries/saved_queries/api.py
- superset/queries/saved_queries/schemas.py
- tests/integration_tests/queries/saved_queries/api_tests.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| def validate_label(value: str) -> None: | ||
| """Reject blank or whitespace-only saved query labels.""" | ||
| if value is None or not value.strip(): | ||
| raise ValidationError(_("Label must not be empty.")) |
There was a problem hiding this comment.
The validate_label function lacks test coverage. Add tests following the pattern in tests/unit_tests/mcp_service/saved_query/tool/test_saved_query_tools.py to verify rejection of blank and whitespace-only labels. Rule [11730] requires comprehensive unit tests for new functionality.
Code Review Run #a3fe20
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| allowed_rel_fields = {"database", "changed_by", "created_by"} | ||
| allowed_distinct_fields = {"catalog", "schema"} | ||
|
|
||
| validators_columns = {"label": validate_label} |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to this newly introduced class-level variable to comply with the requirement for annotating relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is newly added Python code and the class-level variable is unannotated. The rule requires type hints for relevant variables that can be annotated, so this is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/queries/saved_queries/api.py
**Line:** 197:197
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced class-level variable to comply with the requirement for annotating relevant variables.
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
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SQL Lab let users save a query or dataset with an empty/whitespace-only name. The nameless entry then showed up in Saved Queries with a blank, non-clickable Name cell, so it couldn't be opened or managed. Backend: reject blank/whitespace-only `label` on the saved_query API (add & edit) and strip surrounding whitespace before persisting. Frontend: disable the Save/Update action in the Save Query modal and the Save Dataset modal until a non-whitespace name is entered.
… hint Assert 422 (marshmallow ValidationError via validators_columns) instead of 400 for blank saved query labels, and widen validate_label annotation to str | None to match its None handling.
Add focused unit tests covering blank, whitespace-only, and None labels (rejected) and non-blank labels (accepted), so the validator is exercised without the DB-backed integration path.
20dae0a to
eb98cdf
Compare
| assert rv.status_code == 422 | ||
| assert "label" in data["message"] | ||
|
|
||
| def test_create_saved_query_strips_label(self): |
There was a problem hiding this comment.
Suggestion: Add a return type hint to this new test method signature (for example -> None) to comply with the mandatory type-hint rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added test method has no explicit return type hint. That directly matches the type-hint requirement for new or modified Python methods.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 701:701
**Comment:**
*Custom Rule: Add a return type hint to this new test method signature (for example `-> None`) to comply with the mandatory type-hint rule.
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| db.session.commit() | ||
|
|
||
| @pytest.mark.usefixtures("create_saved_queries") | ||
| def test_update_saved_query_blank_label(self): |
There was a problem hiding this comment.
Suggestion: Add a return type hint to this newly introduced test method signature (for example -> None) to meet the type-hint policy. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is another newly introduced Python test method lacking a return type hint, so it violates the stated type-hint rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 728:728
**Comment:**
*Custom Rule: Add a return type hint to this newly introduced test method signature (for example `-> None`) to meet the type-hint policy.
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
Code Review Agent Run #c90a82Actionable 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 |
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
A few small follow-up suggestions:
- Could we add
-> Nonereturn type hints to the three new test methods intests/integration_tests/queries/saved_queries/api_tests.pyfor consistency with the existing convention (and the new tests insaved_queries_schema_test.py)? - I noticed that the submit button is disabled based on
datasetName.trim(), but the submitted value itself isn't trimmed. Would it make sense to trim the name on submit as well, so" foo "is saved as"foo"and the behavior matches the saved-query backend? - One minor documentation nit: the PR description mentions the API returns
400for a blank label, but the validation (and the new tests) expect422. Could we update the description to reflect that?
Address review feedback: - Trim surrounding whitespace from the dataset name when saving, so " foo " is stored as "foo", matching the saved-query backend which strips the label. Add a test covering the trim. - Add `-> None` return hints to the three new saved-query API tests for consistency with the new unit tests.
| post_data = { | ||
| "schema": "schema1", | ||
| "label": label, | ||
| "description": "some description", | ||
| "sql": "SELECT col1, col2 from table1", | ||
| "db_id": example_db.id, | ||
| } |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this request payload dictionary to satisfy the type-hint requirement for relevant local variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new test code introduces a local dictionary without any type annotation, and the rule explicitly requires type hints for relevant variables that can be annotated.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 689:695
**Comment:**
*Custom Rule: Add an explicit type annotation for this request payload dictionary to satisfy the type-hint requirement for relevant local variables.
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| post_data = { | ||
| "schema": "schema1", | ||
| "label": " label1 ", | ||
| "description": "some description", | ||
| "sql": "SELECT col1, col2 from table1", | ||
| "db_id": example_db.id, | ||
| } |
There was a problem hiding this comment.
Suggestion: Add a concrete type annotation for this local dictionary used in the test input so the new code fully complies with required typing. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is another newly added local dictionary in Python code with no type hint, which matches the rule’s requirement to annotate relevant variables in modified code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 709:715
**Comment:**
*Custom Rule: Add a concrete type annotation for this local dictionary used in the test input so the new code fully complies with required typing.
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| saved_query = ( | ||
| db.session.query(SavedQuery).filter(SavedQuery.label == "label1").all()[0] | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this ORM model variable to comply with the rule requiring type hints on relevant variables in new code. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new local ORM result variable is unannotated, and it is a relevant variable that can be type-hinted under the stated rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 732:734
**Comment:**
*Custom Rule: Add an explicit type annotation for this ORM model variable to comply with the rule requiring type hints on relevant variables in new code.
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
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
SQL Lab let users save a query or dataset with an empty/whitespace-only name. The nameless entry then appeared in Saved Queries with a blank, non-clickable Name cell, so it couldn't be opened or managed.
Fix, both layers:
saved_queries): reject a blank/whitespace-onlylabelon add & edit, and strip surrounding whitespace before persisting.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
To add. Before: Save enabled with an empty Name → nameless row in Saved Queries. After: Save disabled until a real name is typed; API returns 422 for a blank
label.TESTING INSTRUCTIONS
POST /api/v1/saved_query/with"label": " "returns 422.ADDITIONAL INFORMATION