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

fix: models get_or_create keyerror #1584

Merged
merged 19 commits into from
Jun 16, 2024
Merged

fix: models get_or_create keyerror #1584

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
eac9e73
add tests
jiangying000 May 2, 2024
95f2547
add changelog
jiangying000 May 2, 2024
6e1e1f9
add changelog
jiangying000 May 2, 2024
44f7b43
fix
jiangying000 May 5, 2024
d36cfc5
Merge branch 'develop' into develop
jiangying000 May 7, 2024
d6d6dfc
merge
jiangying000 May 20, 2024
57958df
throw
jiangying000 May 20, 2024
33db833
fix test
jiangying000 May 20, 2024
b2ec794
Remove unnecessary import and update method doc
waketzheng May 23, 2024
ea7b404
More test cases for update_or_create method
waketzheng May 23, 2024
665a36c
refactor: only query once before create for update_or_create method
waketzheng May 23, 2024
8ac9ddf
merge remote
waketzheng May 23, 2024
173411e
Merge branch 'develop' of github.com:tortoise/tortoise-orm into fix-g…
waketzheng Jun 11, 2024
038b475
Merge pull request #1 from waketzheng/fix-get-or-create-keyerror
jiangying000 Jun 11, 2024
260bd9d
Merge branch 'develop' of github.com:tortoise/tortoise-orm into develop
waketzheng Jun 12, 2024
9562b43
Update changelog and fix mssql ci error
waketzheng Jun 13, 2024
feb9b9a
Merge pull request #2 from waketzheng/fix-mssql
jiangying000 Jun 13, 2024
a8dab73
fix codacy issues
waketzheng Jun 13, 2024
8725fa4
Merge pull request #3 from waketzheng/fix-codacy-issues
jiangying000 Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Fixed
^^^^^
- Fix `DatetimeField` use '__year' report `'int' object has no attribute 'utcoffset'`. (#1575)
- Fix `bulk_update` when using custom fields. (#1564)
- Fix `update_or_create` errors when field value changed. (#1584)
- Fix `optional` parameter in `pydantic_model_creator` does not work for pydantic v2. (#1551)
- Fix `get_annotations` now evaluates annotations in the default scope instead of the app namespace. (#1552)
- Fix `get_or_create` method. (#1404)
Expand Down
5 changes: 5 additions & 0 deletions tests/test_model_methods.py
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add case about kwargs and defaults have same key and same value

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import os
from uuid import uuid4

import tortoise
from tests.testmodels import (
Dest_null,
Event,
Expand Down Expand Up @@ -174,6 +175,10 @@ async def test_update_or_create(self):
self.assertNotEqual(self.mdl, mdl)
mdl2 = await self.cls.get(name="Test2")
self.assertEqual(mdl, mdl2)
mdl_dict = dict(mdl)
mdl.id = 135
with self.assertRaises(tortoise.exceptions.ParamsError):
await self.cls.update_or_create(id=mdl.id, defaults=mdl_dict)

async def test_first(self):
mdl = await self.cls.first()
Expand Down
9 changes: 8 additions & 1 deletion tortoise/models.py
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. kwargs is 'Query parameters', defaults is 'Default values used to update the object', when they have same key with different values, value of kwargs should be covered by defaults. That is to say code must be: merged_defaults = {**kwargs, **defaults}
  2. merged_defaults = ... should be in front of try: (just put it after line 1082)
  3. return ... shoud be stay as one line, do not make it into three lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though that is how it works in django - for me it seems strange that default value overrides explicit value

I would expect opposite behaviour of this

I dunno if we should proceed with this change, as leaving merging on user side seems like more transparent behaviour

Copy link
Contributor Author

@jiangying000 jiangying000 May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would also expect explicit kv overrides the default

do merge rather than throw err seems better for me

when i write:

    mdl.id = 135
    await self.cls.update_or_create(id=mdl.id, defaults=dict(mdl))

i was expecting row with id 135 being updated or created

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me - in your case you should do explicit get_or_create without defaults and them make usual update() for returned object
That shouldn't affect performance, as it is basically what we already do

Django also, from 5.0, introduced separate create_defaults param for update_to_create for similar cases to make them less confusing
May be we should look into doing it too, but it seems there could be some issues in tortoise to use select_for_update with get_or_create

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though that is how it works in django - for me it seems strange that default value overrides explicit value

I would expect opposite behaviour of this

I dunno if we should proceed with this change, as leaving merging on user side seems like more transparent behaviour

Our team use tortoise-orm for a long time, and we are proficient in django. So we prefer it have the same result with django.

Or maybe an exception can be raised when kwargs and defaults have same key with different values:

for key in (defaults.keys() & kwargs.keys()):
    if (default_value := defaults[key]) != (query_value := kwargs[key]):
        raise tortoise.exceptions.ParamError(f'Conflict value with {key=}: {default_value=}  vs {query_value=}')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe an exception can be raised when kwargs and defaults have same key with different values:

I would prefer that, as django behaviour seems confusing to me here, but doing solution opposite to them - would be also confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe an exception can be raised when kwargs and defaults have same key with different values:

I would prefer that, as django behaviour seems confusing to me here, but doing solution opposite to them - would be also confusing

ok, updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff from github is wrong

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from pypika.terms import Term
from typing_extensions import Self

import tortoise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line

from tortoise import connections
from tortoise.backends.base.client import BaseDBAsyncClient
from tortoise.exceptions import (
Expand Down Expand Up @@ -1067,6 +1068,7 @@ async def get_or_create(
:param using_db: Specific DB connection to use instead of default bound
:param kwargs: Query parameters.
:raises IntegrityError: If create failed
:raises TransactionManagementError: If transaction error
"""
if not defaults:
defaults = {}
Expand All @@ -1075,8 +1077,13 @@ async def get_or_create(
return await cls.filter(**kwargs).using_db(db).get(), False
except DoesNotExist:
try:
for key in (defaults.keys() & kwargs.keys()):
if (default_value := defaults[key]) != (query_value := kwargs[key]):
raise tortoise.exceptions.ParamsError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change it to:

raise ParamsError("...")

Then run make style to reformat this file

f'Conflict value with {key=}: {default_value=} vs {query_value=}')
async with in_transaction(connection_name=db.connection_name) as connection:
return await cls.create(using_db=connection, **defaults, **kwargs), True
merged_defaults = {**kwargs, **defaults}
return await cls.create(using_db=connection, **merged_defaults), True
except IntegrityError as exc:
try:
return await cls.filter(**kwargs).using_db(db).get(), False
Expand Down