Permite usar el to_dict de un EmbeddedDocumentField #95
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Pre-merge checks (1 passed, 2 warnings)❌ Failed Checks (2 warnings)
✅ Passed Checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 15 15
Lines 252 254 +2
Branches 22 23 +1
=========================================
+ Hits 252 254 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mongoengine_plus/models/helpers.py (2)
98-101: Apply the same to_dict() preference for EmbeddedDocument items inside ListField.Currently list items always go through mongo_to_dict(), which skips BaseModel masking/exclusions and may leak hidden fields. Mirror the EmbeddedDocumentField logic here.
Apply this diff:
- if isinstance(item, EmbeddedDocument): - return_data.append(mongo_to_dict(item)) + if isinstance(item, EmbeddedDocument): + if callable(getattr(item, "to_dict", None)): + return_data.append(item.to_dict()) + else: + return_data.append(mongo_to_dict(item))
120-121: Fix FloatField branch condition (typo causes it to never match).Using
rv is FloatFieldis incorrect; should comparefield_type.Apply this diff:
- elif rv is FloatField: # pragma: no cover + elif field_type is FloatField: # pragma: no cover rv = float(data)
🧹 Nitpick comments (7)
mongoengine_plus/models/helpers.py (1)
45-51: Avoid mutating the caller-provided exclude_fields list in place.Copying prevents subtle side effects if the same list is reused across calls.
Apply this diff:
- if exclude_fields is None: - exclude_fields = [] + if exclude_fields is None: + exclude_fields = [] + else: + exclude_fields = list(exclude_fields) # remove _cls from heritage embedded fields exclude_fields.append('_cls')tests/models/test_base.py (6)
16-22: Annotate class-level lists as ClassVar to satisfy static analysis and avoid shared-mutable warnings.This matches the BaseModel convention and silences RUF012.
Apply this diff:
-class Address(EmbeddedDocument, BaseModel): +class Address(EmbeddedDocument, BaseModel): # Inheritance from BaseModel, to use _excluded and _hidden - _excluded = ['reference'] - _hidden = ['secret_code'] + _excluded: ClassVar[list[str]] = ['reference'] + _hidden: ClassVar[list[str]] = ['secret_code']And ensure ClassVar is imported (see import suggestion).
1-6: Import ClassVar for the annotations above.Apply this diff:
from mongoengine import ( Document, EmbeddedDocument, EmbeddedDocumentField, StringField, ) +from typing import ClassVar
32-32: Annotate TestModel _hidden as ClassVar for consistency with Address.Apply this diff:
- _hidden = ['secret_field'] + _hidden: ClassVar[list[str]] = ['secret_field']
39-45: Silence S106 in tests or configure tooling to ignore it for test files.These are harmless literals in tests; add
# noqa: S106if needed.Apply this diff:
- secret_code='898612', + secret_code='898612', # noqa: S106 ... - secret_field='secret', + secret_field='secret', # noqa: S106
61-61: Optional: silence S101 or configure ruff to allow asserts in tests.Either keep as-is (common in tests) or append
# noqa: S101.Apply this diff:
- assert model_dict == expected + assert model_dict == expected # noqa: S101
35-62: Add coverage for ListField[EmbeddedDocument] to ensure masking/exclusions apply within lists.Complements the helpers.py change proposed for list items.
I can add a test like:
def test_embedded_list_respects_to_dict(): class Tag(EmbeddedDocument, BaseModel): _hidden: ClassVar[list[str]] = ['secret'] name = StringField() secret = StringField() class WithTags(BaseModel, Document): __test__ = False tags = ListField(EmbeddedDocumentField(Tag)) doc = WithTags(tags=[Tag(name='t1', secret='s1')]) assert doc.to_dict()['tags'] == [{'name': 't1', 'secret': '********'}]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mongoengine_plus/models/helpers.py(1 hunks)mongoengine_plus/version.py(1 hunks)tests/models/test_base.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
mongoengine_plus/models/helpers.pymongoengine_plus/version.pytests/models/test_base.py
🧬 Code graph analysis (2)
mongoengine_plus/models/helpers.py (1)
mongoengine_plus/models/base.py (1)
to_dict(13-20)
tests/models/test_base.py (1)
mongoengine_plus/models/base.py (2)
BaseModel(6-23)to_dict(13-20)
🪛 Ruff (0.12.2)
tests/models/test_base.py
18-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
19-19: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-32: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
39-39: Possible hardcoded password assigned to argument: "secret_code"
(S106)
44-44: Possible hardcoded password assigned to argument: "secret_field"
(S106)
61-61: Use of assert detected
(S101)
🔇 Additional comments (5)
mongoengine_plus/version.py (1)
1-1: Patch version bump is appropriate for this bugfix.No issues spotted.
mongoengine_plus/models/helpers.py (1)
69-72: Prefer embedded to_dict() when available — good fix.This honors BaseModel masking/exclusions on embedded docs and aligns behavior with expectations.
tests/models/test_base.py (3)
11-14: Embedded File doc for fallback path — looks good.Covers the non-BaseModel embedded case.
26-29: Primary key and embedded fields on TestModel — LGTM.This exercises both custom to_dict() (Address) and fallback (File).
49-61: Expected structure and masking assertions — LGTM.Covers id, masking, exclusion, and embedded serialization paths.
rogelioLpz
changed the title
Permite definir el metodo
to_dicten unEmbeddedDocumenty usarlo en la construcción del dict base.