Api for upserting #798
Api for upserting #798
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #798 +/- ##
==========================================
+ Coverage 91.35% 91.37% +0.02%
==========================================
Files 108 108
Lines 7517 7538 +21
==========================================
+ Hits 6867 6888 +21
Misses 650 650
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Thanks for this, and the docs. |
|
No problem. It would be good if you have time to try and review this because if you merge this, it would also solve the problem with duplicate entries in the M2M, because then we will be able to add the if unsaved:
await rows[0].__class__.insert(
*unsaved, on_conflict=OnConflict.do_nothing
).run() |
piccolo/query/methods/insert.py
Outdated
| ): # pragma: no cover | ||
| if self.on_conflict_delegate._on_conflict == OnConflict.do_nothing: | ||
| base = f'INSERT OR IGNORE INTO "{self.table._meta.tablename}"' | ||
| else: |
There was a problem hiding this comment.
I would explicitly check this against OnConflict.do_update, and have the else clause raise a ValueError.
There was a problem hiding this comment.
I need clarification. Which ValueError should I raise because the user only has two valid options (nothing or update) and if the user enters a wrong value for on_conflict the AttributeError is already raised? If no on_conflict clause is specified, the standard insert query is executed.
There was a problem hiding this comment.
The value is meant to be an enum but because it's Python could be anything if the user set it to say a string by accident.
if self.on_conflict_delegate._on_conflict == OnConflict.do_nothing:
base = f'INSERT OR IGNORE INTO "{self.table._meta.tablename}"'
elif self.on_conflict_delegate._on_conflict == OnConflict.do_update:
...
else:
raise ValueError('Invalid on conflict value')
| @@ -36,6 +51,10 @@ def returning(self: Self, *columns: Column) -> Self: | |||
| self.returning_delegate.returning(columns) | |||
| return self | |||
|
|
|||
| def on_conflict(self: Self, conflict: OnConflict) -> Self: | |||
There was a problem hiding this comment.
Do you think we should allow the user to pass in specific columns?
await Band.insert(Band(name="Pythonistas")).on_conflict(Band.name, do_nothing=True)If not specified, then we default to all columns.
There was a problem hiding this comment.
If you look at the tests, even now the user can update one or more columns on do_update conflict. For do_nothing conflict I don't see point of allowing specifying columns because on do_nothing conflict columns don't change anyway.
| self, table: t.Type[TableInstance], *instances: TableInstance, **kwargs | ||
| self, | ||
| table: t.Type[TableInstance], | ||
| on_conflict: t.Optional[OnConflict] = None, |
There was a problem hiding this comment.
I would drop consider dropping this, and just using the on_conflict method instead.
The reason being, we can add extra arguments to the on_conflict method in the future, but we don't want to add too many to __init__.
There was a problem hiding this comment.
I don't think we should change it, but feel free to do as you think is best. With this method, we can also easily solve the problem with duplicate entries in M2M. I'm sorry if I didn't understand well what you wanted to say and feel free to change this if you think on_conflict as method would be better way.
|
Implemented in #816 |
Related to #252