googleapis / python-firestore

Apache License 2.0
219 stars 75 forks source link

AsyncTransaction APIs do not conform to documented return types #750

Open TimPansino opened 1 year ago

TimPansino commented 1 year ago

Environment details

Steps to reproduce

  1. Use any of the APIs on AsyncTransaction that claim to return an AsyncGenerator (get_all(), get(doc_ref), get(query)).
  2. Attempt to iterate on returned type, exception is thrown as return type is coroutine not async_generator.
  3. Attempt to await coroutine instead hoping to receive an async_generator now, exception is thrown as library code unexpectedly attempts to await an async_generator.

Code example

import asyncio
import os
import uuid

from google.cloud.firestore import AsyncClient, async_transactional, FieldFilter

@async_transactional
async def exercise_async_transaction(transaction, collection):
    # get a DocumentReference
    doc_ref = collection.document("doc1")
    [_ async for _ in transaction.get(doc_ref)]  # Crashes
    await transaction.get(doc_ref)  # Alternatively try awaiting the coroutine, still crashes

    # get a Query
    async_query = collection.select("x").where(filter=FieldFilter(field_path="x", op_string=">", value=2))
    [_ async for _ in transaction.get(async_query)]  # Crashes

    # get_all on a list of DocumentReferences
    doc_refs = [collection.document("doc1")]
    [_ async for _ in transaction.get_all(doc_refs)]  # Crashes

async def main():
    os.environ["FIRESTORE_EMULATOR_HOST"] = "localhost:8080"
    client = AsyncClient()

    collection = client.collection(str(uuid.uuid4()))
    try:
        await collection.add({"x": 5}, "doc")
        await exercise_async_transaction(client.transaction(), collection)
    finally:
        await client.recursive_delete(collection)

if __name__ == "__main__":
    asyncio.run(main())

Stack trace

./async_transaction.py:12: RuntimeWarning: coroutine 'AsyncTransaction.get' was never awaited
  [_ async for _ in transaction.get(doc_ref)]  # Crashes
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
Traceback (most recent call last):
  File "/Users/tpansino/NewRelic/Testing/Firestore/./async_transaction.py", line 36, in <module>
    asyncio.run(main())
  File "/opt/homebrew/Cellar/python@3.11/3.11.4/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.11/3.11.4/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.11/3.11.4/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/Users/tpansino/NewRelic/Testing/Firestore/./async_transaction.py", line 31, in main
    await exercise_async_transaction(client.transaction(), collection)
  File "/Users/tpansino/NewRelic/Packages/firestore/google/cloud/firestore_v1/async_transaction.py", line 311, in __call__
    result = await self._pre_commit(transaction, *args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tpansino/NewRelic/Packages/firestore/google/cloud/firestore_v1/async_transaction.py", line 254, in _pre_commit
    return await self.to_wrap(transaction, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tpansino/NewRelic/Testing/Firestore/./async_transaction.py", line 12, in exercise_async_transaction
    [_ async for _ in transaction.get(doc_ref)]  # Crashes
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'async for' requires an object with __aiter__ method, got coroutine

Making sure to follow these steps will guarantee the quickest resolution possible.

Thanks!

TimPansino commented 1 year ago

Proposed solution in #751.

It seems the use of mocking interfered with testing these APIs as the return types for the underlying AsyncClient were mocked incorrectly. Either as a result of drift or just incorrect assumptions.

meredithslota commented 1 year ago

Typing is an issue, linking this to the parent issue #447

daniel-sanche commented 3 months ago

This will be addressed along with https://github.com/googleapis/python-firestore/issues/773