Fixed get_or_create method #1404

ipakeev · 2023-06-14T21:32:23Z

Description

I encountered an error: with simultaneous identical requests, the server responds with 500 error code.
tortoise-orm 0.19.3

INFO:     'POST /api/token HTTP/1.1' 500 Internal Server Error
ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/tortoise/models.py", line 1059, in get_or_create
    await cls.select_for_update().filter(**kwargs).using_db(connection).get(),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/tortoise/queryset.py", line 1020, in _execute
    raise DoesNotExist("Object does not exist")
tortoise.exceptions.DoesNotExist: Object does not exist

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/tortoise/models.py", line 1064, in get_or_create
    return await cls.create(using_db=connection, **defaults, **kwargs), True
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/tortoise/backends/asyncpg/client.py", line 86, in _translate_exceptions
    raise IntegrityError(exc)
tortoise.exceptions.IntegrityError: duplicate key value violates unique constraint "user_phone_key"
DETAIL:  Key (phone)=(9998887764) already exists.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/uvicorn/protocols/http/h11_impl.py", line 407, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/fastapi/applications.py", line 270, in __call__
    await super().__call__(scope, receive, send)
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/applications.py", line 124, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/middleware/errors.py", line 184, in __call__
    raise exc
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/middleware/errors.py", line 162, in __call__
    await self.app(scope, receive, _send)
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
    raise exc
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
    await self.app(scope, receive, sender)
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__
    raise e
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__
    await self.app(scope, receive, send)
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/routing.py", line 706, in __call__
    await route.handle(scope, receive, send)
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/routing.py", line 276, in handle
    await self.app(scope, receive, send)
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/starlette/routing.py", line 66, in app
    response = await func(request)
               ^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/fastapi/routing.py", line 235, in app
    raw_response = await run_endpoint_function(
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/fastapi/routing.py", line 161, in run_endpoint_function
    return await dependant.call(**values)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/work/push-service/push_service/app/token/routes.py", line 13, in create_token
    user, _ = await User.get_or_create(phone=body.phone)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/tortoise/models.py", line 1066, in get_or_create
    return await cls.filter(**kwargs).using_db(connection).get(), False
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ipakeev/.virtualenvs/push-service/lib/python3.11/site-packages/tortoise/backends/asyncpg/client.py", line 88, in _translate_exceptions
    raise TransactionManagementError(exc)

How Has This Been Tested?

After applying this PR, everything works as expected:

        try:
            return await cls.filter(**kwargs).using_db(db).get(), False
        except DoesNotExist:
            try:
                print("not found")
                async with in_transaction(connection_name=db.connection_name) as connection:
                    return await cls.create(using_db=connection, **defaults, **kwargs), True
            except (IntegrityError, TransactionManagementError) as exc:
                print("integrity error")
                try:
                    return await cls.filter(**kwargs).using_db(db).get(), False
                except Exception:
                    raise exc

not found
not found
not found
integrity error
integrity error
INFO:     'POST /api/token HTTP/1.1' 200 OK
INFO:     'POST /api/token HTTP/1.1' 200 OK
INFO:     'POST /api/token HTTP/1.1' 200 OK
INFO:     'POST /api/token HTTP/1.1' 200 OK
INFO:     'POST /api/token HTTP/1.1' 200 OK

I did't find a way to reproduce this error in autotests.

Checklist:

My code follows the code style of this project.
~~My change requires a change to the documentation.~~
~~I have updated the documentation accordingly.~~
I have added the changelog accordingly.
I have read the CONTRIBUTING document.
~~I have added tests to cover my changes.~~
All new and existing tests passed.

long2ice · 2023-11-03T06:35:53Z

tests/test_concurrency.py

@@ -77,11 +77,3 @@ async def test_nonconcurrent_get_or_create(self):
        self.assertEqual(len(una_created), 1)
        for una in unas:
            self.assertEqual(una[0], unas[0][0])
-
-    @test.skipIf(sys.version_info < (3, 7), "aiocontextvars backport not handling this well")
-    async def test_concurrent_get_or_create(self):


Why remove this testcase?

Because it fails in the TestConcurrencyTransactioned. But the same test works in the TestConcurrencyIsolated.

abondar

Please rebase on actual develop and add changes to changelog

abondar · 2024-04-27T06:27:10Z

tortoise/models.py

+            except (IntegrityError, TransactionManagementError) as exc:
+                try:
+                    return await cls.filter(**kwargs).using_db(db).get(), False
+                except Exception:


this try-catch seems like debug artefact? Remove it please

The goal is to re-raise integrity exception instead of DoesNotExist error

abondar · 2024-04-27T06:27:37Z

tortoise/models.py

-                )
-            except DoesNotExist:
-                try:
+                async with in_transaction(connection_name=db.connection_name) as connection:


