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
tests/test_model_methods.py
Outdated
There was a problem hiding this comment.
Should add case about kwargs and defaults have same key and same value
tortoise/models.py
Outdated
| @@ -23,6 +23,7 @@ | |||
| from pypika.terms import Term | |||
| from typing_extensions import Self | |||
|
|
|||
| import tortoise | |||
tortoise/models.py
Outdated
| 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
|
@jiangying000 How is that? |
sorry, no progress yet, too busy dealing with my employment job tasks |
…et-or-create-keyerror
|
feel free to take on this pr, or close it |
I have make a pr, could you merge it into your develop branch? |
refactor: optimize update_or_create logic
done, thanks |
|
@abondar Could you review this? |
|
@waketzheng seems like there is some issue Also we would need to update changelog to put change in new section |
Fix ci error with mssql and update changelog
fix codacy issues
@abondar done |
Pull Request Test Coverage Report for Build 9501103456Warning: 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.
Details
💛 - Coveralls |
|
@abondar Cloud you review it? |
#1583