Fix Document model: bad test, attribute bug, and field cleanup #1113
Conversation
WalkthroughThe Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Hey @nnethercott thanks for your PR!
I'm not sure if I got the idea behind this part:
This PR removes the __doc field so that it now looks like:
So it means this introduces a breaking-change?
![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
🧹 Nitpick comments (2)
pyproject.toml (1)
117-117: Consider improving exception handling instead of disabling the warning.While disabling the "raise-missing-from" warning is acceptable, consider whether the exception handling in the
Documentclass could be improved to follow Python best practices for exception chaining.tests/models/test_document.py (1)
28-28: LGTM: Verifies correct iteration behavior.The updated assertion correctly checks that iterating over a
Documentyields key-value pairs from the internal dictionary.Minor nitpick: There's a typo in the test data -
"fiels2"should probably be"field2". If this is intentional for testing purposes, consider adding a comment to clarify.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
meilisearch/models/document.py(1 hunks)pyproject.toml(1 hunks)tests/models/test_document.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/models/test_document.py (2)
meilisearch/models/document.py (1)
Document(4-15)tests/models/test_index.py (1)
test_getattr(8-10)
🪛 Ruff (0.11.9)
meilisearch/models/document.py
12-12: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
meilisearch/models/document.py (1)
6-6: LGTM: Eliminates duplicate data storage.This change correctly addresses the duplicate data storage issue by updating the instance's
__dict__directly instead of maintaining a separate__docattribute.tests/models/test_document.py (2)
9-12: LGTM: Good test coverage for the refactored initialization.This new test properly verifies that the
Documentclass correctly initializes from a dictionary and can be converted back to a dictionary, ensuring the refactoring works as expected.
17-17: LGTM: Fixes the erroneous assertion.This change correctly fixes the test to assert the actual attribute value (
"test 1") instead of the attribute name ("field1"), aligning with the corrected__getattr__implementation.
meilisearch/models/document.py
Outdated
| def __getattr__(self, attr: str) -> str: | ||
| if attr in self.__doc.keys(): | ||
| return attr | ||
| raise AttributeError(f"{self.__class__.__name__} object has no attribute {attr}") | ||
| try: | ||
| return self.__dict__[attr] | ||
| except Exception as _: | ||
| raise AttributeError(f"{self.__class__.__name__} object has no attribute {attr}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type annotation and exception handling.
The __getattr__ method fixes the core issue but has room for improvement:
- Return type should be
Anysince attributes can be of any type - Exception handling should be more specific and follow best practices
- def __getattr__(self, attr: str) -> str:
+ def __getattr__(self, attr: str) -> Any:
try:
return self.__dict__[attr]
- except Exception as _:
- raise AttributeError(f"{self.__class__.__name__} object has no attribute {attr}")
+ except KeyError:
+ raise AttributeError(f"{self.__class__.__name__} object has no attribute {attr}") from NoneThis addresses the static analysis hint about exception chaining and makes the exception handling more precise.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __getattr__(self, attr: str) -> str: | |
| if attr in self.__doc.keys(): | |
| return attr | |
| raise AttributeError(f"{self.__class__.__name__} object has no attribute {attr}") | |
| try: | |
| return self.__dict__[attr] | |
| except Exception as _: | |
| raise AttributeError(f"{self.__class__.__name__} object has no attribute {attr}") | |
| def __getattr__(self, attr: str) -> Any: | |
| try: | |
| return self.__dict__[attr] | |
| except KeyError: | |
| raise AttributeError(f"{self.__class__.__name__} object has no attribute {attr}") from None |
🧰 Tools
🪛 Ruff (0.11.9)
12-12: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In meilisearch/models/document.py around lines 8 to 12, update the __getattr__
method to have a return type of Any instead of str to reflect that attributes
can be any type. Also, replace the broad except Exception clause with a more
specific KeyError exception to catch only missing dictionary keys. When raising
the AttributeError, use "from None" to suppress exception chaining and follow
best practices for clearer error handling.
Kind of a grey area tbh.
Since this PR changes the dict representation I guess it's safer to label it as a breaking-change ! |
|
Thanks a lot for your answer @nnethercott 🙏 Can you create a small migration guide, so I can inform other users within the release changelog? You can update the PR description and I copy from there. |
|
@brunoocasali sure thing ! 🛠 migration guidewhat changed
impact
what you need to do:
example updated usagedoc = index.get_document("foo")
doc_dict = dict(doc)
doc_dict.pop("_Document__doc") # ❌ Not needed anymore |
Pull Request
Small bug fix & improvement.
The
Documentmodel stores twice as many fields needed throughDocument.__docand doesn't properly implement__getattr__.An example of an assert that fails :
Duplicated data
Currently
dict(Document)gives this:{ "_Document__doc": { "id": 5, "title": "Hard Times", "genres": [ "Classics", "Fiction", "Victorian", "Literature" ], "publisher": "Penguin Classics", "language": "English", "author": "Charles Dickens", "description": "Hard Times is a novel of social protest which attacks utilitarianism, Bentham's theory which only considered the practical aspects of happiness and ignored emotional, spiritual and moral values. It is set in Coketown, a fictious industrial city in northern England in the mid-1800s.", "format": "Hardcover", "rating": 3 }, "id": 5, "title": "Hard Times", "genres": [ "Classics", "Fiction", "Victorian", "Literature" ], "publisher": "Penguin Classics", "language": "English", "author": "Charles Dickens", "description": "Hard Times is a novel of social protest which attacks utilitarianism, Bentham's theory which only considered the practical aspects of happiness and ignored emotional, spiritual and moral values. It is set in Coketown, a fictious industrial city in northern England in the mid-1800s.", "format": "Hardcover", "rating": 3 }This PR removes the __doc field so that it now looks like:
{ "id": 5, "title": "Hard Times", "genres": [ "Classics", "Fiction", "Victorian", "Literature" ], "publisher": "Penguin Classics", "language": "English", "author": "Charles Dickens", "description": "Hard Times is a novel of social protest which attacks utilitarianism, Bentham's theory which only considered the practical aspects of happiness and ignored emotional, spiritual and moral values. It is set in Coketown, a fictious industrial city in northern England in the mid-1800s.", "format": "Hardcover", "rating": 3 }What does this PR do?
__getattr__implementation forDocumentPR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
Bug Fixes
Tests