Do we need transaction here for just single create?

Probably not. But this logic is in django.

https://github.com/django/django/blob/main/django/db/models/query.py#L938

Seems like this sub-transaction is used, so that in case of integrity error here - it aborts it's transaction, so we should have one here for it to be aborted, otherwise it can abort whole parent transaction that is out of scope of this function, which would lead for unexpected behaviour for user

Let's make similar transaction here, as it was in your previous version

abondar · 2024-05-07T06:21:34Z

Please rebase and resolve conflict

coveralls · 2024-05-14T07:53:11Z

Pull Request Test Coverage Report for Build 9043068735

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

7 of 10 (70.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 87.905%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tortoise/models.py	7	10	70.0%

Totals
Change from base Build 9028420328:	-0.03%
Covered Lines:	5771
Relevant Lines:	6470

💛 - Coveralls

long2ice previously approved these changes Nov 3, 2023

View reviewed changes

long2ice self-requested a review November 3, 2023 06:36

long2ice self-assigned this Nov 3, 2023

droserasprout mentioned this pull request Nov 22, 2023

Parallel execution of multiple get_or_create calls wrapped with transaction raises TransactionManagementError with postgresql backend #792

Open

abondar reviewed Apr 27, 2024

View reviewed changes

ipakeev dismissed long2ice’s stale review via 42e90ce May 1, 2024 10:54

ipakeev force-pushed the fix_get_or_create branch 2 times, most recently from 42e90ce to 982d95b Compare May 1, 2024 10:58

ipakeev requested a review from abondar May 1, 2024 11:00

ipakeev force-pushed the fix_get_or_create branch from 0da910f to ba2ed92 Compare May 7, 2024 20:58

ipakeev added 2 commits May 11, 2024 13:34

fixed get_or_create

2d3f146

added transaction

63cfb50

ipakeev force-pushed the fix_get_or_create branch from ba2ed92 to 63cfb50 Compare May 11, 2024 10:34

Merge branch 'develop' into fix_get_or_create

628a9a3

abondar approved these changes May 14, 2024

View reviewed changes

abondar merged commit 7cb7e72 into tortoise:develop May 14, 2024
2 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed get_or_create method #1404

Fixed get_or_create method #1404

ipakeev commented Jun 14, 2023 •

edited

long2ice Nov 3, 2023

ipakeev Nov 6, 2023

abondar left a comment

abondar Apr 27, 2024

ipakeev May 1, 2024 •

edited

abondar Apr 27, 2024

ipakeev May 1, 2024

abondar May 7, 2024

ipakeev May 7, 2024

abondar commented May 7, 2024

coveralls commented May 14, 2024

Fixed get_or_create method #1404

Fixed get_or_create method #1404

Conversation

ipakeev commented Jun 14, 2023 • edited

Description

How Has This Been Tested?

Checklist:

long2ice Nov 3, 2023

Choose a reason for hiding this comment

ipakeev Nov 6, 2023

Choose a reason for hiding this comment

abondar left a comment

Choose a reason for hiding this comment

abondar Apr 27, 2024

Choose a reason for hiding this comment

ipakeev May 1, 2024 • edited

Choose a reason for hiding this comment

abondar Apr 27, 2024

Choose a reason for hiding this comment

ipakeev May 1, 2024

Choose a reason for hiding this comment

abondar May 7, 2024

Choose a reason for hiding this comment

ipakeev May 7, 2024

Choose a reason for hiding this comment

abondar commented May 7, 2024

coveralls commented May 14, 2024

Pull Request Test Coverage Report for Build 9043068735

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ipakeev commented Jun 14, 2023 •

edited

ipakeev May 1, 2024 •

edited