fix: avoid warning spam when default spinner SVG is missing#40481
fix: avoid warning spam when default spinner SVG is missing#40481durgaprasadml wants to merge 3 commits into
Conversation
Code Review Agent Run #bf6d83Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if not os.path.exists(svg_path): | ||
| return None |
There was a problem hiding this comment.
Suggestion: The existence pre-check can silently swallow filesystem access problems and skip the warning path you intended to keep for unexpected read failures. os.path.exists() returns False when stat() fails (for example due to permission-denied on parent directories), so the function returns None as if the file were missing instead of logging the underlying error. Handle missing-file behavior in the open() exception flow (suppress only file-not-found) so non-missing I/O failures are still reported. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ SPA endpoints hide spinner filesystem permission misconfigurations.
- ⚠️ Harder to diagnose asset permission issues from logs.Steps of Reproduction ✅
1. In a running Superset deployment, ensure the default spinner is using the backend
helper by not setting `brandSpinnerSvg` or `brandSpinnerUrl` in the theme tokens used by
`get_spa_template_context` in `superset/views/base.py:580-641`, so the code at
`superset/views/base.py:63-70` (file lines 620-627 from the Read output) follows the `elif
not theme_tokens.get("brandSpinnerUrl"):` branch and calls `get_default_spinner_svg()`.
2. On the filesystem, create the SVG file at the path constructed in
`get_default_spinner_svg()` (`superset/views/base.py:37-49` in the snippet, corresponding
to diff lines 416-428) but remove execute/read permission from one of its parent
directories (for example `superset-frontend/`), so that `os.stat(svg_path)` fails with a
`PermissionError` even though the file exists; per CPython semantics,
`os.path.exists(svg_path)` in `superset/views/base.py:431` will return `False` when this
`stat()` call fails.
3. Trigger any SPA view that uses `render_app_template()`, for example send an
authenticated `GET /extensions/list/` request, which is handled by `ExtensionsView.list()`
in `superset/extensions/view.py:7-11`, calling `super().render_app_template()`, which in
turn calls `get_spa_template_context()` in `superset/views/base.py:240-243` and ultimately
`get_default_spinner_svg()` at `superset/views/base.py:409-60`.
4. Observe that `get_default_spinner_svg()` returns `None` early via the `if not
os.path.exists(svg_path): return None` branch at `superset/views/base.py:431-432`, so the
`try/except (OSError, UnicodeDecodeError)` block at `superset/views/base.py:435-60` is
never entered and no `logger.warning("Could not load default spinner SVG: %s", e)` message
is emitted, even though the underlying filesystem error is a permission problem rather
than a genuinely missing file; if the pre-check were removed and missing-file handling
moved into the `open()` exception flow (catching `FileNotFoundError` separately), the same
misconfiguration would result in a warning log as originally intended for unexpected I/O
failures.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 431:432
**Comment:**
*Incorrect Condition Logic: The existence pre-check can silently swallow filesystem access problems and skip the warning path you intended to keep for unexpected read failures. `os.path.exists()` returns `False` when `stat()` fails (for example due to permission-denied on parent directories), so the function returns `None` as if the file were missing instead of logging the underlying error. Handle missing-file behavior in the `open()` exception flow (suppress only file-not-found) so non-missing I/O failures are still reported.
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 is valid and addresses a real issue in the code. The current implementation uses The suggested fix is to move the missing-file check into the To implement this fix, you can modify the code to remove the superset/views/base.py |
|
Hi @durgaprasadml ! Thanks for the fix. Please see the comments left by both bots. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40481 +/- ##
==========================================
- Coverage 64.17% 55.86% -8.32%
==========================================
Files 2592 2593 +1
Lines 139299 139677 +378
Branches 32347 32439 +92
==========================================
- Hits 89395 78025 -11370
- Misses 48367 60985 +12618
+ Partials 1537 667 -870
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the review and for pointing out the issue with the os.path.exists() pre-check. I updated the implementation to follow the suggested EAFP-style handling by:
All checks are now passing successfully. |
Code Review Agent Run #d27019Actionable 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 |
|
@semohr review |
|
I have no say here and am not a maintainer. Just came here because I had the same issue. From my perspective this seems good 🤷♀️ |
|
@sfirke review |
|
@semohr I appreciate your input! You pushed this to be a better fix, doesn't matter if you're a committer. Thank you! I will review on Monday when I'm back at work. |
3 participants
Summary
Avoid warning log pollution when the default spinner SVG asset is unavailable in packaged/runtime environments.
In production deployments, frontend source assets under superset-frontend/ are not packaged or shipped. The backend fallback helper get_default_spinner_svg() attempted to load the frontend source SVG directly, causing repeated warning logs when the file was missing:
text id="fwbmtx" Could not load default spinner SVG: [Errno 2] No such file or directory ...
Since the spinner asset is optional and frontend production builds already package spinner assets separately, the warning was unnecessary and polluted logs in containerized/runtime deployments.
This change:
Files Changed
Tests Added
Added regression coverage for:
Validation
Executed:
bash id="9szt7w" PYTHONPATH=. pytest tests/unit_tests/views/test_base_theme_helpers.py
Result:
text id="mzgw0c" 34 passed
Also executed:
bash id="e98wjv" pre-commit run --files superset/views/base.py tests/unit_tests/views/test_base_theme_helpers.py
All checks passed.
Edge Cases Considered
Fixes #40478