Skip to content

Conversation

@MaitreyaBuddha
Copy link

@MaitreyaBuddha MaitreyaBuddha commented Sep 3, 2025

PR Type

Bug fix, Tests


Description

  • Return None for non-existent documents

  • Adjust tests to expect None

  • Align mock behavior with Firestore


Diagram Walkthrough

flowchart LR
  Doc["DocumentSnapshot.to_dict()"] -- "non-existent" --> NoneNode["Return None"]
  Doc -- "exists" --> DictNode["Return stored _doc"]
  Tests["test_document_reference.py"] -- "update expectation" --> NoneNode
Loading

File Walkthrough

Relevant files
Bug fix
document.py
to_dict returns None for missing documents                             

mockfirestore/document.py

  • Modify to_dict to return None when document doesn't exist
  • Add existence check before returning _doc
+2/-0     
Tests
test_document_reference.py
Update test to reflect None return                                             

tests/test_document_reference.py

  • Update test to expect None for missing document
  • Keep other tests unchanged
+1/-1     

@github-actions
Copy link

github-actions bot commented Sep 3, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The check in to_dict uses if not self.exists: which references the method object, not its return value. This will always evaluate to False and never return None. It should likely be if not self.exists():.

def to_dict(self) -> Document:
    if not self.exists:
        return None
    return self._doc

@github-actions
Copy link

github-actions bot commented Sep 3, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix method call in condition

Call the exists method instead of referencing it to ensure the check is executed. As
written, the condition always evaluates to False, never returning None for
non-existent documents.

mockfirestore/document.py [25-28]

 def to_dict(self) -> Document:
-    if not self.exists:
+    if not self.exists():
         return None
     return self._doc
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that exists is a method and must be called (exists()); otherwise the condition is always truthy, breaking the new behavior to return None for non-existent docs. This is a correctness bug fix with high impact.

High

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants