Conversation
PGijsbers
added
the
dev-tools
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
setup_log_sinks, you mutate the configuration dict by doingsink_configuration.pop("sink"), which alters theload_configuration()result globally; consider copying the dict (e.g.cfg = sink_configuration.copy()) before popping to avoid surprising side effects if the config is reused elsewhere. - The
request_response_loggermiddleware logs full headers, cookies, query params, and request bodies, which can easily include sensitive data and large payloads; it would be safer to filter/whitelist fields or truncate payloads before logging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setup_log_sinks`, you mutate the configuration dict by doing `sink_configuration.pop("sink")`, which alters the `load_configuration()` result globally; consider copying the dict (e.g. `cfg = sink_configuration.copy()`) before popping to avoid surprising side effects if the config is reused elsewhere.
- The `request_response_logger` middleware logs full headers, cookies, query params, and request bodies, which can easily include sensitive data and large payloads; it would be safer to filter/whitelist fields or truncate payloads before logging.
## Individual Comments
### Comment 1
<location path="src/core/logging.py" line_range="16-18" />
<code_context>
+def setup_log_sinks() -> None:
+ """Configure loguru based on app configuration."""
+ configuration = load_configuration()
+ for nickname, sink_configuration in configuration.get("logging", {}).items():
+ logger.info("Configuring sink", nickname=nickname, **sink_configuration)
+ sink = sink_configuration.pop("sink")
+ logger.add(sink, serialize=True, **sink_configuration)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid mutating configuration dict when extracting `sink` for logger setup.
`sink_configuration.pop("sink")` mutates the dict from `load_configuration()`. If that config is cached, reused, or if `setup_log_sinks` is called multiple times, later calls may fail or behave unexpectedly once `sink` has been removed. Prefer either copying first (e.g. `cfg = dict(sink_configuration); sink = cfg.pop("sink")`) or accessing without mutation (e.g. `sink = sink_configuration["sink"]`).
</issue_to_address>
### Comment 2
<location path="src/core/logging.py" line_range="38-45" />
<code_context>
-logger.info("Loading configuration file from '%s'", _config_file)
-logger.info("Loading environment variables from '%s'", _dotenv_file)
+
+logger.info(
+ "Determined configuration sources.",
+ configuration_directory=_config_directory,
</code_context>
<issue_to_address>
**🚨 issue (security):** Logging full request bodies, headers, and cookies at INFO level is heavy and may expose sensitive data.
In addition to bodies, this also logs headers, cookies, and query params for every request, which increases cost and risk of leaking secrets (e.g., auth tokens or credentials) to logs. Please either lower this to DEBUG or implement safeguards such as redacting/safelisting sensitive fields, limiting logged body size, or skipping logging for specific paths or content types.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded Loguru to project dependencies and updated 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (97.82%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #306 +/- ##
==========================================
+ Coverage 93.04% 93.12% +0.08%
==========================================
Files 71 72 +1
Lines 2991 3026 +35
Branches 221 223 +2
==========================================
+ Hits 2783 2818 +35
+ Misses 152 151 -1
- Partials 56 57 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.gitignore (1)
2-3: Duplicate*.logpattern.Line 64 already contains
*.logunder the Django section. Consider removing one of them to avoid redundancy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 2 - 3, Remove the duplicate '*.log' entry in .gitignore to avoid redundancy: keep one '*.log' pattern (prefer the one under the Django section) and remove the other occurrence (the standalone '*.log' near the top that sits alongside 'logs/'), ensuring 'logs/' remains if you still want to ignore that directory.src/config.toml (1)
12-20: Consider using an absolute or configured log path.The relative path
develop.logwill create the log file in the current working directory, which may vary depending on how the application is launched. Consider:
- Using an absolute path, or
- Making it relative to a well-known directory (e.g., the config directory), or
- Documenting the expected behavior.
This is minor since it's a development configuration, but worth considering for consistency across deployment scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.toml` around lines 12 - 20, The logging configuration uses a relative sink "develop.log" under the [logging.develop] table which can produce logs in unpredictable working directories; update the sink value to either an absolute path or construct it from a known config directory (for example derive path from your app config dir) or add a configurable setting (e.g., logging.develop.sinkPath) and use that when initializing the logger so the sink points to a stable location instead of "develop.log"; ensure references to the sink key in logger initialization (the code that reads logging.develop.sink) are updated to use the new path value or config variable.src/core/logging.py (1)
16-18: Mutating configuration dict withpop()is safe but fragile.Since
load_configuration()returns a fresh dict on each call, thepop("sink")mutation doesn't corrupt shared state. However, callingsetup_log_sinks()multiple times would add duplicate sinks to Loguru. Consider adding a guard or documenting this as a single-call function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/logging.py` around lines 16 - 18, The loop in setup_log_sinks mutates sink_configuration with pop("sink") which is fragile and can produce duplicate Loguru sinks if setup_log_sinks() is called more than once; change the implementation to avoid mutating the original dict (e.g., read sink = sink_configuration.get("sink") or operate on a shallow copy of sink_configuration before removing the key) and make setup_log_sinks idempotent by tracking configured sink names (e.g., a module-level set) or skipping sinks already added based on nickname; reference the configuration.get("logging", {}), sink_configuration, logger and setup_log_sinks when locating where to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/logging.py`:
- Around line 38-46: The current logger.info call logs headers, cookies, and
full body (logger.info ... url=request.url, headers=request.headers,
cookies=request.cookies, body=await request.body()), which can leak sensitive
data; update the logging in the function using logger.info to: redact sensitive
headers (at least Authorization, Cookie, Set-Cookie) from request.headers, avoid
logging raw cookies (replace with a redacted marker or boolean flag), only log
bodies for safe content-types (e.g., text/* or application/json) and truncate
them to a reasonable length (or omit entirely), and move detailed request
payload logging to DEBUG level while keeping non-sensitive metadata (url,
path_params, query_params) at INFO; locate and modify the logger.info call and
any helper that reads request.body() to implement these redaction/truncation
rules.
- Line 45: The middleware currently calls await request.body(), which drains the
request stream and prevents downstream handlers from reading it; instead, read
and store the body into a local variable, then replace the request's receive
callable with a wrapper that replays the cached body (i.e., create a new receive
coroutine that returns the cached bytes and use Request(scope, receive=receive)
or set request._receive) so downstream code (in the same middleware
function/logging middleware where body=await request.body()) can call
request.body()/json()/form() normally; ensure the logged body uses the cached
value rather than consuming the original stream.
---
Nitpick comments:
In @.gitignore:
- Around line 2-3: Remove the duplicate '*.log' entry in .gitignore to avoid
redundancy: keep one '*.log' pattern (prefer the one under the Django section)
and remove the other occurrence (the standalone '*.log' near the top that sits
alongside 'logs/'), ensuring 'logs/' remains if you still want to ignore that
directory.
In `@src/config.toml`:
- Around line 12-20: The logging configuration uses a relative sink
"develop.log" under the [logging.develop] table which can produce logs in
unpredictable working directories; update the sink value to either an absolute
path or construct it from a known config directory (for example derive path from
your app config dir) or add a configurable setting (e.g.,
logging.develop.sinkPath) and use that when initializing the logger so the sink
points to a stable location instead of "develop.log"; ensure references to the
sink key in logger initialization (the code that reads logging.develop.sink) are
updated to use the new path value or config variable.
In `@src/core/logging.py`:
- Around line 16-18: The loop in setup_log_sinks mutates sink_configuration with
pop("sink") which is fragile and can produce duplicate Loguru sinks if
setup_log_sinks() is called more than once; change the implementation to avoid
mutating the original dict (e.g., read sink = sink_configuration.get("sink") or
operate on a shallow copy of sink_configuration before removing the key) and
make setup_log_sinks idempotent by tracking configured sink names (e.g., a
module-level set) or skipping sinks already added based on nickname; reference
the configuration.get("logging", {}), sink_configuration, logger and
setup_log_sinks when locating where to apply the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf2bd90d-f495-40f1-a4cf-04eb490adb18
📒 Files selected for processing (6)
.gitignorepyproject.tomlsrc/config.pysrc/config.tomlsrc/core/logging.pysrc/main.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.py`:
- Line 9: The logging pipeline is disabled because the log setup import and
calls and the request logging/context middlewares are commented out; restore the
logging behavior by uncommenting the import (loguru/logger), re-enabling the
call to setup_log_sinks(...) with the same args used originally, and
re-registering the request logging and request context middlewares so request
IDs and request/response logging are applied at runtime; make sure
setup_log_sinks and middleware registration occur early during app
initialization (before registering routes/startup) so RequestContext and
request-logging behavior is active for incoming requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d268304-b0b2-42f1-941f-143e685150ad
📒 Files selected for processing (3)
src/config.pysrc/core/logging.pysrc/main.py
✅ Files skipped from review due to trivial changes (1)
- src/core/logging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/config.py (1)
28-34: Structured logging kwargs may not appear in output without format configuration.Loguru places extra keyword arguments in the record's
extradict. To see these values in log output, the sink format must include them (e.g.,{extra}). If the default format doesn't include extras, these context values won't be visible.Consider including placeholders in the message or verifying the log sinks are configured to display extras:
logger.info( "Determined configuration sources: config_dir={configuration_directory}, " "config_file={configuration_file}, dotenv={dotenv_file}", configuration_directory=_config_directory, configuration_file=_config_file, dotenv_file=_dotenv_file, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.py` around lines 28 - 34, The structured kwargs passed to logger.info (the call that supplies configuration_directory, configuration_file, dotenv_file) won't be visible unless the sink format includes extra; update the logger call to embed these values into the message string (e.g., include placeholders for configuration_directory, configuration_file, dotenv_file) or alternatively ensure the Loguru sink format is configured to render {extra} so those keys appear in output; change the call in the logger.info invocation in src/config.py to either include the placeholders in the message or verify logger configuration emits extras.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.py`:
- Around line 82-84: load_configuration currently re-reads and re-parses the
file by calling tomllib.loads(file.read_text()) instead of delegating to the
cached helper; change load_configuration to simply delegate to the cached
_load_configuration by calling and returning _load_configuration(file or
_config_file) so it reuses the existing functools.cache behavior and removes the
duplicated parsing logic (refer to load_configuration, _load_configuration, and
_config_file).
---
Nitpick comments:
In `@src/config.py`:
- Around line 28-34: The structured kwargs passed to logger.info (the call that
supplies configuration_directory, configuration_file, dotenv_file) won't be
visible unless the sink format includes extra; update the logger call to embed
these values into the message string (e.g., include placeholders for
configuration_directory, configuration_file, dotenv_file) or alternatively
ensure the Loguru sink format is configured to render {extra} so those keys
appear in output; change the call in the logger.info invocation in src/config.py
to either include the placeholders in the message or verify logger configuration
emits extras.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12a61132-5463-4c74-96cd-3d1beba8ff31
📒 Files selected for processing (3)
src/config.pysrc/core/logging.pysrc/main.py
✅ Files skipped from review due to trivial changes (1)
- src/core/logging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.py
Comment on lines
+82
to
84
| def load_configuration(file: Path | None = None) -> TomlTable: | ||
| file = file or _config_file | ||
| return tomllib.loads(file.read_text()) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing caching - delegates to uncached file read.
load_configuration duplicates the tomllib.loads(file.read_text()) logic from _load_configuration (line 49-50) but lacks the @functools.cache decorator. Each call will re-read and re-parse the file.
Delegate to _load_configuration to benefit from caching and reduce duplication:
♻️ Proposed fix
def load_configuration(file: Path | None = None) -> TomlTable:
file = file or _config_file
- return tomllib.loads(file.read_text())
+ return _load_configuration(file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.py` around lines 82 - 84, load_configuration currently re-reads
and re-parses the file by calling tomllib.loads(file.read_text()) instead of
delegating to the cached helper; change load_configuration to simply delegate to
the cached _load_configuration by calling and returning _load_configuration(file
or _config_file) so it reuses the existing functools.cache behavior and removes
the duplicated parsing logic (refer to load_configuration, _load_configuration,
and _config_file).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #182.
Adds logging configuration and setup.
develop.logconfig.tomlThe exact interaction for logging response and request content needs to be examined (which should be logged, make use of background tasks, ...).