Fix collections.ControlledDict handling of Iterators via update method
#732
Conversation
WalkthroughThe pull request introduces changes to several files, primarily focusing on enhancing Python version support in testing and project configuration. The Changes
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #732 +/- ##
=======================================
Coverage 84.19% 84.19%
=======================================
Files 27 27
Lines 1670 1670
Branches 314 313 -1
=======================================
Hits 1406 1406
Misses 202 202
Partials 62 62 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/test_collections.py (1)
65-68: LGTM! Consider adding more iterator test cases.The test for iterator handling is a good addition and aligns with the PR objective. However, consider enhancing test coverage with additional cases:
# Test empty iterator dct.update(zip([], [])) assert len(dct) == previous_len # No changes # Test exhausted iterator iterator = zip(["e"], [13]) list(iterator) # exhaust it dct.update(iterator) assert "e" not in dct # No changes # Test iterator with invalid values with pytest.raises(ValueError): dct.update(zip(["f"], [])) # uneven length.github/workflows/test.yml (1)
12-12: LGTM! Consider matrix optimization.The addition of Python 3.11 and 3.13 to the test matrix improves coverage. However, with 4 Python versions and 2 OS targets, you're now running 8 parallel jobs.
Consider if you need to test all Python versions on all OS targets. You might optimize by:
- Testing all Python versions on Ubuntu
- Testing only LTS Python versions (3.10, 3.12) on macOS
This would reduce the job count while maintaining good coverage.src/monty/collections.py (1)
Line range hint
92-104: LGTM with minor performance optimization suggestion.The changes correctly fix the iterator handling by constructing a dictionary before validation. However, the dict construction is performed twice unnecessarily.
Consider optimizing the validation loop:
- updates = dict(*args, **kwargs) - for key in dict(*args, **kwargs): + updates = dict(*args, **kwargs) + for key in updates: if key not in self.data and not self._allow_add: raise TypeError( f"Cannot add new key {key!r} using update, because add is disabled." ) elif key in self.data and not self._allow_update: raise TypeError( f"Cannot update key {key!r} using update, because update is disabled." ) super().update(updates)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)pyproject.toml(1 hunks)src/monty/collections.py(2 hunks)tests/test_collections.py(2 hunks)
🔇 Additional comments (2)
tests/test_collections.py (1)
164-167: LGTM! Clear test case for Namespace update behavior.
The test case clearly documents that update can be used to add new values to a Namespace, which is distinct from updating existing values.
src/monty/collections.py (1)
Line range hint 92-104: Verify fix against pymatgen issue.
Let's verify that this change resolves the iterator handling issue mentioned in pymatgen PR #4223.
✅ Verification successful
Iterator handling in ControlledDict.update() is properly tested and working
Looking at the test coverage in tests/test_collections.py, specifically the test_update_allowed method, there is explicit testing for iterator handling:
# Test Iterator handling
dct.update(zip(["c", "d"], [11, 12]))
assert dct["c"] == 11The implementation in the review correctly handles iterators by using dict(*args, **kwargs) to process the input before updating, and the test suite verifies this behavior. The changes appear to properly resolve any iterator-related issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related test cases and usage patterns in pymatgen
# Look for ControlledDict usage with iterators in tests
rg -A 5 "ControlledDict.*update.*zip\(" || \
rg -A 5 "ControlledDict.*update.*iter\(" || \
rg -A 5 "ControlledDict.*update.*map\("
# Look for any failing tests related to ControlledDict
rg -B 2 -A 5 "def test.*ControlledDict.*fail"
Length of output: 174
Script:
#!/bin/bash
# Let's try a broader search to understand the context and test coverage
# Search for ControlledDict tests
rg -B 2 -A 5 "class.*Test.*ControlledDict"
# Look for update method tests specifically
rg -B 2 -A 5 "test.*update" src/monty/collections.py
# Search for any test files that might contain ControlledDict tests
fd "test.*\.py" | xargs rg "ControlledDict"
# Look for the actual implementation to understand the context better
ast-grep --pattern 'class ControlledDict {
$$$
update($$$) {
$$$
}
$$$
}'
Length of output: 1272
Script:
#!/bin/bash
# Let's look at the specific test cases for update method
rg -B 5 -A 10 "def test.*update" tests/test_collections.py
# Also check the actual test file content to understand the test coverage
cat tests/test_collections.py
# Look for any related issues or PRs in the commit history
git log --grep="ControlledDict.*update" --grep="pymatgen.*4223" -i
Length of output: 7848
DanielYang59
force-pushed
the
fix-namespace-dict-with-iterator
branch
from
0aeb657 to
d54bde4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/monty/io.py (1)
Line range hint
337-463: Enhance FileLock security and robustnessThe FileLock implementation could benefit from additional security measures and robustness improvements.
Consider these security enhancements:
def acquire(self) -> None: + # Ensure atomic creation of lock file + lock_dir = os.path.dirname(self.lockfile) + if not os.path.exists(lock_dir): + os.makedirs(lock_dir, mode=0o755, exist_ok=True) + start_time = time.time() while True: try: - self.fd = os.open(self.lockfile, os.O_CREAT | os.O_EXCL | os.O_RDWR) + # Use restrictive permissions for the lock file + old_umask = os.umask(0o077) + try: + self.fd = os.open( + self.lockfile, + os.O_CREAT | os.O_EXCL | os.O_RDWR, + mode=0o600, + ) + finally: + os.umask(old_umask) breakThis change:
- Ensures the lock directory exists
- Uses restrictive permissions for the lock file
- Properly handles umask restoration
🧹 Nitpick comments (3)
src/monty/io.py (3)
Line range hint
61-90: Enhance warning messages structure and consistencyThe warning messages are well-implemented but could benefit from some improvements:
Consider applying these enhancements:
- "We strongly discourage using a default `mode`, it would be " - f"set to `r` now but would not be allowed after {_deadline}", + f"Default mode 'r' is deprecated and will be removed after {_deadline}. " + "Please explicitly specify the file mode.", FutureWarning, stacklevel=2, ) - "We strongly discourage using implicit binary/text `mode`, " - f"and this would not be allowed after {_deadline}. " - "I.e. you should pass t/b in `mode`.", + f"Implicit binary/text mode is deprecated and will be removed after {_deadline}. " + "Please explicitly include 'b' or 't' in the mode string.", FutureWarning, stacklevel=2, ) - "We strongly encourage explicit `encoding`, " - "and we would use UTF-8 by default as per PEP 686", + "No encoding specified. Using UTF-8 by default (PEP 686). " + "Please explicitly specify the encoding for text mode operations.",This makes the messages more concise and action-oriented while maintaining all the important information.
Line range hint
132-169: Enhance type safety in line ending detectionThe function handles different file types well, but we can improve type safety and error handling.
Consider these improvements:
def _get_line_ending( file: str | Path | io.TextIOWrapper | io.BufferedReader | gzip.GzipFile | bz2.BZ2File, ) -> Literal["\r\n", "\n"]: + # Type guard for binary data + def ensure_bytes(data: bytes | None) -> bytes | None: + if data is not None and not isinstance(data, bytes): + raise TypeError(f"Expected bytes, got {type(data).__name__}") + return data + if isinstance(file, (str, Path)): with zopen(file, "rb") as f: - first_line = f.readline() + first_line = ensure_bytes(f.readline()) elif isinstance(file, io.TextIOWrapper): - first_line = file.buffer.readline() + first_line = ensure_bytes(file.buffer.readline())
Line range hint
171-334: Improve error handling and documentation in reverse reading functionsThe reverse reading implementations are well-optimized but could benefit from better error handling and documentation.
- Add performance characteristics to docstring:
def reverse_readfile( filename: Union[str, Path], ) -> Iterator[str]: """ A much faster reverse read of file using memory-mapped files. Performance characteristics: - O(1) memory usage for regular files using mmap - O(n) memory usage for compressed files - Best for files > 1MB due to mmap overhead """
- Add explicit error handling for memory mapping failures:
try: filemap = mmap.mmap(file.fileno(), 0, access=mmap.ACCESS_READ) - except ValueError: + except (ValueError, OSError) as e: warnings.warn( - "trying to mmap an empty file.", + f"Failed to memory-map file: {e}. Falling back to standard reading.", stacklevel=2 ) return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/monty/io.py(1 hunks)
🔇 Additional comments (1)
src/monty/io.py (1)
Line range hint 1-463: Verify security implications of file operations
Let's verify the security implications of the file operations across the codebase.
✅ Verification successful
Let me analyze the security implications based on the results. The file operations appear to be well-implemented with proper security considerations:
-
File operations use proper error handling and secure practices:
zopenuses explicit encoding (UTF-8) and enforces explicit mode specification- File locks implement proper cleanup in
__del__and context managers - Memory-mapped operations have proper bounds checking
-
Encoding handling is consistent:
- All decode operations explicitly specify UTF-8
- Binary/text mode handling is strictly enforced with warnings
- Line ending detection is robust across platforms
-
OS operations are limited and secure:
os.openis only used with proper flags for atomic file lockingos.makedirsusesexist_okparameter to prevent race conditions- No unsafe chmod/chown operations found
Let me verify one final aspect regarding path traversal protection:
Let me fix the regex and check directory traversal one more time:
File operations are securely implemented with proper safeguards
The verification reveals that the file operations in src/monty/io.py are secure:
-
Path handling is properly sanitized:
- Uses
os.path.abspathfor file lock paths - Employs
pathlib.Pathfor safe path manipulation - No direct path traversal vulnerabilities found
- Uses
-
File operations have proper security controls:
- Enforces explicit encoding (UTF-8) and mode specification
- Implements proper cleanup in context managers
- Uses atomic operations for file locking
- Memory-mapped operations have bounds checking
-
Directory operations are protected:
- No unsafe directory traversal patterns in the codebase
- Path joining is done safely using
os.path.joinandPath - Proper error handling for file operations
The review comment's security concerns are addressed by the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in file operations
# Check for unsafe file operations
echo "Checking for potentially unsafe file operations..."
rg -n "os\.(open|makedirs|chmod|chown)" --type py
# Check for proper encoding handling
echo "Checking for potential encoding issues..."
rg -n "\.decode\(|\.encode\(" --type py
# Check for proper error handling in file operations
echo "Checking for file operation error handling..."
ast-grep --pattern 'try {
$$$
open($_, $_)
$$$
}'
Length of output: 4167
Script:
#!/bin/bash
# Check for path sanitization
echo "Checking path handling..."
rg -n "os\.path\.(abspath|normpath|realpath)" --type py
# Check for directory traversal prevention
echo "Checking directory traversal prevention..."
rg -n "(\.\.\/|\.\.\\\\)" --type py
Length of output: 1443
Script:
#!/bin/bash
# Check for directory traversal patterns with proper escaping
echo "Checking directory traversal patterns..."
rg -n '\.\./' --type py
# Check for path joining operations
echo "Checking path joining methods..."
rg -n "os\.path\.join|Path\(" --type py -A 2
Length of output: 15039
|
Haven't looked closely yet, but the macos failure might be related to: actions/setup-python#981 |
collections.ControlledDicthandling of Iterators viaupdatemethod, to fix Fix CI failure: bumpnetcdf4, replaceNamespacewith standarddictforio.abinit.pseudosmaterialsproject/pymatgen#4223montytest is pretty cheapSummary by CodeRabbit
New Features
ControlledDictto accept zipped lists for updates.Bug Fixes
updatemethod ofControlledDict.setdefaultmethod.zopenfunction to enforce explicit mode and encoding.Tests
ControlledDict.Namespacetests to reflect changes in expected behavior.