bito-code-review[bot] commented on PR #40481:
URL: https://github.com/apache/superset/pull/40481#issuecomment-4561091894
<!-- Bito Reply -->
The suggestion is valid and addresses a real issue in the code. The current
implementation uses `os.path.exists()` to check for the existence of a file
before attempting to open it. However, this approach can silently swallow
filesystem access problems, such as permission issues on parent directories,
leading to incorrect behavior where the function returns `None` as if the file
were missing, instead of logging the underlying error.
The suggested fix is to move the missing-file check into the `open()`
exception flow, catching only `FileNotFoundError` and allowing other I/O errors
to be reported. This ensures that permission issues and other unexpected I/O
failures are logged as intended.
To implement this fix, you can modify the code to remove the
`os.path.exists()` check and handle the missing-file case within the
`try/except` block. Here's a concise implementation of the fix:
**superset/views/base.py**
```
def get_default_spinner_svg():
svg_path = ... # logic to construct the path
try:
with open(svg_path, 'r') as f:
return f.read()
except FileNotFoundError:
# Handle missing file case
return None
except (OSError, UnicodeDecodeError) as e:
logger.warning("Could not load default spinner SVG: %s", e)
return None
```
--
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]
