Fix mutable default argument in merge_logs_to_list#1470
Fix mutable default argument in merge_logs_to_list#1470juandiego-bmu wants to merge 1 commit 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
Summary by CodeRabbit
WalkthroughFixes a mutable default argument bug in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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, preventing log entries from leaking across calls and avoiding unbounded growth of a shared default list.
Changes:
- Change
merge_logs_to_list(result, log_list=[])to defaultlog_list=None. - Initialize
log_list = []inside the function whenNoneis provided.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nettacker/core/utils/common.py (1)
34-36: Consider adding type hints (and a short docstring) for this public utility.This function is in a core shared utility path; typing the parameters/return improves readability and safer callsites.
♻️ Suggested refactor
-def merge_logs_to_list(result, log_list=None): +def merge_logs_to_list(result: object, log_list: list[str] | None = None) -> list[str]: + """Collect and deduplicate nested `log` values from a result structure.""" 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` around lines 34 - 36, Update merge_logs_to_list to include type hints and a short docstring: annotate parameters as result: Dict[str, Any] (or appropriate specific type) and log_list: Optional[List[Dict[str, Any]]] = None, and the return type as List[Dict[str, Any]]; add a one- or two-line docstring describing the function purpose, parameters, and return value; ensure you import Optional, List, Dict, Any from typing at the top and keep the existing None-default pattern to avoid mutable defaults.
🤖 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`:
- Around line 34-36: Update merge_logs_to_list to include type hints and a short
docstring: annotate parameters as result: Dict[str, Any] (or appropriate
specific type) and log_list: Optional[List[Dict[str, Any]]] = None, and the
return type as List[Dict[str, Any]]; add a one- or two-line docstring describing
the function purpose, parameters, and return value; ensure you import Optional,
List, Dict, Any from typing at the top and keep the existing None-default
pattern to avoid mutable defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2df119d-d3f1-4316-aa1c-c657e9978a33
📒 Files selected for processing (1)
nettacker/core/utils/common.py
|
Closing to recreate with proper PR template and signed commits as requested. |
Summary
Replace
log_list=[]withlog_list=Noneand initialize inside the function body.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.Split from #1466 as requested by @pUrGe12.
Fixes #1464