Repository navigation
SWE Pipeline: Automated code improvements for ipfs_dict_chain - #7
Conversation
…hods-save-load-cid-breaking-the-api into swe-pipeline/integration
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
WouterGlorieux
left a comment
There was a problem hiding this comment.
Summary
This PR fixes a significant API-breaking bug where dictionary data keys could shadow IPFSDict class methods (like save(), load(), and cid()) when accessed via dot notation. The fix inverts the attribute lookup logic in __getattribute__ — instead of checking data keys first (with a hardcoded exclusion list for dict methods), it now checks class attributes/methods first and falls back to data keys only when no matching class attribute exists. This is a more general and robust approach that protects all class methods, not just the ones in the previously hardcoded _DICT_METHODS set.
The PR includes comprehensive test coverage for the new behavior, documentation updates in both the README and CHANGELOG, and a couple of minor style cleanups from ruff auto-fixes. All 142 tests pass with 100% coverage.
Changes Overview
Core fix — ipfs_dict_chain/IPFSDict.py:
- Removed the
_DICT_METHODSfrozenset that hardcoded a list of protected dict methods. - Rewrote
__getattribute__to trysuper().__getattribute__(key)first (which finds class methods and attributes), then fall back tosuper().__getitem__(key)for data keys, and finally raiseAttributeErrorfor genuinely missing attributes.
Tests — tests/test_IPFSDict.py:
- Added 7 new tests covering: method names not shadowing methods (
save,load,cid), method calls still working when a colliding data key exists, regular data keys still accessible via dot notation, attribute assignment to method names, bracket-notation access to colliding keys, and missing attributes raisingAttributeError.
Style — ipfs_dict_chain/IPFS.py and tests/test_IPFS.py:
- Removed an unnecessary
# noqa: BLE001comment from theconnect()method'sexceptblock (ruff auto-fix). - Removed a stray blank line in test imports.
Documentation — README.md and CHANGELOG.md:
- Added a note explaining that class methods take precedence over data keys in dot notation, and that bracket notation should be used to access raw data values when a key collides with a method name.
- Added a CHANGELOG entry describing the fix.
Issues & Concerns
-
The new
test_nonexistent_attribute_raisestest intests/test_IPFSDict.pyis redundant with the existingtest_error_handlingtest, which already verifies that accessing a non-existent attribute raisesAttributeError. While having a dedicated test is not harmful, it does add slight duplication. Consider consolidating or removing the redundant assertion to keep the test suite focused. -
The ruff auto-fix removed the
# noqa: BLE001comment from theexcept Exceptionblock inconnect()(inipfs_dict_chain/IPFS.py), but the otherexcept Exceptionblocks in the same file (_add_json,_get_json,_test_connection) still carry the comment. Since ruff passes without flagging any of these, the remaining# noqa: BLE001comments on the other blocks are also unnecessary. This is a minor consistency issue — either remove the noqa comments from all blocks or keep them on all blocks for uniformity. -
The new
__getattribute__implementation relies on atry/except AttributeErrorfor every data-key access via dot notation. This is a known Python anti-pattern that can theoretically mask realAttributeErrorexceptions raised from within property getters or descriptors. In the current codebase this is not a practical concern sinceIPFSDictand its parents define no properties or descriptors, but it's worth noting as a design trade-off. The alternative of checkingtype(self).__dict__or using a precomputed set of method names would avoid the exception-based flow control, though the current approach is more maintainable since it automatically covers future methods added to the class.
Recommendation
Changes needed before merging — the fix itself is correct, well-tested, and properly documented, but the redundant test in item 1 should be cleaned up to avoid duplication in the test suite. Items 2 and 3 are minor and non-blocking but worth addressing for code quality consistency.
WouterGlorieux
left a comment
There was a problem hiding this comment.
Summary
This PR fixes a critical API issue where dictionary data keys could shadow IPFSDict class methods (save(), load(), cid()) when accessed via dot notation, breaking the class's public API. The fix replaces the hardcoded _DICT_METHODS frozenset with a dynamic MRO-based check in __getattribute__, ensuring that any real class attribute or method (from any class in the inheritance chain) takes precedence over data keys during attribute-style access. The change also simplifies the fallback path so that accessing a genuinely missing attribute raises AttributeError directly rather than falling through to super().__getattribute__.
The implementation is clean, well-tested, and properly documented. All 141 tests pass with 100% coverage, and the new tests comprehensively cover the fix's behavior: method names remain callable when shadowed by data keys, regular data keys still work via dot notation, and data remains accessible via bracket notation when key names collide with methods.
Changes Overview
ipfs_dict_chain/IPFSDict.py: Removed the hardcoded_DICT_METHODSfrozenset. Rewrote__getattribute__to check the MRO (any(key in cls.__dict__ for cls in type(self).__mro__)) to determine if a key refers to a real class attribute/method before falling back to dict data lookup. Changed the missing-key fallback to raiseAttributeError(key)directly.tests/test_IPFSDict.py: Added 5 new tests covering method-name shadowing scenarios (save/load/cid), method callability with shadowed keys, regular data key access, attribute assignment to method names, and bracket-notation access to shadowed keys. Removed one redundant test (test_nonexistent_attribute_raises) that duplicated coverage already present intest_error_handling.tests/test_IPFS.py: Removed a blank line (ruff auto-fix).README.md: Added a note explaining that class methods take precedence over data keys in dot notation, and that bracket notation should be used to access raw data when a key collides with a method name.CHANGELOG.md: Documented the fix.
Issues & Concerns
No issues identified.
The fix is correct and addresses the reported problem thoroughly. The MRO-based approach is more robust than the previous hardcoded method-name list — it automatically covers all methods from all classes in the inheritance chain (including IPFSDict's own save, load, cid, and any future methods added to the class or its parents). The behavior is consistent: data keys that collide with method names remain stored and accessible via bracket notation (d['save']), while dot notation (d.save) always returns the bound method. The AttributeError fallback for genuinely missing attributes is preserved. Subclasses like IPFSDictChain are also correctly protected since the MRO check traverses the full inheritance chain.
Recommendation
Ready to merge
SWE Pipeline — Automated Code Improvements
This pull request was automatically generated by the SWE Pipeline.
Summary
What changed
See the commit history on the
swe-pipeline/integrationbranch for the full list of changes.Review
The pipeline performed multiple code reviews and all issues found were addressed. The codebase is at 100% test coverage with a green test suite.
Automatically generated at commit
0a3abec2.