Fix mutable default argument in merge_logs_to_list#1497
Fix mutable default argument in merge_logs_to_list#1497juandiego-bmu wants to merge 4 commits intoOWASP:masterfrom
Conversation
Replace log_list=[] with log_list=None and initialize inside the function body. The mutable default was shared across all calls, causing log entries from previous invocations to leak into subsequent results and growing without bound. Fixes OWASP#1464
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughRefactored Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Python mutable-default-argument bug in merge_logs_to_list() that caused log entries to leak across invocations and grow unbounded over time (issue #1464).
Changes:
- Replace
log_list=[]default withlog_list=None. - Initialize
log_listto a new list inside the function whenNoneis provided.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def merge_logs_to_list(result, log_list=None): | ||
| if log_list is None: | ||
| log_list = [] |
There was a problem hiding this comment.
Add a unit test for merge_logs_to_list covering the original bug: calling it multiple times without providing log_list should return independent results (no cross-call contamination). This helps prevent regressions of the mutable-default-argument issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nettacker/core/utils/common.py (1)
34-34: Consider adding type hints to clarify the recursive accumulator contract.This utility is easier to reason about with an explicit signature.
♻️ Optional refactor
+from typing import Any + -def merge_logs_to_list(result, log_list=None): +def merge_logs_to_list(result: Any, log_list: list[str] | None = None) -> list[str]: if log_list is None: log_list = []As per coding guidelines, "Keep functions small, use type hints where practical, and add docstrings for public APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/utils/common.py` at line 34, Add explicit type hints and a short docstring to clarify the recursive accumulator contract for merge_logs_to_list: change the signature to use typing (e.g., def merge_logs_to_list(result: Any, log_list: Optional[list] = None) -> list) or a more specific element type if known, import Optional/Any/List as needed, document that log_list is an accumulator initialized to None and the function returns a list of merged log entries, and ensure recursive calls and return statements preserve the annotated types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nettacker/core/utils/common.py`:
- Line 34: Add explicit type hints and a short docstring to clarify the
recursive accumulator contract for merge_logs_to_list: change the signature to
use typing (e.g., def merge_logs_to_list(result: Any, log_list: Optional[list] =
None) -> list) or a more specific element type if known, import
Optional/Any/List as needed, document that log_list is an accumulator
initialized to None and the function returns a list of merged log entries, and
ensure recursive calls and return statements preserve the annotated types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a765295d-4ba2-4b66-ae42-6849b066632d
📒 Files selected for processing (1)
nettacker/core/utils/common.py
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
This is a good catch. Could you address ai bot comments and add tests related to this change?
Thank you!
Address review feedback: add docstring to meet coverage threshold and add tests verifying correct behavior including no shared state between consecutive calls (the core bug this PR fixes).
|
Done! I pushed a new commit that:
All existing + new tests pass locally. |
Proposed change
Replace
log_list=[]withlog_list=Noneand initialize inside the function body inmerge_logs_to_list()(nettacker/core/utils/common.py).The mutable default list was shared across all calls. Since it's mutated via
.append()on line 41, log entries from previous invocations leak into subsequent results and the list grows without bound.Linked issue: #1464
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folderRecreated from #1470 with signed commits and proper template as requested by @securestep9.