Conversation
# Conflicts: # rollbar/contrib/django/middleware.py # rollbar/test/test_rollbar.py
# Conflicts: # .github/workflows/ci.yml # pyproject.toml
|
@claude review |
| "uvicorn", | ||
| ] | ||
|
|
||
| #type = [ |
There was a problem hiding this comment.
Commented-out code, can we remove?
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| _levels = { | ||
| 'CRITICAL': logging.CRITICAL, | ||
| 'FATAL': logging.FATAL, | ||
| 'ERROR': logging.ERROR, | ||
| 'WARN': logging.WARNING, | ||
| 'WARNING': logging.WARNING, | ||
| 'INFO': logging.INFO, | ||
| 'DEBUG': logging.DEBUG, | ||
| 'NOTSET': logging.NOTSET, | ||
| } | ||
|
|
||
|
|
||
| def check_level(level: str | int ) -> int: | ||
| """ | ||
| Convert level to numeric logging level. | ||
|
|
||
| Modeled after logging._checkLevel to avoid dependency on private API. | ||
| """ | ||
| if isinstance(level, int): | ||
| rv = level | ||
| elif str(level) == level: | ||
| if level not in _levels: | ||
| raise ValueError(f"Unknown level: {level!r}") | ||
| rv = _levels[level] | ||
| else: | ||
| raise TypeError(f"Level not an integer or a valid string: {level!r}") | ||
| return rv |
There was a problem hiding this comment.
🔴 The new check_level() function in rollbar/logger.py uses a hardcoded dict of 8 standard level names, breaking users who register custom logging levels via logging.addLevelName(). Any call to rollbar_handler.setLevel('NOTICE') or RollbarHandler(level='NOTICE') after logging.addLevelName(25, 'NOTICE') will raise ValueError("Unknown level: 'NOTICE'") instead of returning 25 as the old logging._checkLevel did. To fix, replace the hardcoded lookup with one that consults logging._nameToLevel or uses logging.getLevelName() so that dynamically registered levels are recognized.
Extended reasoning...
What the bug is and how it manifests
This PR replaces 'from logging import _checkLevel' with a new check_level() function (rollbar/logger.py:31-57). The replacement function uses a module-level dict _levels populated at import time with exactly 8 standard level names. Any level name not in that dict causes an immediate ValueError.
The specific code path that triggers it
- User calls logging.addLevelName(25, 'NOTICE') — a public, documented Python API.
- User instantiates RollbarHandler(level='NOTICE') or calls rollbar_handler.setLevel('NOTICE').
- Both paths call check_level('NOTICE') (rollbar/logger.py:101 and 115 respectively).
- 'NOTICE' is not in _levels, so check_level raises ValueError("Unknown level: 'NOTICE'").
- The handler is never created / the level is never updated.
Why existing code does not prevent it
The old logging._checkLevel reads from logging._nameToLevel, which is the authoritative registry updated by every logging.addLevelName() call. Because the new implementation copies none of that dynamic state (it builds _levels from hardcoded constants at module load time), any level registered after import is invisible to check_level. Integer levels are unaffected because the isinstance(level, int) branch returns the value directly, so rollbar_handler.setLevel(25) still works. Only string names for custom levels are broken.
What the impact would be
Any user who (a) registers a custom log level name via logging.addLevelName() and (b) uses that name as a string when configuring RollbarHandler will see a runtime ValueError at handler creation or setLevel() time. This is a breaking change for a legitimate, common Python logging usage pattern (e.g., adding NOTICE, TRACE, VERBOSE, SUCCESS levels).
How to fix it
The simplest correct fix is to call the public logging.getLevelName(name) and verify the return type is int:
def check_level(level: str | int) -> int:
if isinstance(level, int):
return level
elif isinstance(level, str):
result = logging.getLevelName(level)
if isinstance(result, int):
return result
raise ValueError(f"Unknown level: {level\!r}")
raise TypeError(f"Level not an integer or a valid string: {level\!r}")
This avoids the private-API dependency the PR was trying to remove, while still supporting dynamically registered levels.
Step-by-step proof
import logging
logging.addLevelName(25, 'NOTICE')
# Old behaviour (logging._checkLevel):
# _checkLevel reads logging._nameToLevel which now contains 'NOTICE': 25
# => returns 25
# New behaviour (this PR, _levels is static):
# 'NOTICE' not in _levels => raises ValueError("Unknown level: 'NOTICE'")
# With the proposed fix:
# logging.getLevelName('NOTICE') => 25 (int, because addLevelName registered it)
# isinstance(25, int) => True => returns 25
|
|
||
| routes = [ | ||
| Route('/', endpoint=root), | ||
| Route('/error', endpoint=error), | ||
| ] | ||
|
|
||
| # Integrate Rollbar with Starlette application | ||
| app = Starlette() |
There was a problem hiding this comment.
🔴 Three example apps define a routes list but omit it from the Starlette() constructor, leaving every HTTP request returning 404. The affected files are rollbar/examples/asgi/app.py (line 62), rollbar/examples/starlette/app_global_request.py (line 38), and rollbar/examples/starlette/app_logger.py (line 61); each should be changed to app = Starlette(routes=routes) to match the already-correct rollbar/examples/starlette/app.py.
Extended reasoning...
What the bug is and how it manifests
This PR refactored all Starlette/ASGI example apps from decorator-based route registration (@app.route('/')) to the explicit Route()-list style. The refactoring requires two changes: (1) build the routes list, and (2) pass it to the Starlette(routes=routes) constructor. The PR correctly made both changes in rollbar/examples/starlette/app.py, but missed the second change in three other files: rollbar/examples/asgi/app.py, rollbar/examples/starlette/app_global_request.py, and rollbar/examples/starlette/app_logger.py.
The specific code paths that trigger it
rollbar/examples/asgi/app.pylines 55–62:routes = [Route('/', endpoint=root), Route('/error', endpoint=error)]is defined, butapp = Starlette()on line 62 does not receiveroutes=routes.rollbar/examples/starlette/app_global_request.pylines 33–38:routes = [Route('/', endpoint=root)]is defined, butapp = Starlette()on line 38 does not receiveroutes=routes.rollbar/examples/starlette/app_logger.pylines 56–61:routes = [Route('/', endpoint=root)]is defined, butapp = Starlette()on line 61 does not receiveroutes=routes.
Why existing code doesn't prevent it
Starlette's Starlette() constructor accepts an optional routes keyword argument. When the argument is omitted, the app is created with an empty router and no 404 handler for the defined paths. There is no error at startup — the app starts fine but silently discards the routes list entirely, so the omission is not caught until a real HTTP request is made.
What the impact would be
Any developer following the example by running python app.py and then curl http://localhost:8888 or curl http://localhost:8888/error will receive a 404 response for every request. The Rollbar middleware integration will never exercise the intended handlers, making the examples non-functional as documentation.
How to fix it
In each affected file, change the bare app = Starlette() to app = Starlette(routes=routes). This is exactly the pattern already used in the correctly-fixed rollbar/examples/starlette/app.py (line 62 in the new file).
Step-by-step proof
- The old code used
@app.route('/')which registered routes directly on the app object at decoration time. - The PR removed those decorators and introduced
routes = [Route('/', endpoint=root), ...]. - In
rollbar/examples/starlette/app.pythe constructor was updated:app = Starlette(routes=routes)✓ - In
rollbar/examples/asgi/app.py,rollbar/examples/starlette/app_global_request.py, androllbar/examples/starlette/app_logger.pythe constructor was NOT updated:app = Starlette()✗ - Running any of the three broken examples and sending
GET /returns HTTP 404 because the router has no registered routes.
2 participants
Description of the change
This PR adds the initial work of getting static type checking with mypy working. Because we support Python 3.9 to 3.14 and have integrations with a lot of frameworks that may or may not exist in the environment, our type imports can be a little messy at times. As much as possible we try not to ignore types.
Type of change
Related issues
Checklists
Development
Code review