fix(reports): escape LIKE wildcards in text filter and preserve typed screenshot width#40980
fix(reports): escape LIKE wildcards in text filter and preserve typed screenshot width#40980sadpandajoe wants to merge 1 commit into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The reported issue is correct. The To resolve this, you should coerce the superset/reports/filters.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40980 +/- ##
=======================================
Coverage 64.19% 64.19%
=======================================
Files 2655 2655
Lines 143914 143960 +46
Branches 33178 33189 +11
=======================================
+ Hits 92379 92409 +30
- Misses 49916 49927 +11
- Partials 1619 1624 +5
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:
|
dosubot
Bot
added
the
alert-reports
There was a problem hiding this comment.
Pull request overview
This PR makes two correctness fixes in the Alerts & Reports feature: (1) it ensures user-entered %, _, and \ are treated as literals (not wildcards) in the reports “All Text” filter, and (2) it preserves a user-typed 0 in the custom screenshot-width input so server-side validation can handle it. It also adds regression tests to lock in the intended backend/command validation and frontend input behavior.
Changes:
- Escape
LIKE/ILIKEwildcard characters in the reports list text filter and use an explicitESCAPE '\'clause for literal matching. - Fix ReportModal screenshot width input handling to avoid coercing
0to “empty” by using??andNumber.isNaNlogic. - Add backend unit tests for wildcard escaping / extra_json
autoescapeand update validation, plus a frontend RTL test for the0width behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
superset/reports/filters.py |
Escapes wildcard characters for ILIKE-based “All Text” filtering and adds escape="\" to match literals. |
tests/unit_tests/reports/filters_test.py |
Adds a regression test asserting wildcard/backslash escaping is applied to each ILIKE clause. |
tests/unit_tests/reports/dao_test.py |
Adds tests pinning extra_json.contains(..., autoescape=True) behavior for strings containing %/_. |
tests/unit_tests/commands/report/update_test.py |
Adds update validation tests for extra.dashboard.activeTabs against the model dashboard layout. |
superset-frontend/src/features/reports/ReportModal/index.tsx |
Preserves typed 0 for custom_width by switching ` |
superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx |
Adds RTL test ensuring typed 0 is displayed/preserved and clearing results in an empty field. |
… screenshot width The report/alert list text filter passed user input straight into ILIKE, so % and _ were treated as wildcards. Add a local _escape_like() helper (mirroring the daos/datasource.py precedent) and pass escape="\\" to each filter clause so the characters match literally. The ReportModal custom screenshot-width input used `|| null` / `|| ''`, which silently coerced a typed 0 to "no custom width". Use `?? ''` for display and an explicit Number.isNaN() check on change, so a typed 0 is submitted and surfaced by the server's min-width validation instead of being dropped on the client. Add regression tests covering report DAO autoescape behavior and the update command's extra.dashboard.activeTabs validation to lock in the existing correct behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sadpandajoe
force-pushed
the
fix-alert-report-command-skips
branch
from
7d6a421 to
2a161db
Compare
![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 #27ed6d
Actionable Suggestions - 2
-
tests/unit_tests/reports/filters_test.py - 1
- Test expectation incorrect · Line 81-81
-
tests/unit_tests/reports/dao_test.py - 1
- SEMANTIC_DUPLICATION: Duplicate test coverage · Line 94-111
Additional Suggestions - 1
-
tests/unit_tests/reports/dao_test.py - 1
-
Inconsistent test pattern: mocks vs integration · Line 114-130This test uses mocks instead of the integration test pattern (real database session) established in `tests/unit_tests/daos/test_report_dao.py`. Converting to an integration test would verify actual database behavior for LIKE wildcard escaping.
-
Review Details
-
Files reviewed - 6 · Commit Range:
7d6a421..7d6a421- superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx
- superset-frontend/src/features/reports/ReportModal/index.tsx
- superset/reports/filters.py
- tests/unit_tests/commands/report/update_test.py
- tests/unit_tests/reports/dao_test.py
- tests/unit_tests/reports/filters_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ 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
| f.apply(query, "50%_off\\promo") | ||
|
|
||
| # %, _ and \ are all escaped, and the literal is wrapped for a "contains" match | ||
| expected = "%50\\%\\_off\\\\promo%" |
There was a problem hiding this comment.
The _escape_like function (filters.py:33) doubles each backslash once via value.replace("\\", "\\\\"), so a single \ in input becomes \\ in output. The test expectation has \\\\ (four backslashes) before promo but the implementation produces \\ (two backslashes). This test will fail at runtime.
Code suggestion
Check the AI-generated fix before applying
--- a/tests/unit_tests/reports/filters_test.py
+++ b/tests/unit_tests/reports/filters_test.py
@@ -81 +81 @@
- expected = "%50\\%\\_off\\\\promo%"
+ expected = "%50\\%\\_off\\promo%"
Code Review Run #27ed6d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @patch("superset.daos.report.ReportSchedule") | ||
| @patch("superset.daos.report.db") | ||
| def test_find_by_extra_metadata_escapes_like_wildcards( | ||
| mock_db: MagicMock, mock_report_schedule: MagicMock | ||
| ) -> None: | ||
| """LIKE wildcards in the slug must be escaped so they match literally.""" | ||
| from superset.daos.report import ReportScheduleDAO | ||
|
|
||
| expected: list[MagicMock] = [MagicMock()] | ||
| mock_db.session.query.return_value.filter.return_value.all.return_value = expected | ||
|
|
||
| result = ReportScheduleDAO.find_by_extra_metadata("100%_off") | ||
|
|
||
| assert result is expected | ||
| # autoescape=True is what neutralises the LIKE wildcards in the slug | ||
| mock_report_schedule.extra_json.contains.assert_called_once_with( | ||
| "100%_off", autoescape=True | ||
| ) |
There was a problem hiding this comment.
This test duplicates existing integration tests in tests/unit_tests/daos/test_report_dao.py (lines 71-92) that verify the same LIKE wildcard escaping behavior. Integration tests with real database sessions are superior because they verify actual query behavior, not just mock call arguments.
Code Review Run #27ed6d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Code Review Agent Run #d65847
Actionable Suggestions - 1
-
superset/reports/filters.py - 1
- Duplicate function across modules · Line 28-33
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/reports/dao_test.py - 2
- SEMANTIC_DUPLICATION: test coverage · Line 96-111
- SEMANTIC_DUPLICATION: test coverage · Line 116-130
Review Details
-
Files reviewed - 6 · Commit Range:
2a161db..2a161db- superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx
- superset-frontend/src/features/reports/ReportModal/index.tsx
- superset/reports/filters.py
- tests/unit_tests/commands/report/update_test.py
- tests/unit_tests/reports/dao_test.py
- tests/unit_tests/reports/filters_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ 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 _escape_like(value: str) -> str: | ||
| """ | ||
| Escape LIKE/ILIKE wildcard characters so user-supplied search text is matched | ||
| literally instead of being interpreted as wildcards (e.g. ``%`` and ``_``). | ||
| """ | ||
| return value.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") |
There was a problem hiding this comment.
The _escape_like function is duplicated identically in superset/daos/base.py:84. Maintaining two identical copies creates maintenance risk: future changes must be applied in both locations or the codebase will become inconsistent.
Code Review Run #d65847
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
SUMMARY
Two small correctness fixes in the Alerts & Reports feature, plus regression tests.
1. Escape
LIKEwildcards in the reports text filter. The text search filter onthe alerts/reports list passed user input directly into
ILIKE, so%and_wereinterpreted as wildcards rather than literal characters. A search for
a_bwouldmatch
axb, etc. The fix adds a local_escape_like()helper (mirroring the existingdaos/datasource.pyprecedent) and passesescape="\"to each filter clause so thecharacters match literally.
2. Preserve a typed
0in the ReportModal custom screenshot-width input. The inputused
|| nullfor the submitted value and|| ''for the display value, which silentlycoerced a typed
0into "no custom width". The fix uses?? ''for display and anexplicit
Number.isNaN()check on change. A typed0is now submitted and surfaced bythe server's min-width validation instead of being dropped on the client; clearing the
field still yields an empty value (parsed
NaN→ null).Regression tests. Adds backend unit tests pinning the report DAO's
autoescapebehavior and the update command's
extra.dashboard.activeTabsvalidation (both alreadycorrect in code), and frontend RTL tests for the width-input behavior above.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — behavior fixes, no visual change to the layout.
TESTING INSTRUCTIONS
Backend:
pytest tests/unit_tests/reports/filters_test.py \ tests/unit_tests/reports/dao_test.py \ tests/unit_tests/commands/report/update_test.pyFrontend:
Manual: in the Alerts & Reports list, search the name filter for a term containing
%or
_and confirm it matches literally. In the report/alert modal for a non-dashboardchart, type
0into the custom screenshot width and confirm the value is preserved(and rejected by validation) rather than silently cleared.
ADDITIONAL INFORMATION