fix: models get_or_create keyerror #1584
Conversation
|
Could you please rebase your PR on actual develop branch? |
yes, i did this |
There was a problem hiding this comment.
kwargsis 'Query parameters',defaultsis 'Default values used to update the object', when they have same key with different values, value ofkwargsshould be covered bydefaults. That is to say code must be:merged_defaults = {**kwargs, **defaults}- merged_defaults = ... should be in front of
try:(just put it after line 1082) return ...shoud be stay as one line, do not make it into three lines.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Even though that is how it works in django - for me it seems strange that
defaultvalue overrides explicit valueI 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=}')There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
the diff from github is wrong
…oise#1609) * fix: Make get_annotations evaluate annotations in default scope fixes tortoise#1552 * misc: update changelog and fix lint
…eprecated. Use the explicit style "transport=ASGITransport(app=...)" instead.` (tortoise#1606)
* optimize bulk operations - remove converting objects iterable to list in bulk_create - change chunk method to return iterables lazy * add changelog * revert BulkCreateQuery * add name to contributors
* refactor: optimize query utils * Use position argument instead of kwargs
* Remove duplicate version info * freeze package name to tortoise-orm * fix missing assert * fix Codacy Static Code Analysis issues * fix lint error
* Improve type hints for fastapi example * fix static code analysis * Add more type hint for test file
* fixed get_or_create * added transaction --------- Co-authored-by: Andrey Bondar <andrey@bondar.ru>
There was a problem hiding this comment.
Should add case about kwargs and defaults have same key and same value
| @@ -23,6 +23,7 @@ | |||
| from pypika.terms import Term | |||
| from typing_extensions import Self | |||
|
|
|||
| import tortoise | |||
| except DoesNotExist: | ||
| for key in (defaults.keys() & kwargs.keys()): | ||
| if (default_value := defaults[key]) != (query_value := kwargs[key]): | ||
| raise tortoise.exceptions.ParamsError( |
There was a problem hiding this comment.
Change it to:
raise ParamsError("...")Then run make style to reformat this file
#1583