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

fix: models get_or_create keyerror #1584

Merged
merged 19 commits into from
Jun 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ Changelog
0.21
====

0.21.4
------
Fixed
^^^^^
- Fix `update_or_create` errors when field value changed. (#1584)

0.21.3
------
Fixed
Expand Down
34 changes: 32 additions & 2 deletions tests/test_model_methods.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ async def test_implicit_clone_pk_required_none(self):
class TestModelMethods(test.TestCase):
async def asyncSetUp(self):
await super().asyncSetUp()
self.mdl = await Tournament.create(name="Test")
self.mdl2 = Tournament(name="Test")
self.cls = Tournament
self.mdl = await self.cls.create(name="Test")
self.mdl2 = self.cls(name="Test")

async def test_save(self):
oldid = self.mdl.id
Expand Down Expand Up @@ -176,6 +176,36 @@ async def test_update_or_create(self):
mdl2 = await self.cls.get(name="Test2")
self.assertEqual(mdl, mdl2)

async def test_update_or_create_with_defaults(self):
mdl = await self.cls.get(name=self.mdl.name)
mdl_dict = dict(mdl)
oldid = mdl.id
mdl.id = 135
with self.assertRaisesRegex(ParamsError, "Conflict value with key='id':"):
# Missing query: check conflict with kwargs and defaults before create
await self.cls.update_or_create(id=mdl.id, defaults=mdl_dict)
desc = str(uuid4())
# If there is no conflict with defaults and kwargs, it will be success to update or create
defaults = dict(mdl_dict, desc=desc)
kwargs = {"id": defaults["id"], "name": defaults["name"]}
mdl, created = await self.cls.update_or_create(defaults, **kwargs)
self.assertFalse(created)
self.assertEqual(defaults["desc"], mdl.desc)
self.assertNotEqual(self.mdl.desc, mdl.desc)
# Hint query: use defauts to update without checking conflict
mdl2, created = await self.cls.update_or_create(
id=oldid, desc=desc, defaults=dict(mdl_dict, desc="new desc")
)
self.assertFalse(created)
self.assertNotEqual(dict(mdl), dict(mdl2))
# Missing query: success to create if no conflict
not_exist_name = str(uuid4())
no_conflict_defaults = {"name": not_exist_name, "desc": desc}
no_conflict_kwargs = {"name": not_exist_name}
mdl, created = await self.cls.update_or_create(no_conflict_defaults, **no_conflict_kwargs)
self.assertTrue(created)
self.assertEqual(not_exist_name, mdl.name)

async def test_first(self):
mdl = await self.cls.first()
self.assertEqual(self.mdl.id, mdl.id)
Expand Down
31 changes: 22 additions & 9 deletions tortoise/models.py
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

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 @@ -1026,22 +1026,35 @@ 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
:raises ParamsError: If defaults conflict with kwargs
"""
if not defaults:
defaults = {}
db = using_db or cls._choose_db(True)
try:
return await cls.filter(**kwargs).using_db(db).get(), False
except DoesNotExist:
return await cls._create_or_get(db, defaults, **kwargs)

@classmethod
async def _create_or_get(
cls, db: BaseDBAsyncClient, defaults: dict, **kwargs
) -> Tuple[Self, bool]:
"""Try to create, if fails with IntegrityError then try to get"""
for key in defaults.keys() & kwargs.keys():
if (default_value := defaults[key]) != (query_value := kwargs[key]):
raise ParamsError(f"Conflict value with {key=}: {default_value=} vs {query_value=}")
merged_defaults = {**kwargs, **defaults}
try:
async with in_transaction(connection_name=db.connection_name) as connection:
return await cls.create(using_db=connection, **merged_defaults), True
except IntegrityError as exc:
try:
async with in_transaction(connection_name=db.connection_name) as connection:
return await cls.create(using_db=connection, **defaults, **kwargs), True
except IntegrityError as exc:
try:
return await cls.filter(**kwargs).using_db(db).get(), False
except DoesNotExist:
pass
raise exc
return await cls.filter(**kwargs).using_db(db).get(), False
except DoesNotExist:
pass
raise exc

@classmethod
def select_for_update(
Expand Down Expand Up @@ -1084,7 +1097,7 @@ async def update_or_create(
if instance:
await instance.update_from_dict(defaults).save(using_db=connection)
return instance, False
return await cls.get_or_create(defaults, db, **kwargs)
return await cls._create_or_get(db, defaults, **kwargs)

@classmethod
async def create(
Expand Down