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.

Sign up for GitHub

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

Jump to bottom

AsyncTransaction APIs do not conform to documented return types #750

Open
TimPansino opened this issue Aug 8, 2023 · 3 comments · May be fixed by #751
Open

AsyncTransaction APIs do not conform to documented return types #750

TimPansino opened this issue Aug 8, 2023 · 3 comments · May be fixed by #751
Assignees
@daniel-sanche
Labels
api: firestore Issues related to the googleapis/python-firestore API. priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Milestone
Q3

Comments

@TimPansino
Copy link

TimPansino commented Aug 8, 2023 •
edited
Loading

Environment details

  • OS type and version: MacOS (arm64, amd64)
  • Python version: 3.11.4
  • pip version: 23.2
  • google-cloud-firestore version: 2.11.1 (main)

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/[email protected]/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/[email protected]/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/[email protected]/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!
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Aug 8, 2023
@TimPansino TimPansino linked a pull request Aug 8, 2023 that will close this issue
Draft
4 tasks
@TimPansino
Copy link
Author

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
Copy link
Contributor

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

@triplequark triplequark added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Nov 15, 2023
@daniel-sanche daniel-sanche added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jan 30, 2024
@daniel-sanche daniel-sanche added this to the Q2 milestone Feb 23, 2024
@daniel-sanche
Copy link
Contributor

This will be addressed along with #773

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

@daniel-sanche daniel-sanche

Labels
api: firestore Issues related to the googleapis/python-firestore API. priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
Q3
Development

Successfully merging a pull request may close this issue.

fix: Correct AsyncTransaction Return Types TimPansino/python-firestore
4 participants
@daniel-sanche @meredithslota @TimPansino @triplequark