Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getting a document creates empty document which is returned in collection.stream() #45

Open
tommie-lie opened this issue Aug 1, 2021 · 0 comments

Comments

@tommie-lie
Copy link

I noticed the problem in a test that asserts that a specific document does not exist and then retrieves all other documents from a collection. The previously queried document is in the stream, but a .exists check yields False. Hence I'd have to filter all stream()ed documents by checking their exists state, which I don't have to do in the original Firestore.

I have identified the problem to be the handling of empty/non-existing documents in python-mock-firestore, specifically two problems:

  1. CollectionReference.document() creates an empty document if there is no existing document for the given name. This empty document is normally treated as doc.exists == False, but CollectionReference.stream() still returns it.
  2. mock-firestore treats empty documents as non-existing. This deviates from original Firestore which can hold empty documents (and treats them as existing)

The following to tests reproduce the problem (note that they both work with Google's Firestore but fail with mock-firestore):

def test_does_not_stream_nonexisting_documents(self):
    client = MockFirestore()
    assert client.collection("foo").document("bar").get().exists == False
    assert len(list(client.collection("foo").stream())) == 0 # actual value is 1

def test_can_create_empty_documents(self):
    client = MockFirestore()
    client.collection("foo").document("bar").set({})
    assert client.collection("foo").document("bar").get().exists == True

Since I don't need empty documents in my tests, I have awkardly worked around the first issue by monkeypatching CollectionReference.stream() but I'd like to contribute a real solution to the problem. However, I am unsure how to go forward from here and would like to discuss the issue.

I propose to change document and collection lookup so that they don't create empty dicts anymore, but None values. For documents, treat None as non-existing and treat empty documents as existing. Then, to allow lazy creation of documents, _helpers.set_by_path() would have to iterate over path elements and create them instead of just trusting that the parent path exists and is a dict. This makes _helpers.set_by_path() a little slower, though, but I think it warrants the tradeoff.

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant