From 6533ee455d11e2d3fc74216afdbd188151053f2c Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 00:16:08 +0100 Subject: [PATCH 01/31] initial commit --- piccolo/query/methods/insert.py | 41 +++++++++- piccolo/query/mixins.py | 129 +++++++++++++++++++++++++++++++- tests/base.py | 2 +- tests/table/test_insert.py | 27 ++++++- 4 files changed, 189 insertions(+), 10 deletions(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index 9f31f445a..071b6ac1f 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -4,7 +4,12 @@ from piccolo.custom_types import TableInstance from piccolo.query.base import Query -from piccolo.query.mixins import AddDelegate, ReturningDelegate +from piccolo.query.mixins import ( + AddDelegate, + OnConflictAction, + OnConflictDelegate, + ReturningDelegate, +) from piccolo.querystring import QueryString if t.TYPE_CHECKING: # pragma: no cover @@ -15,7 +20,7 @@ class Insert( t.Generic[TableInstance], Query[TableInstance, t.List[t.Dict[str, t.Any]]] ): - __slots__ = ("add_delegate", "returning_delegate") + __slots__ = ("add_delegate", "on_conflict_delegate", "returning_delegate") def __init__( self, table: t.Type[TableInstance], *instances: TableInstance, **kwargs @@ -23,6 +28,7 @@ def __init__( super().__init__(table, **kwargs) self.add_delegate = AddDelegate() self.returning_delegate = ReturningDelegate() + self.on_conflict_delegate = OnConflictDelegate() self.add(*instances) ########################################################################### @@ -36,6 +42,21 @@ def returning(self: Self, *columns: Column) -> Self: self.returning_delegate.returning(columns) return self + def on_conflict( + self: Self, + target: t.Optional[t.Sequence[t.Union[str, Column]]] = None, + action: t.Union[ + OnConflictAction, t.Literal["DO NOTHING", "DO UPDATE"] + ] = OnConflictAction.do_nothing, + values: t.Optional[ + t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] + ] = None, + ) -> Self: + self.on_conflict_delegate.on_conflict( + target=target, action=action, values=values + ) + return self + ########################################################################### def _raw_response_callback(self, results): @@ -70,16 +91,28 @@ def default_querystrings(self) -> t.Sequence[QueryString]: engine_type = self.engine_type + # TODO - check SQLite version + on_conflict = self.on_conflict_delegate._on_conflict + if on_conflict: + querystring = QueryString( + "{}{}", + querystring, + on_conflict.querystring, + query_type="insert", + table=self.table, + ) + if engine_type in ("postgres", "cockroach") or ( engine_type == "sqlite" and self.table._meta.db.get_version_sync() >= 3.35 ): - if self.returning_delegate._returning: + returning = self.returning_delegate._returning + if returning: return [ QueryString( "{}{}", querystring, - self.returning_delegate._returning.querystring, + returning.querystring, query_type="insert", table=self.table, ) diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 109b48629..d4c35dcd7 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -581,9 +581,10 @@ class OffsetDelegate: Typically used in conjunction with order_by and limit. - Example usage: + Example usage:: + + .offset(100) - .offset(100) """ _offset: t.Optional[Offset] = None @@ -613,12 +614,132 @@ def __str__(self): @dataclass class GroupByDelegate: """ - Used to group results - needed when doing aggregation. + Used to group results - needed when doing aggregation:: + + .group_by(Band.name) - .group_by(Band.name) """ _group_by: t.Optional[GroupBy] = None def group_by(self, *columns: Column): self._group_by = GroupBy(columns=columns) + + +class OnConflictAction(str, Enum): + do_nothing = "DO NOTHING" + do_update = "DO UPDATE" + + +@dataclass +class OnConflict: + target: t.Optional[t.Sequence[t.Union[str, Column]]] = None + action: t.Optional[OnConflictAction] = None + values: t.Optional[ + t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] + ] = None + + @property + def target_string(self) -> str: + assert self.target + + def to_string(value) -> str: + if isinstance(value, Column): + return f'"{value._meta.name}"' + elif isinstance(value, str): + return value + else: + raise ValueError("OnConflict.target isn't a valid type") + + columns_str = ", ".join([to_string(i) for i in self.target]) + return f"({columns_str})" + + @property + def action_string(self) -> QueryString: + action = self.action + if isinstance(action, OnConflictAction): + if action == OnConflictAction.do_nothing: + return QueryString(OnConflictAction.do_nothing.value) + elif action == OnConflictAction.do_update: + values = [] + query = f"{OnConflictAction.do_update.value} SET" + + if not self.values: + raise ValueError("No values specified for `on conflict`") + + for value in self.values: + if isinstance(value, Column): + column_name = value._meta.db_column_name + query += f' "{column_name}"=EXCLUDED."{column_name}"' + elif isinstance(value, tuple): + column = value[0] + value_ = value[1] + if isinstance(column, Column): + column_name = column._meta.db_column_name + elif isinstance(column, str): + column_name = column + else: + raise ValueError("Unsupported column type") + + query += f' "{column_name}"={{}}' + values.append(value_) + + return QueryString(query, *values) + + raise ValueError("OnConflict.action isn't a valid type") + + @property + def querystring(self) -> QueryString: + query = " ON CONFLICT" + values = [] + + if self.target: + query += f" {self.target_string}" + + if self.action: + query += " {}" + values.append(self.action_string) + + return QueryString(query, *values) + + def __str__(self) -> str: + return self.querystring.__str__() + + +@dataclass +class OnConflictDelegate: + """ + Used with insert queries to specify what to do when a query fails due to + a constraint:: + + .on_conflict(action='DO NOTHING') + + .on_conflict(action='DO UPDATE', values=[Band.popularity]) + + .on_conflict(action='DO UPDATE', values=[(Band.popularity, 1)]) + + """ + + _on_conflict: t.Optional[OnConflict] = None + + def on_conflict( + self, + target: t.Optional[t.Sequence[t.Union[str, Column]]] = None, + action: t.Union[ + OnConflictAction, t.Literal["DO NOTHING", "DO UPDATE"] + ] = OnConflictAction.do_nothing, + values: t.Optional[ + t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] + ] = None, + ): + action_: OnConflictAction + if isinstance(action, OnConflictAction): + action_ = action + elif isinstance(action, str): + action_ = OnConflictAction(action) + else: + raise ValueError("Unrecognised `on conflict` action.") + + self._on_conflict = OnConflict( + action=action_, target=target, values=values + ) diff --git a/tests/base.py b/tests/base.py index f08312fc8..c04b9324e 100644 --- a/tests/base.py +++ b/tests/base.py @@ -234,7 +234,7 @@ def create_tables(self): """ CREATE TABLE band ( id SERIAL PRIMARY KEY, - name VARCHAR(50), + name VARCHAR(50) UNIQUE, manager INTEGER REFERENCES manager, popularity SMALLINT );""" diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 474497f25..d1764e205 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -1,6 +1,13 @@ +from unittest import TestCase + import pytest -from tests.base import DBTestCase, engine_version_lt, is_running_sqlite +from tests.base import ( + DBTestCase, + engine_version_lt, + is_running_sqlite, + postgres_only, +) from tests.example_apps.music.tables import Band, Manager @@ -76,3 +83,21 @@ def test_insert_returning_alias(self): ) self.assertListEqual(response, [{"manager_name": "Maz"}]) + + +class TestOnConflict(DBTestCase): + # TODO - make sure it works with other engines. + @postgres_only + def test_do_update(self): + self.insert_row() + + Band.insert(Band(name="Pythonistas", popularity=5000)).on_conflict( + target=[Band.name], + action="DO UPDATE", + values=[Band.popularity], + ).run_sync() + + self.assertListEqual( + Band.select(Band.name, Band.popularity).run_sync(), + [{"name": "Pythonistas", "popularity": 5000}], + ) From 0ee5c89aad993fadbe0ab4c8d7ee98f4416ebc85 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 09:58:17 +0100 Subject: [PATCH 02/31] fix tests --- tests/base.py | 2 +- tests/table/test_insert.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/base.py b/tests/base.py index c04b9324e..f08312fc8 100644 --- a/tests/base.py +++ b/tests/base.py @@ -234,7 +234,7 @@ def create_tables(self): """ CREATE TABLE band ( id SERIAL PRIMARY KEY, - name VARCHAR(50) UNIQUE, + name VARCHAR(50), manager INTEGER REFERENCES manager, popularity SMALLINT );""" diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index d1764e205..c0050d573 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -1,5 +1,3 @@ -from unittest import TestCase - import pytest from tests.base import ( @@ -89,6 +87,8 @@ class TestOnConflict(DBTestCase): # TODO - make sure it works with other engines. @postgres_only def test_do_update(self): + Band.alter().set_unique(Band.name).run_sync() + self.insert_row() Band.insert(Band(name="Pythonistas", popularity=5000)).on_conflict( From c58dbbc82174b4a5773f6f722d7e84908cb33e86 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 10:16:00 +0100 Subject: [PATCH 03/31] version pin litestar --- piccolo/apps/asgi/commands/new.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/piccolo/apps/asgi/commands/new.py b/piccolo/apps/asgi/commands/new.py index aedabdf93..f4b0e16e3 100644 --- a/piccolo/apps/asgi/commands/new.py +++ b/piccolo/apps/asgi/commands/new.py @@ -12,7 +12,7 @@ SERVERS = ["uvicorn", "Hypercorn"] ROUTERS = ["starlette", "fastapi", "blacksheep", "litestar"] ROUTER_DEPENDENCIES = { - "litestar": ["litestar>=2.0.0a3"], + "litestar": ["litestar==2.0.0a3"], } From ccc15389371bf539785f0b72dff2369fa8fa2283 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 10:23:32 +0100 Subject: [PATCH 04/31] use typing extensions for Literal --- piccolo/query/methods/insert.py | 4 +++- piccolo/query/mixins.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index 071b6ac1f..192d0db76 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -2,6 +2,8 @@ import typing as t +from typing_extensions import Literal + from piccolo.custom_types import TableInstance from piccolo.query.base import Query from piccolo.query.mixins import ( @@ -46,7 +48,7 @@ def on_conflict( self: Self, target: t.Optional[t.Sequence[t.Union[str, Column]]] = None, action: t.Union[ - OnConflictAction, t.Literal["DO NOTHING", "DO UPDATE"] + OnConflictAction, Literal["DO NOTHING", "DO UPDATE"] ] = OnConflictAction.do_nothing, values: t.Optional[ t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index d4c35dcd7..fb8374fb0 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -7,6 +7,8 @@ from dataclasses import dataclass, field from enum import Enum, auto +from typing_extensions import Literal + from piccolo.columns import And, Column, Or, Where from piccolo.columns.column_types import ForeignKey from piccolo.custom_types import Combinable @@ -726,7 +728,7 @@ def on_conflict( self, target: t.Optional[t.Sequence[t.Union[str, Column]]] = None, action: t.Union[ - OnConflictAction, t.Literal["DO NOTHING", "DO UPDATE"] + OnConflictAction, Literal["DO NOTHING", "DO UPDATE"] ] = OnConflictAction.do_nothing, values: t.Optional[ t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] From 03287bc8fc841c6f1aa4f1902c0baf65269040c6 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 17:49:09 +0100 Subject: [PATCH 05/31] make suggested change --- piccolo/query/mixins.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index fb8374fb0..58a701095 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -647,7 +647,7 @@ def target_string(self) -> str: def to_string(value) -> str: if isinstance(value, Column): - return f'"{value._meta.name}"' + return f'"{value._meta.db_column_name}"' elif isinstance(value, str): return value else: @@ -672,7 +672,7 @@ def action_string(self) -> QueryString: for value in self.values: if isinstance(value, Column): column_name = value._meta.db_column_name - query += f' "{column_name}"=EXCLUDED."{column_name}"' + query += f' "{column_name}"=EXCLUDED."{column_name}",' elif isinstance(value, tuple): column = value[0] value_ = value[1] @@ -686,7 +686,7 @@ def action_string(self) -> QueryString: query += f' "{column_name}"={{}}' values.append(value_) - return QueryString(query, *values) + return QueryString(query.rstrip(","), *values) raise ValueError("OnConflict.action isn't a valid type") From 87afd15d3f13f9f2df5edad7163dd6a59717c99e Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 17:50:08 +0100 Subject: [PATCH 06/31] undo --- piccolo/query/mixins.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 58a701095..0cdcfb880 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -672,7 +672,7 @@ def action_string(self) -> QueryString: for value in self.values: if isinstance(value, Column): column_name = value._meta.db_column_name - query += f' "{column_name}"=EXCLUDED."{column_name}",' + query += f' "{column_name}"=EXCLUDED."{column_name}"' elif isinstance(value, tuple): column = value[0] value_ = value[1] @@ -686,7 +686,7 @@ def action_string(self) -> QueryString: query += f' "{column_name}"={{}}' values.append(value_) - return QueryString(query.rstrip(","), *values) + return QueryString(query, *values) raise ValueError("OnConflict.action isn't a valid type") From 745630a77dc5b7cbcf00f993535c731e3a2bf85b Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 17:50:24 +0100 Subject: [PATCH 07/31] Update piccolo/query/mixins.py Co-authored-by: sinisaos --- piccolo/query/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 0cdcfb880..d70a739b5 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -686,7 +686,7 @@ def action_string(self) -> QueryString: query += f' "{column_name}"={{}}' values.append(value_) - return QueryString(query, *values) + return QueryString(query.rstrip(","), *values) raise ValueError("OnConflict.action isn't a valid type") From d32b89b47533084341b4070a55ffee45ae3ae7a6 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 17:50:32 +0100 Subject: [PATCH 08/31] Update piccolo/query/mixins.py Co-authored-by: sinisaos --- piccolo/query/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index d70a739b5..58a701095 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -672,7 +672,7 @@ def action_string(self) -> QueryString: for value in self.values: if isinstance(value, Column): column_name = value._meta.db_column_name - query += f' "{column_name}"=EXCLUDED."{column_name}"' + query += f' "{column_name}"=EXCLUDED."{column_name}",' elif isinstance(value, tuple): column = value[0] value_ = value[1] From 85ee5a0d119346e746168d9f2b24bb2fee1c5087 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 17:51:47 +0100 Subject: [PATCH 09/31] add one extra comma --- piccolo/query/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 0cdcfb880..256edb3b4 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -683,7 +683,7 @@ def action_string(self) -> QueryString: else: raise ValueError("Unsupported column type") - query += f' "{column_name}"={{}}' + query += f' "{column_name}"={{}},' values.append(value_) return QueryString(query, *values) From e171a1ae0b8deec0351e76b0ea0e0f8e78381a6a Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Fri, 28 Apr 2023 22:58:07 +0100 Subject: [PATCH 10/31] first attempt at docs --- docs/src/piccolo/query_types/insert.rst | 193 ++++++++++++++++-- docs/src/piccolo/query_types/insert/bands.csv | 2 + piccolo/query/mixins.py | 6 +- 3 files changed, 184 insertions(+), 17 deletions(-) create mode 100644 docs/src/piccolo/query_types/insert/bands.csv diff --git a/docs/src/piccolo/query_types/insert.rst b/docs/src/piccolo/query_types/insert.rst index eda460de1..a578fa306 100644 --- a/docs/src/piccolo/query_types/insert.rst +++ b/docs/src/piccolo/query_types/insert.rst @@ -3,33 +3,194 @@ Insert ====== -This is used to insert rows into the table. - -.. code-block:: python - - >>> await Band.insert(Band(name="Pythonistas")) - [{'id': 3}] - -We can insert multiple rows in one go: +This is used to bulk insert rows into the table: .. code-block:: python await Band.insert( + Band(name="Pythonistas") Band(name="Darts"), Band(name="Gophers") ) ------------------------------------------------------------------------------- -add ---- +``add`` +------- -You can also compose it as follows: +If we later decide to insert additional rows, we can use the ``add`` method: .. code-block:: python - await Band.insert().add( - Band(name="Darts") - ).add( - Band(name="Gophers") - ) + query = Band.insert(Band(name="Pythonistas")) + + if other_bands: + query = query.add( + Band(name="Darts"), + Band(name="Gophers") + ) + + await query + +------------------------------------------------------------------------------- + +``on_conflict`` +--------------- + +When inserting a row, if a constraint fails (for example, a unique constraint), +then the insertion fails. + +Using the ``on_conflict`` clause, we can instead tell the database to ignore +the error (using ``DO NOTHING``), or to update the row (using ``DO UPDATE``). + +This is sometimes called an **upsert** (update if it already exists else insert). + +Example data +~~~~~~~~~~~~ + +If we have the following table: + +.. code-block:: python + + class Band(Table): + name = Varchar(unique=True) + popularity = Integer() + +With this data: + +.. csv-table:: + :file: ./insert/bands.csv + +Let's try inserting another row with the same ``name``, and we'll get an error: + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ) + Unique constraint error! + +``DO NOTHING`` +~~~~~~~~~~~~~~ + +To ignore the error: + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING" + ... ) + +If we fetch the data from the database, we'll see that it hasn't changed: + +.. code-block:: python + + >>> await Band.select().where(Band.name == "Pythonistas").first() + {'id': 1, 'name': 'Pythonistas', 'popularity': 1000} + + +``DO UPDATE`` +~~~~~~~~~~~~~ + +Instead, if we want to update the ``popularity``: + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=[Band.popularity] + ... ) + +If we fetch the data from the database, we'll see that it was updated: + +.. code-block:: python + + >>> await Band.select().where(Band.name == "Pythonistas").first() + {'id': 1, 'name': 'Pythonistas', 'popularity': 1200} + +``target`` +~~~~~~~~~~ + +Using the ``target`` argument, we can specify which constraints we're concerned +with. By specifying ``target=[Band.name]`` we're only concerned with unique +constraints for the ``band`` column. If you omit the ``target`` argument, then +it works for all constraints on the table. + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING", + ... target=[Band.name] + ... ) + +You can also specify the name of a constraint using a string: + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING", + ... target=['some_constraint'] + ... ) + +``values`` +~~~~~~~~~~ + +This lets us specify which values to update when a conflict occurs. + +By specifying a :class:`Column `, this means that +the new value for that column will be used: + +.. code-block:: python + :emphasize-lines: 6 + + # The new popularity will be 1200. + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=[Band.popularity] + ... ) + +Instead, we can specify a custom value using a tuple: + +.. code-block:: python + :emphasize-lines: 6 + + # The new popularity will be 1111. + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=[(Band.popularity, 1111)] + ... ) + +Source +~~~~~~ + +.. currentmodule:: piccolo.query.methods.insert + +.. automethod:: Insert.on_conflict + +.. autoclass:: OnConflictAction + :members: + :undoc-members: + +------------------------------------------------------------------------------- + +Query clauses +------------- + +returning +~~~~~~~~~ + +See :ref:`Returning`. diff --git a/docs/src/piccolo/query_types/insert/bands.csv b/docs/src/piccolo/query_types/insert/bands.csv new file mode 100644 index 000000000..d796928a1 --- /dev/null +++ b/docs/src/piccolo/query_types/insert/bands.csv @@ -0,0 +1,2 @@ +id,name,popularity +1,Pythonistas,1000 diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index ce8c3c517..02771f6f3 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -629,6 +629,10 @@ def group_by(self, *columns: Column): class OnConflictAction(str, Enum): + """ + Specify which action to take on conflict. + """ + do_nothing = "DO NOTHING" do_update = "DO UPDATE" @@ -738,7 +742,7 @@ def on_conflict( if isinstance(action, OnConflictAction): action_ = action elif isinstance(action, str): - action_ = OnConflictAction(action) + action_ = OnConflictAction(action.upper()) else: raise ValueError("Unrecognised `on conflict` action.") From 96a22575a325c8df9e44461b35f850bd47be5a52 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 29 Apr 2023 14:42:54 +0100 Subject: [PATCH 11/31] add `NotImplementedError` for unsupported methods --- piccolo/query/methods/insert.py | 8 ++++++++ piccolo/query/methods/objects.py | 2 ++ piccolo/query/methods/select.py | 12 +++++------- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index 192d0db76..3da3f36d5 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -54,6 +54,14 @@ def on_conflict( t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] ] = None, ) -> Self: + if ( + self.engine_type == "sqlite" + and self.table._meta.db.get_version_sync() < 3.34 + ): + raise NotImplementedError( + "SQLite versions lower than 3.24 don't support ON CONFLICT" + ) + self.on_conflict_delegate.on_conflict( target=target, action=action, values=values ) diff --git a/piccolo/query/methods/objects.py b/piccolo/query/methods/objects.py index db9e43ccc..6892fc95c 100644 --- a/piccolo/query/methods/objects.py +++ b/piccolo/query/methods/objects.py @@ -230,6 +230,8 @@ def callback( return self def as_of(self, interval: str = "-1s") -> Objects: + if self.engine_type != "cockroach": + raise NotImplementedError("Only CockroachDB supports AS OF") self.as_of_delegate.as_of(interval) return self diff --git a/piccolo/query/methods/select.py b/piccolo/query/methods/select.py index f94e173dc..4e1949f55 100644 --- a/piccolo/query/methods/select.py +++ b/piccolo/query/methods/select.py @@ -356,13 +356,8 @@ def columns(self: Self, *columns: t.Union[Selectable, str]) -> Self: def distinct( self: Self, *, on: t.Optional[t.Sequence[Column]] = None ) -> Self: - if on is not None and self.engine_type not in ( - "postgres", - "cockroach", - ): - raise ValueError( - "Only Postgres and Cockroach supports DISTINCT ON" - ) + if on is not None and self.engine_type == "sqlite": + raise NotImplementedError("SQLite doesn't support DISTINCT ON") self.distinct_delegate.distinct(enabled=True, on=on) return self @@ -377,6 +372,9 @@ def group_by(self: Self, *columns: t.Union[Column, str]) -> Self: return self def as_of(self: Self, interval: str = "-1s") -> Self: + if self.engine_type != "cockroach": + raise NotImplementedError("Only CockroachDB supports AS OF") + self.as_of_delegate.as_of(interval) return self From 84cd6ec725aea9f4cf86bc77f8bd74fc4d09dd90 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 29 Apr 2023 14:52:38 +0100 Subject: [PATCH 12/31] fix typo in sqlite version number --- piccolo/query/methods/insert.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index 3da3f36d5..56739ecb2 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -56,7 +56,7 @@ def on_conflict( ) -> Self: if ( self.engine_type == "sqlite" - and self.table._meta.db.get_version_sync() < 3.34 + and self.table._meta.db.get_version_sync() < 3.24 ): raise NotImplementedError( "SQLite versions lower than 3.24 don't support ON CONFLICT" From 627528d1c5f80235a3dabc9567988c7ca294d60c Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 29 Apr 2023 14:58:18 +0100 Subject: [PATCH 13/31] fix tests --- tests/table/test_select.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/table/test_select.py b/tests/table/test_select.py index 2ce429c2d..892972aec 100644 --- a/tests/table/test_select.py +++ b/tests/table/test_select.py @@ -1364,12 +1364,12 @@ def test_distinct_on_sqlite(self): SQLite doesn't support ``DISTINCT ON``, so a ``ValueError`` should be raised. """ - with self.assertRaises(ValueError) as manager: + with self.assertRaises(NotImplementedError) as manager: Album.select().distinct(on=[Album.band]) self.assertEqual( manager.exception.__str__(), - "Only Postgres and Cockroach supports DISTINCT ON", + "SQLite doesn't support DISTINCT ON", ) @engines_only("postgres", "cockroach") From aa3d0a0b4a91d96ea2722ec7ab98a3b2e1438da7 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 20:56:48 +0100 Subject: [PATCH 14/31] get tests running for sqlite --- tests/table/test_insert.py | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index c0050d573..38cd7a2a9 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -1,11 +1,10 @@ +from unittest import TestCase + import pytest -from tests.base import ( - DBTestCase, - engine_version_lt, - is_running_sqlite, - postgres_only, -) +from piccolo.columns import Integer, Varchar +from piccolo.table import Table +from tests.base import DBTestCase, engine_version_lt, is_running_sqlite from tests.example_apps.music.tables import Band, Manager @@ -83,13 +82,27 @@ def test_insert_returning_alias(self): self.assertListEqual(response, [{"manager_name": "Maz"}]) -class TestOnConflict(DBTestCase): - # TODO - make sure it works with other engines. - @postgres_only +@pytest.mark.skipif( + is_running_sqlite() and engine_version_lt(3.24), + reason="SQLite version not supported", +) +class TestOnConflict(TestCase): + class Band(Table): + name = Varchar(unique=True) + popularity = Integer() + + def setUp(self) -> None: + self.Band.create_table().run_sync() + + def tearDown(self) -> None: + self.Band.alter().drop_table().run_sync() + def test_do_update(self): - Band.alter().set_unique(Band.name).run_sync() + Band = self.Band - self.insert_row() + Band( + {Band.name: "Pythonistas", Band.popularity: 1000} + ).save().run_sync() Band.insert(Band(name="Pythonistas", popularity=5000)).on_conflict( target=[Band.name], From 8c42dcc8b3101a99e2d2db974848fb5f08d4b7dd Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 21:15:24 +0100 Subject: [PATCH 15/31] add test for `do nothing` --- tests/table/test_insert.py | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 38cd7a2a9..7dda723a7 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -92,19 +92,23 @@ class Band(Table): popularity = Integer() def setUp(self) -> None: - self.Band.create_table().run_sync() + Band = self.Band + Band.create_table().run_sync() + self.band = Band({Band.name: "Pythonistas", Band.popularity: 1000}) + self.band.save().run_sync() def tearDown(self) -> None: - self.Band.alter().drop_table().run_sync() + Band = self.Band + Band.alter().drop_table().run_sync() def test_do_update(self): Band = self.Band - Band( - {Band.name: "Pythonistas", Band.popularity: 1000} - ).save().run_sync() + new_popularity = self.band.popularity + 1000 - Band.insert(Band(name="Pythonistas", popularity=5000)).on_conflict( + Band.insert( + Band(name=self.band.name, popularity=new_popularity) + ).on_conflict( target=[Band.name], action="DO UPDATE", values=[Band.popularity], @@ -112,5 +116,17 @@ def test_do_update(self): self.assertListEqual( Band.select(Band.name, Band.popularity).run_sync(), - [{"name": "Pythonistas", "popularity": 5000}], + [{"name": self.band.name, "popularity": new_popularity}], + ) + + def test_do_nothing(self): + Band = self.Band + + Band.insert(Band(name="Pythonistas", popularity=5000)).on_conflict( + action="DO NOTHING" + ).run_sync() + + self.assertListEqual( + Band.select(Band.name, Band.popularity).run_sync(), + [{"name": self.band.name, "popularity": self.band.popularity}], ) From 850481d62d00be18205fc99fbf2ec3d147bdc2ca Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 21:30:53 +0100 Subject: [PATCH 16/31] add test for violating non target constraint --- tests/table/test_insert.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 7dda723a7..2645cc6fd 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -1,12 +1,16 @@ +import sqlite3 from unittest import TestCase import pytest from piccolo.columns import Integer, Varchar from piccolo.table import Table +from piccolo.utils.lazy_loader import LazyLoader from tests.base import DBTestCase, engine_version_lt, is_running_sqlite from tests.example_apps.music.tables import Band, Manager +asyncpg = LazyLoader("asyncpg", globals(), "asyncpg") + class TestInsert(DBTestCase): def test_insert(self): @@ -102,6 +106,9 @@ def tearDown(self) -> None: Band.alter().drop_table().run_sync() def test_do_update(self): + """ + Make sure that `DO UPDATE` works. + """ Band = self.Band new_popularity = self.band.popularity + 1000 @@ -119,6 +126,31 @@ def test_do_update(self): [{"name": self.band.name, "popularity": new_popularity}], ) + def test_non_target(self): + """ + Make sure that if we specify a target constraint, but violate a + different constraint, then we still get the error. + """ + Band = self.Band + + new_popularity = self.band.popularity + 1000 + + with self.assertRaises(Exception) as manager: + Band.insert( + Band(name=self.band.name, popularity=new_popularity) + ).on_conflict( + target=[Band.id], # Target the primary key instead. + action="DO UPDATE", + values=[Band.popularity], + ).run_sync() + + if self.Band._meta.db.engine_type in ("postgres", "cockroach"): + self.assertIsInstance( + manager.exception, asyncpg.exceptions.UniqueViolationError + ) + elif self.Band._meta.db.engine_type == "sqlite": + self.assertIsInstance(manager.exception, sqlite3.IntegrityError) + def test_do_nothing(self): Band = self.Band From a29d9f7cb9de09f1afc6829925f07507e2865f36 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 21:39:42 +0100 Subject: [PATCH 17/31] remove old comment --- piccolo/query/methods/insert.py | 1 - 1 file changed, 1 deletion(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index 56739ecb2..8ce959d05 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -101,7 +101,6 @@ def default_querystrings(self) -> t.Sequence[QueryString]: engine_type = self.engine_type - # TODO - check SQLite version on_conflict = self.on_conflict_delegate._on_conflict if on_conflict: querystring = QueryString( From 636399090ed3691f96cccc338e312f0b882edbb5 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 21:40:15 +0100 Subject: [PATCH 18/31] allow multiple on conflict clauses --- piccolo/query/methods/insert.py | 2 +- piccolo/query/mixins.py | 28 ++++++++++++++++++++++++---- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index 8ce959d05..a3a4fe9c6 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -102,7 +102,7 @@ def default_querystrings(self) -> t.Sequence[QueryString]: engine_type = self.engine_type on_conflict = self.on_conflict_delegate._on_conflict - if on_conflict: + if on_conflict.on_conflict_items: querystring = QueryString( "{}{}", querystring, diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 02771f6f3..7f6e75938 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -638,7 +638,7 @@ class OnConflictAction(str, Enum): @dataclass -class OnConflict: +class OnConflictItem: target: t.Optional[t.Sequence[t.Union[str, Column]]] = None action: t.Optional[OnConflictAction] = None values: t.Optional[ @@ -712,6 +712,26 @@ def __str__(self) -> str: return self.querystring.__str__() +@dataclass +class OnConflict: + """ + Multiple `ON CONFLICT` statements are allowed - which is why we have this + parent class. + """ + + on_conflict_items: t.List[OnConflictItem] = field(default_factory=list) + + @property + def querystring(self) -> QueryString: + query = " ".join("{}" for i in self.on_conflict_items) + return QueryString( + query, *[i.querystring for i in self.on_conflict_items] + ) + + def __str__(self) -> str: + return self.querystring.__str__() + + @dataclass class OnConflictDelegate: """ @@ -726,7 +746,7 @@ class OnConflictDelegate: """ - _on_conflict: t.Optional[OnConflict] = None + _on_conflict: OnConflict = field(default_factory=OnConflict) def on_conflict( self, @@ -746,6 +766,6 @@ def on_conflict( else: raise ValueError("Unrecognised `on conflict` action.") - self._on_conflict = OnConflict( - action=action_, target=target, values=values + self._on_conflict.on_conflict_items.append( + OnConflictItem(action=action_, target=target, values=values) ) From a9c3e6e8b114bccd4f8c14b093d020dab86aa15f Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 21:47:08 +0100 Subject: [PATCH 19/31] `target` -> `targets` It accepts a list, so targets makes more sense. --- docs/src/piccolo/query_types/insert.rst | 14 +++++++------- piccolo/query/methods/insert.py | 4 ++-- piccolo/query/mixins.py | 16 ++++++++-------- tests/table/test_insert.py | 4 ++-- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/docs/src/piccolo/query_types/insert.rst b/docs/src/piccolo/query_types/insert.rst index a578fa306..ee7dc8f9b 100644 --- a/docs/src/piccolo/query_types/insert.rst +++ b/docs/src/piccolo/query_types/insert.rst @@ -112,12 +112,12 @@ If we fetch the data from the database, we'll see that it was updated: >>> await Band.select().where(Band.name == "Pythonistas").first() {'id': 1, 'name': 'Pythonistas', 'popularity': 1200} -``target`` -~~~~~~~~~~ +``targets`` +~~~~~~~~~~~ -Using the ``target`` argument, we can specify which constraints we're concerned -with. By specifying ``target=[Band.name]`` we're only concerned with unique -constraints for the ``band`` column. If you omit the ``target`` argument, then +Using the ``targets`` argument, we can specify which constraints we're concerned +with. By specifying ``targets=[Band.name]`` we're only concerned with unique +constraints for the ``band`` column. If you omit the ``targets`` argument, then it works for all constraints on the table. .. code-block:: python @@ -127,7 +127,7 @@ it works for all constraints on the table. ... Band(name="Pythonistas", popularity=1200) ... ).on_conflict( ... action="DO NOTHING", - ... target=[Band.name] + ... targets=[Band.name] ... ) You can also specify the name of a constraint using a string: @@ -139,7 +139,7 @@ You can also specify the name of a constraint using a string: ... Band(name="Pythonistas", popularity=1200) ... ).on_conflict( ... action="DO NOTHING", - ... target=['some_constraint'] + ... targets=['some_constraint'] ... ) ``values`` diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index a3a4fe9c6..da50ec60b 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -46,7 +46,7 @@ def returning(self: Self, *columns: Column) -> Self: def on_conflict( self: Self, - target: t.Optional[t.Sequence[t.Union[str, Column]]] = None, + targets: t.Optional[t.Sequence[t.Union[str, Column]]] = None, action: t.Union[ OnConflictAction, Literal["DO NOTHING", "DO UPDATE"] ] = OnConflictAction.do_nothing, @@ -63,7 +63,7 @@ def on_conflict( ) self.on_conflict_delegate.on_conflict( - target=target, action=action, values=values + targets=targets, action=action, values=values ) return self diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 7f6e75938..125cf078a 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -639,15 +639,15 @@ class OnConflictAction(str, Enum): @dataclass class OnConflictItem: - target: t.Optional[t.Sequence[t.Union[str, Column]]] = None + targets: t.Optional[t.Sequence[t.Union[str, Column]]] = None action: t.Optional[OnConflictAction] = None values: t.Optional[ t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] ] = None @property - def target_string(self) -> str: - assert self.target + def targets_string(self) -> str: + assert self.targets def to_string(value) -> str: if isinstance(value, Column): @@ -657,7 +657,7 @@ def to_string(value) -> str: else: raise ValueError("OnConflict.target isn't a valid type") - columns_str = ", ".join([to_string(i) for i in self.target]) + columns_str = ", ".join([to_string(i) for i in self.targets]) return f"({columns_str})" @property @@ -699,8 +699,8 @@ def querystring(self) -> QueryString: query = " ON CONFLICT" values = [] - if self.target: - query += f" {self.target_string}" + if self.targets: + query += f" {self.targets_string}" if self.action: query += " {}" @@ -750,7 +750,7 @@ class OnConflictDelegate: def on_conflict( self, - target: t.Optional[t.Sequence[t.Union[str, Column]]] = None, + targets: t.Optional[t.Sequence[t.Union[str, Column]]] = None, action: t.Union[ OnConflictAction, Literal["DO NOTHING", "DO UPDATE"] ] = OnConflictAction.do_nothing, @@ -767,5 +767,5 @@ def on_conflict( raise ValueError("Unrecognised `on conflict` action.") self._on_conflict.on_conflict_items.append( - OnConflictItem(action=action_, target=target, values=values) + OnConflictItem(action=action_, targets=targets, values=values) ) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 2645cc6fd..37bb6d16e 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -116,7 +116,7 @@ def test_do_update(self): Band.insert( Band(name=self.band.name, popularity=new_popularity) ).on_conflict( - target=[Band.name], + targets=[Band.name], action="DO UPDATE", values=[Band.popularity], ).run_sync() @@ -139,7 +139,7 @@ def test_non_target(self): Band.insert( Band(name=self.band.name, popularity=new_popularity) ).on_conflict( - target=[Band.id], # Target the primary key instead. + targets=[Band.id], # Target the primary key instead. action="DO UPDATE", values=[Band.popularity], ).run_sync() From ee32a1b31df1d649467f5537379d7d7fe2ca4167 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 21:47:32 +0100 Subject: [PATCH 20/31] add docstring for `test_do_nothing` --- tests/table/test_insert.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 37bb6d16e..1f1f08d1a 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -152,6 +152,9 @@ def test_non_target(self): self.assertIsInstance(manager.exception, sqlite3.IntegrityError) def test_do_nothing(self): + """ + Make sure that `DO NOTHING` works. + """ Band = self.Band Band.insert(Band(name="Pythonistas", popularity=5000)).on_conflict( From ae973ec505664e9a9c6b341a89a69245d8a35489 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 22:28:20 +0100 Subject: [PATCH 21/31] add tests for multiple ON CONFLICT clauses --- piccolo/query/methods/insert.py | 10 +++ piccolo/query/mixins.py | 2 +- tests/table/test_insert.py | 109 ++++++++++++++++++++++++++++++-- 3 files changed, 113 insertions(+), 8 deletions(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index da50ec60b..d767f869b 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -62,6 +62,16 @@ def on_conflict( "SQLite versions lower than 3.24 don't support ON CONFLICT" ) + if ( + self.engine_type in ("postgres", "cockroach") + and len(self.on_conflict_delegate._on_conflict.on_conflict_items) + == 1 + ): + raise NotImplementedError( + "Postgres and Cockroach only support a single ON CONFLICT " + "clause." + ) + self.on_conflict_delegate.on_conflict( targets=targets, action=action, values=values ) diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 125cf078a..8020718cd 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -723,7 +723,7 @@ class OnConflict: @property def querystring(self) -> QueryString: - query = " ".join("{}" for i in self.on_conflict_items) + query = "".join("{}" for i in self.on_conflict_items) return QueryString( query, *[i.querystring for i in self.on_conflict_items] ) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 1f1f08d1a..fe10413e0 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -6,7 +6,12 @@ from piccolo.columns import Integer, Varchar from piccolo.table import Table from piccolo.utils.lazy_loader import LazyLoader -from tests.base import DBTestCase, engine_version_lt, is_running_sqlite +from tests.base import ( + DBTestCase, + engine_version_lt, + engines_only, + is_running_sqlite, +) from tests.example_apps.music.tables import Band, Manager asyncpg = LazyLoader("asyncpg", globals(), "asyncpg") @@ -122,8 +127,14 @@ def test_do_update(self): ).run_sync() self.assertListEqual( - Band.select(Band.name, Band.popularity).run_sync(), - [{"name": self.band.name, "popularity": new_popularity}], + Band.select().run_sync(), + [ + { + "id": self.band.id, + "name": self.band.name, + "popularity": new_popularity, # changed + } + ], ) def test_non_target(self): @@ -157,11 +168,95 @@ def test_do_nothing(self): """ Band = self.Band - Band.insert(Band(name="Pythonistas", popularity=5000)).on_conflict( - action="DO NOTHING" + new_popularity = self.band.popularity + 1000 + + Band.insert( + Band(name="Pythonistas", popularity=new_popularity) + ).on_conflict(action="DO NOTHING").run_sync() + + self.assertListEqual( + Band.select().run_sync(), + [ + { + "id": self.band.id, + "name": self.band.name, + "popularity": self.band.popularity, + } + ], + ) + + @engines_only("sqlite") + def test_multiple_do_update(self): + """ + Make sure multiple `ON CONFLICT` clauses work. + """ + Band = self.Band + + new_popularity = self.band.popularity + 1000 + + # Conflicting with name - should update. + Band.insert( + Band(name="Pythonistas", popularity=new_popularity) + ).on_conflict(action="DO NOTHING", targets=[Band.id]).on_conflict( + action="DO UPDATE", targets=[Band.name], values=[Band.popularity] ).run_sync() self.assertListEqual( - Band.select(Band.name, Band.popularity).run_sync(), - [{"name": self.band.name, "popularity": self.band.popularity}], + Band.select().run_sync(), + [ + { + "id": self.band.id, + "name": self.band.name, + "popularity": new_popularity, # changed + } + ], + ) + + @engines_only("sqlite") + def test_multiple_do_nothing(self): + """ + Make sure multiple `ON CONFLICT` clauses work for SQLite. + """ + Band = self.Band + + new_popularity = self.band.popularity + 1000 + + # Conflicting with ID - should be ignored. + Band.insert( + Band( + id=self.band.id, + name="Pythonistas", + popularity=new_popularity, + ) + ).on_conflict(action="DO NOTHING", targets=[Band.id]).on_conflict( + action="DO UPDATE", + targets=[Band.name], + values=[Band.popularity], + ).run_sync() + + self.assertListEqual( + Band.select().run_sync(), + [ + { + "id": self.band.id, + "name": self.band.name, + "popularity": self.band.popularity, + } + ], + ) + + @engines_only("postgres", "cockroach") + def tset_mutiple_error(self): + """ + Postgres and Cockroach don't support multiple `ON CONFLICT` clauses. + """ + with self.assertRaises(NotImplementedError) as manager: + Band = self.Band + + Band.insert(Band()).on_conflict(action="DO NOTHING").on_conflict( + action="DO UPDATE", + ).run_sync() + + assert manager.exception.__str__() == ( + "Postgres and Cockroach only support a single ON CONFLICT clause." ) From 78415d4e7ad5b616b5b53610b4cca944225bd961 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 22:29:10 +0100 Subject: [PATCH 22/31] add docs for multiple ``on_conflict`` clauses --- docs/src/piccolo/query_types/insert.rst | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/src/piccolo/query_types/insert.rst b/docs/src/piccolo/query_types/insert.rst index ee7dc8f9b..fb0478d6e 100644 --- a/docs/src/piccolo/query_types/insert.rst +++ b/docs/src/piccolo/query_types/insert.rst @@ -174,6 +174,24 @@ Instead, we can specify a custom value using a tuple: ... values=[(Band.popularity, 1111)] ... ) +Multiple ``on_conflict`` clauses +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +SQLite allows you to specify multiple ``ON CONFLICT`` clauses, but Postgres and +Cockroach don't. + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... ... + ... ).on_conflict( + ... action="DO NOTHING", + ... ... + ) + Source ~~~~~~ From 455200b4917dc2780f0f4584f0ab49a6364c4aa8 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 22:29:27 +0100 Subject: [PATCH 23/31] add docs for using `all_columns` --- docs/src/piccolo/query_types/insert.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/src/piccolo/query_types/insert.rst b/docs/src/piccolo/query_types/insert.rst index fb0478d6e..69ff840d9 100644 --- a/docs/src/piccolo/query_types/insert.rst +++ b/docs/src/piccolo/query_types/insert.rst @@ -174,6 +174,18 @@ Instead, we can specify a custom value using a tuple: ... values=[(Band.popularity, 1111)] ... ) +If we want to update all of the values, we can use :meth:`all_columns`. + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(id=1, name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=Band.all_columns() + ... ) + Multiple ``on_conflict`` clauses ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ From d20affff68ce223fa34e7a23cdaeb87c18c763e8 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 22:31:38 +0100 Subject: [PATCH 24/31] fix typo in test name --- tests/table/test_insert.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index fe10413e0..011a35e35 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -188,7 +188,7 @@ def test_do_nothing(self): @engines_only("sqlite") def test_multiple_do_update(self): """ - Make sure multiple `ON CONFLICT` clauses work. + Make sure multiple `ON CONFLICT` clauses work for SQLite. """ Band = self.Band @@ -246,7 +246,7 @@ def test_multiple_do_nothing(self): ) @engines_only("postgres", "cockroach") - def tset_mutiple_error(self): + def test_mutiple_error(self): """ Postgres and Cockroach don't support multiple `ON CONFLICT` clauses. """ From 11de6f601cb8f51d36ae4915b8b91dd88ab59b3f Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 22:36:59 +0100 Subject: [PATCH 25/31] add test for using `all_columns` --- tests/table/test_insert.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 011a35e35..66bbf7b98 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -260,3 +260,37 @@ def test_mutiple_error(self): assert manager.exception.__str__() == ( "Postgres and Cockroach only support a single ON CONFLICT clause." ) + + def test_all_columns(self): + """ + We can use ``all_columns`` instead of specifying the ``values`` + manually. + """ + Band = self.Band + + new_popularity = self.band.popularity + 1000 + new_name = "Rustaceans" + + # Conflicting with ID - should be ignored. + Band.insert( + Band( + id=self.band.id, + name=new_name, + popularity=new_popularity, + ) + ).on_conflict( + action="DO UPDATE", + targets=[Band.id], + values=Band.all_columns(), + ).run_sync() + + self.assertListEqual( + Band.select().run_sync(), + [ + { + "id": self.band.id, + "name": new_name, + "popularity": new_popularity, + } + ], + ) From ba0e8e2c7179c4f9572e8e70c07b60977c1e120e Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 22:37:11 +0100 Subject: [PATCH 26/31] add test for using an enum to specify the action --- tests/table/test_insert.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 66bbf7b98..3809f3cba 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -4,6 +4,7 @@ import pytest from piccolo.columns import Integer, Varchar +from piccolo.query.methods.insert import OnConflictAction from piccolo.table import Table from piccolo.utils.lazy_loader import LazyLoader from tests.base import ( @@ -294,3 +295,29 @@ def test_all_columns(self): } ], ) + + def test_enum(self): + """ + A string literal can be passed in, or an Enum, to determine the action. + Make sure that the enum works. + """ + Band = self.Band + + Band.insert( + Band( + id=self.band.id, + name=self.band.name, + popularity=self.band.popularity, + ) + ).on_conflict(action=OnConflictAction.do_nothing).run_sync() + + self.assertListEqual( + Band.select().run_sync(), + [ + { + "id": self.band.id, + "name": self.band.name, + "popularity": self.band.popularity, + } + ], + ) From 0d2ce6d807bc708f34ee11f317e3e8ed312aa5bf Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sun, 30 Apr 2023 22:41:37 +0100 Subject: [PATCH 27/31] add a test to make sure `DO UPDATE` with no values raises an exception --- tests/table/test_insert.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 3809f3cba..194b7e73e 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -138,7 +138,28 @@ def test_do_update(self): ], ) - def test_non_target(self): + def test_do_update_no_values(self): + """ + Make sure that `DO UPDATE` with no `values` raises an exception. + """ + Band = self.Band + + new_popularity = self.band.popularity + 1000 + + with self.assertRaises(ValueError) as manager: + Band.insert( + Band(name=self.band.name, popularity=new_popularity) + ).on_conflict( + targets=[Band.name], + action="DO UPDATE", + ).run_sync() + + self.assertEqual( + manager.exception.__str__(), + "No values specified for `on conflict`", + ) + + def test_violate_non_target(self): """ Make sure that if we specify a target constraint, but violate a different constraint, then we still get the error. From bbe4c6f2f9867c5b3d751a51c07bd07544426f29 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Tue, 2 May 2023 23:12:22 +0100 Subject: [PATCH 28/31] rename `targets` back to `target` --- docs/src/piccolo/query_types/insert.rst | 31 ++++++-- piccolo/query/methods/insert.py | 9 ++- piccolo/query/mixins.py | 42 ++++++---- tests/table/test_insert.py | 100 +++++++++++++++++++++--- 4 files changed, 145 insertions(+), 37 deletions(-) diff --git a/docs/src/piccolo/query_types/insert.rst b/docs/src/piccolo/query_types/insert.rst index 69ff840d9..110464496 100644 --- a/docs/src/piccolo/query_types/insert.rst +++ b/docs/src/piccolo/query_types/insert.rst @@ -37,6 +37,10 @@ If we later decide to insert additional rows, we can use the ``add`` method: ``on_conflict`` --------------- +.. hint:: This is an advanced topic, and first time learners of Piccolo + can skip if they want. + + When inserting a row, if a constraint fails (for example, a unique constraint), then the insertion fails. @@ -112,12 +116,12 @@ If we fetch the data from the database, we'll see that it was updated: >>> await Band.select().where(Band.name == "Pythonistas").first() {'id': 1, 'name': 'Pythonistas', 'popularity': 1200} -``targets`` -~~~~~~~~~~~ +``target`` +~~~~~~~~~~ -Using the ``targets`` argument, we can specify which constraints we're concerned -with. By specifying ``targets=[Band.name]`` we're only concerned with unique -constraints for the ``band`` column. If you omit the ``targets`` argument, then +Using the ``target`` argument, we can specify which constraint we're concerned +with. By specifying ``target=Band.name`` we're only concerned with unique +constraint for the ``band`` column. If you omit the ``target`` argument, then it works for all constraints on the table. .. code-block:: python @@ -127,7 +131,20 @@ it works for all constraints on the table. ... Band(name="Pythonistas", popularity=1200) ... ).on_conflict( ... action="DO NOTHING", - ... targets=[Band.name] + ... target=Band.name + ... ) + +If you want to target a composite unique constraint, you can do so by passing +in a tuple of columns: + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING", + ... target=(Band.name, Band.popularity) ... ) You can also specify the name of a constraint using a string: @@ -139,7 +156,7 @@ You can also specify the name of a constraint using a string: ... Band(name="Pythonistas", popularity=1200) ... ).on_conflict( ... action="DO NOTHING", - ... targets=['some_constraint'] + ... target='some_constraint' ... ) ``values`` diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index d767f869b..fa68d92cf 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -4,7 +4,7 @@ from typing_extensions import Literal -from piccolo.custom_types import TableInstance +from piccolo.custom_types import Combinable, TableInstance from piccolo.query.base import Query from piccolo.query.mixins import ( AddDelegate, @@ -46,13 +46,14 @@ def returning(self: Self, *columns: Column) -> Self: def on_conflict( self: Self, - targets: t.Optional[t.Sequence[t.Union[str, Column]]] = None, + target: t.Optional[t.Union[str, Column, t.Tuple[Column, ...]]] = None, action: t.Union[ OnConflictAction, Literal["DO NOTHING", "DO UPDATE"] ] = OnConflictAction.do_nothing, values: t.Optional[ - t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] + t.Sequence[t.Union[Column, t.Tuple[Column, t.Any]]] ] = None, + where: t.Optional[Combinable] = None, ) -> Self: if ( self.engine_type == "sqlite" @@ -73,7 +74,7 @@ def on_conflict( ) self.on_conflict_delegate.on_conflict( - targets=targets, action=action, values=values + target=target, where=where, action=action, values=values ) return self diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 8020718cd..4637221fa 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -639,26 +639,33 @@ class OnConflictAction(str, Enum): @dataclass class OnConflictItem: - targets: t.Optional[t.Sequence[t.Union[str, Column]]] = None + target: t.Optional[t.Union[str, Column, t.Tuple[Column, ...]]] = None + where: t.Optional[Combinable] = None action: t.Optional[OnConflictAction] = None values: t.Optional[ - t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] + t.Sequence[t.Union[Column, t.Tuple[Column, t.Any]]] ] = None @property - def targets_string(self) -> str: - assert self.targets + def target_string(self) -> str: + target = self.target + assert target def to_string(value) -> str: if isinstance(value, Column): return f'"{value._meta.db_column_name}"' - elif isinstance(value, str): - return value else: raise ValueError("OnConflict.target isn't a valid type") - columns_str = ", ".join([to_string(i) for i in self.targets]) - return f"({columns_str})" + if isinstance(target, str): + return f'ON CONSTRAINT "{target}"' + elif isinstance(target, Column): + return f"({to_string(target)})" + elif isinstance(target, tuple): + columns_str = ", ".join([to_string(i) for i in target]) + return f"({columns_str})" + else: + raise ValueError("OnConflict.target isn't a valid type") @property def action_string(self) -> QueryString: @@ -682,8 +689,6 @@ def action_string(self) -> QueryString: value_ = value[1] if isinstance(column, Column): column_name = column._meta.db_column_name - elif isinstance(column, str): - column_name = column else: raise ValueError("Unsupported column type") @@ -699,8 +704,12 @@ def querystring(self) -> QueryString: query = " ON CONFLICT" values = [] - if self.targets: - query += f" {self.targets_string}" + if self.target: + query += f" {self.target_string}" + + if self.where: + query += " WHERE {}" + values.append(self.where.querystring) if self.action: query += " {}" @@ -750,12 +759,13 @@ class OnConflictDelegate: def on_conflict( self, - targets: t.Optional[t.Sequence[t.Union[str, Column]]] = None, + target: t.Optional[t.Union[str, Column, t.Tuple[Column, ...]]] = None, + where: t.Optional[Combinable] = None, action: t.Union[ OnConflictAction, Literal["DO NOTHING", "DO UPDATE"] ] = OnConflictAction.do_nothing, values: t.Optional[ - t.List[t.Union[Column, t.Tuple[t.Union[str, Column], t.Any]]] + t.Sequence[t.Union[Column, t.Tuple[Column, t.Any]]] ] = None, ): action_: OnConflictAction @@ -767,5 +777,7 @@ def on_conflict( raise ValueError("Unrecognised `on conflict` action.") self._on_conflict.on_conflict_items.append( - OnConflictItem(action=action_, targets=targets, values=values) + OnConflictItem( + target=target, where=where, action=action_, values=values + ) ) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 194b7e73e..d8170f10d 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -122,7 +122,7 @@ def test_do_update(self): Band.insert( Band(name=self.band.name, popularity=new_popularity) ).on_conflict( - targets=[Band.name], + target=Band.name, action="DO UPDATE", values=[Band.popularity], ).run_sync() @@ -150,7 +150,7 @@ def test_do_update_no_values(self): Band.insert( Band(name=self.band.name, popularity=new_popularity) ).on_conflict( - targets=[Band.name], + target=Band.name, action="DO UPDATE", ).run_sync() @@ -159,6 +159,59 @@ def test_do_update_no_values(self): "No values specified for `on conflict`", ) + @engines_only("postgres", "cockroach") + def test_target_tuple(self): + """ + Make sure that a composite unique constraint can be used as a target. + + We only run it on Postgres and Cockroach because we use ALTER TABLE + to add a contraint, which SQLite doesn't support. + """ + Band = self.Band + + # Add a composite unique constraint: + Band.raw( + "ALTER TABLE band ADD CONSTRAINT id_name_unique UNIQUE (id, name)" + ).run_sync() + + Band.insert( + Band( + id=self.band.id, + name=self.band.name, + popularity=self.band.popularity, + ) + ).on_conflict( + target=(Band.id, Band.name), + action="DO NOTHING", + ).run_sync() + + @engines_only("postgres", "cockroach") + def test_target_string(self): + """ + Make sure we can explicitly specify the name of target constraint using + a string. + + We just test this on Postgres for now, as we have to get the constraint + name from the database. + """ + Band = self.Band + + constraint_name = Band.raw( + """ + SELECT constraint_name + FROM information_schema.constraint_column_usage + WHERE column_name = 'name' + AND table_name = 'band'; + """ + ).run_sync()[0]["constraint_name"] + + query = Band.insert(Band(name=self.band.name)).on_conflict( + target=constraint_name, + action="DO NOTHING", + ) + self.assertIn(f'ON CONSTRAINT "{constraint_name}"', query.__str__()) + query.run_sync() + def test_violate_non_target(self): """ Make sure that if we specify a target constraint, but violate a @@ -172,7 +225,7 @@ def test_violate_non_target(self): Band.insert( Band(name=self.band.name, popularity=new_popularity) ).on_conflict( - targets=[Band.id], # Target the primary key instead. + target=Band.id, # Target the primary key instead. action="DO UPDATE", values=[Band.popularity], ).run_sync() @@ -184,6 +237,30 @@ def test_violate_non_target(self): elif self.Band._meta.db.engine_type == "sqlite": self.assertIsInstance(manager.exception, sqlite3.IntegrityError) + def test_where(self): + """ + Make sure we can pass in a `where` argument. + """ + Band = self.Band + + new_popularity = self.band.popularity + 1000 + + query = Band.insert( + Band(name=self.band.name, popularity=new_popularity) + ).on_conflict( + target=Band.name, + where=Band.popularity < self.band.popularity, + action="DO UPDATE", + values=[Band.popularity], + ) + + self.assertIn( + f'WHERE "band"."popularity" < {self.band.popularity}', + query.__str__(), + ) + + query.run_sync() + def test_do_nothing(self): """ Make sure that `DO NOTHING` works. @@ -219,8 +296,8 @@ def test_multiple_do_update(self): # Conflicting with name - should update. Band.insert( Band(name="Pythonistas", popularity=new_popularity) - ).on_conflict(action="DO NOTHING", targets=[Band.id]).on_conflict( - action="DO UPDATE", targets=[Band.name], values=[Band.popularity] + ).on_conflict(action="DO NOTHING", target=Band.id).on_conflict( + action="DO UPDATE", target=Band.name, values=[Band.popularity] ).run_sync() self.assertListEqual( @@ -250,9 +327,9 @@ def test_multiple_do_nothing(self): name="Pythonistas", popularity=new_popularity, ) - ).on_conflict(action="DO NOTHING", targets=[Band.id]).on_conflict( + ).on_conflict(action="DO NOTHING", target=Band.id).on_conflict( action="DO UPDATE", - targets=[Band.name], + target=Band.name, values=[Band.popularity], ).run_sync() @@ -294,7 +371,7 @@ def test_all_columns(self): new_name = "Rustaceans" # Conflicting with ID - should be ignored. - Band.insert( + q = Band.insert( Band( id=self.band.id, name=new_name, @@ -302,9 +379,10 @@ def test_all_columns(self): ) ).on_conflict( action="DO UPDATE", - targets=[Band.id], + target=Band.id, values=Band.all_columns(), - ).run_sync() + ) + q.run_sync() self.assertListEqual( Band.select().run_sync(), @@ -319,7 +397,7 @@ def test_all_columns(self): def test_enum(self): """ - A string literal can be passed in, or an Enum, to determine the action. + A string literal can be passed in, or an enum, to determine the action. Make sure that the enum works. """ Band = self.Band From 2f3881a5faf265f306a8f64b650da28a7be8fefa Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Tue, 2 May 2023 23:16:01 +0100 Subject: [PATCH 29/31] integrate @sinisaos tests --- tests/table/test_insert.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index d8170f10d..6b576f5c3 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -138,6 +138,41 @@ def test_do_update(self): ], ) + def test_do_update_tuple_values(self): + """ + Make sure we can use tuples in ``values``. + """ + Band = self.Band + + new_popularity = self.band.popularity + 1000 + new_name = "Rustaceans" + + Band.insert( + Band( + id=self.band.id, + name=new_name, + popularity=new_popularity, + ) + ).on_conflict( + action="DO UPDATE", + target=Band.id, + values=[ + (Band.name, new_name), + (Band.popularity, new_popularity + 2000), + ], + ).run_sync() + + self.assertListEqual( + Band.select().run_sync(), + [ + { + "id": self.band.id, + "name": new_name, + "popularity": new_popularity + 2000, + } + ], + ) + def test_do_update_no_values(self): """ Make sure that `DO UPDATE` with no `values` raises an exception. From 54c02da139080b12c25f473f4317bfd4f94558fa Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Wed, 3 May 2023 16:35:05 +0100 Subject: [PATCH 30/31] move `on_conflict` to its own page --- docs/src/piccolo/query_clauses/as_of.rst | 4 +- docs/src/piccolo/query_clauses/index.rst | 1 + .../src/piccolo/query_clauses/on_conflict.rst | 229 ++++++++++++++++++ .../on_conflict}/bands.csv | 0 docs/src/piccolo/query_types/insert.rst | 204 +--------------- 5 files changed, 237 insertions(+), 201 deletions(-) create mode 100644 docs/src/piccolo/query_clauses/on_conflict.rst rename docs/src/piccolo/{query_types/insert => query_clauses/on_conflict}/bands.csv (100%) diff --git a/docs/src/piccolo/query_clauses/as_of.rst b/docs/src/piccolo/query_clauses/as_of.rst index 2d8407e42..97527033b 100644 --- a/docs/src/piccolo/query_clauses/as_of.rst +++ b/docs/src/piccolo/query_clauses/as_of.rst @@ -3,6 +3,8 @@ as_of ===== +.. note:: Cockroach only. + You can use ``as_of`` clause with the following queries: * :ref:`Select` @@ -21,5 +23,3 @@ This generates an ``AS OF SYSTEM TIME`` clause. See `documentation `_. This is very useful for performance, as it will reduce transaction contention across a cluster. - -Currently only supported on Cockroach Engine. diff --git a/docs/src/piccolo/query_clauses/index.rst b/docs/src/piccolo/query_clauses/index.rst index abb3fa18b..f9167ff63 100644 --- a/docs/src/piccolo/query_clauses/index.rst +++ b/docs/src/piccolo/query_clauses/index.rst @@ -26,6 +26,7 @@ by modifying the return values. ./freeze ./group_by ./offset + ./on_conflict ./output ./returning diff --git a/docs/src/piccolo/query_clauses/on_conflict.rst b/docs/src/piccolo/query_clauses/on_conflict.rst new file mode 100644 index 000000000..9ae8444e3 --- /dev/null +++ b/docs/src/piccolo/query_clauses/on_conflict.rst @@ -0,0 +1,229 @@ +.. _on_conflict: + +on_conflict +=========== + +.. hint:: This is an advanced topic, and first time learners of Piccolo + can skip if they want. + +You can use the ``on_conflict`` clause with the following queries: + +* :ref:`Insert` + +Introduction +------------ + +When inserting rows into a table, if a unique constraint fails on one or more +of the rows, then the insertion fails. + +Using the ``on_conflict`` clause, we can instead tell the database to ignore +the error (using ``DO NOTHING``), or to update the row (using ``DO UPDATE``). + +This is sometimes called an **upsert** (update if it already exists else insert). + +Example data +------------ + +If we have the following table: + +.. code-block:: python + + class Band(Table): + name = Varchar(unique=True) + popularity = Integer() + +With this data: + +.. csv-table:: + :file: ./on_conflict/bands.csv + +Let's try inserting another row with the same ``name``, and we'll get an error: + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ) + Unique constraint error! + +``DO NOTHING`` +-------------- + +To ignore the error: + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING" + ... ) + +If we fetch the data from the database, we'll see that it hasn't changed: + +.. code-block:: python + + >>> await Band.select().where(Band.name == "Pythonistas").first() + {'id': 1, 'name': 'Pythonistas', 'popularity': 1000} + + +``DO UPDATE`` +------------- + +Instead, if we want to update the ``popularity``: + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=[Band.popularity] + ... ) + +If we fetch the data from the database, we'll see that it was updated: + +.. code-block:: python + + >>> await Band.select().where(Band.name == "Pythonistas").first() + {'id': 1, 'name': 'Pythonistas', 'popularity': 1200} + +``target`` +---------- + +Using the ``target`` argument, we can specify which constraint we're concerned +with. By specifying ``target=Band.name`` we're only concerned with the unique +constraint for the ``band`` column. If you omit the ``target`` argument, then +it works for all constraints on the table. + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING", + ... target=Band.name + ... ) + +If you want to target a composite unique constraint, you can do so by passing +in a tuple of columns: + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING", + ... target=(Band.name, Band.popularity) + ... ) + +You can also specify the name of a constraint using a string: + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO NOTHING", + ... target='some_constraint' + ... ) + +``values`` +---------- + +This lets us specify which values to update when a conflict occurs. + +By specifying a :class:`Column `, this means that +the new value for that column will be used: + +.. code-block:: python + :emphasize-lines: 6 + + # The new popularity will be 1200. + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=[Band.popularity] + ... ) + +Instead, we can specify a custom value using a tuple: + +.. code-block:: python + :emphasize-lines: 6 + + # The new popularity will be 1111. + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=[(Band.popularity, 1111)] + ... ) + +If we want to update all of the values, we can use :meth:`all_columns`. + +.. code-block:: python + :emphasize-lines: 5 + + >>> await Band.insert( + ... Band(id=1, name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=Band.all_columns() + ... ) + +``where`` +--------- + +This can be used with ``DO UPDATE``. It gives us more control over whether the +update should be made: + +.. code-block:: python + :emphasize-lines: 6 + + >>> await Band.insert( + ... Band(id=1, name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... values=[Band.popularity], + ... where=Band.popularity < 1000 + ... ) + +Multiple ``on_conflict`` clauses +-------------------------------- + +SQLite allows you to specify multiple ``ON CONFLICT`` clauses, but Postgres and +Cockroach don't. + +.. code-block:: python + + >>> await Band.insert( + ... Band(name="Pythonistas", popularity=1200) + ... ).on_conflict( + ... action="DO UPDATE", + ... ... + ... ).on_conflict( + ... action="DO NOTHING", + ... ... + ... ) + +Learn more +---------- + +* `Postgres docs `_ +* `Cockroach docs `_ +* `SQLite docs `_ + +Source +------ + +.. currentmodule:: piccolo.query.methods.insert + +.. automethod:: Insert.on_conflict + +.. autoclass:: OnConflictAction + :members: + :undoc-members: diff --git a/docs/src/piccolo/query_types/insert/bands.csv b/docs/src/piccolo/query_clauses/on_conflict/bands.csv similarity index 100% rename from docs/src/piccolo/query_types/insert/bands.csv rename to docs/src/piccolo/query_clauses/on_conflict/bands.csv diff --git a/docs/src/piccolo/query_types/insert.rst b/docs/src/piccolo/query_types/insert.rst index 110464496..f8d1de007 100644 --- a/docs/src/piccolo/query_types/insert.rst +++ b/docs/src/piccolo/query_types/insert.rst @@ -34,208 +34,14 @@ If we later decide to insert additional rows, we can use the ``add`` method: ------------------------------------------------------------------------------- -``on_conflict`` ---------------- - -.. hint:: This is an advanced topic, and first time learners of Piccolo - can skip if they want. - - -When inserting a row, if a constraint fails (for example, a unique constraint), -then the insertion fails. - -Using the ``on_conflict`` clause, we can instead tell the database to ignore -the error (using ``DO NOTHING``), or to update the row (using ``DO UPDATE``). - -This is sometimes called an **upsert** (update if it already exists else insert). - -Example data -~~~~~~~~~~~~ - -If we have the following table: - -.. code-block:: python - - class Band(Table): - name = Varchar(unique=True) - popularity = Integer() - -With this data: - -.. csv-table:: - :file: ./insert/bands.csv - -Let's try inserting another row with the same ``name``, and we'll get an error: - -.. code-block:: python - - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ) - Unique constraint error! - -``DO NOTHING`` -~~~~~~~~~~~~~~ - -To ignore the error: - -.. code-block:: python - - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO NOTHING" - ... ) - -If we fetch the data from the database, we'll see that it hasn't changed: - -.. code-block:: python - - >>> await Band.select().where(Band.name == "Pythonistas").first() - {'id': 1, 'name': 'Pythonistas', 'popularity': 1000} - - -``DO UPDATE`` -~~~~~~~~~~~~~ - -Instead, if we want to update the ``popularity``: - -.. code-block:: python - - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO UPDATE", - ... values=[Band.popularity] - ... ) - -If we fetch the data from the database, we'll see that it was updated: - -.. code-block:: python - - >>> await Band.select().where(Band.name == "Pythonistas").first() - {'id': 1, 'name': 'Pythonistas', 'popularity': 1200} - -``target`` -~~~~~~~~~~ - -Using the ``target`` argument, we can specify which constraint we're concerned -with. By specifying ``target=Band.name`` we're only concerned with unique -constraint for the ``band`` column. If you omit the ``target`` argument, then -it works for all constraints on the table. - -.. code-block:: python - :emphasize-lines: 5 - - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO NOTHING", - ... target=Band.name - ... ) - -If you want to target a composite unique constraint, you can do so by passing -in a tuple of columns: - -.. code-block:: python - :emphasize-lines: 5 - - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO NOTHING", - ... target=(Band.name, Band.popularity) - ... ) - -You can also specify the name of a constraint using a string: - -.. code-block:: python - :emphasize-lines: 5 - - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO NOTHING", - ... target='some_constraint' - ... ) - -``values`` -~~~~~~~~~~ - -This lets us specify which values to update when a conflict occurs. - -By specifying a :class:`Column `, this means that -the new value for that column will be used: - -.. code-block:: python - :emphasize-lines: 6 - - # The new popularity will be 1200. - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO UPDATE", - ... values=[Band.popularity] - ... ) - -Instead, we can specify a custom value using a tuple: - -.. code-block:: python - :emphasize-lines: 6 - - # The new popularity will be 1111. - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO UPDATE", - ... values=[(Band.popularity, 1111)] - ... ) - -If we want to update all of the values, we can use :meth:`all_columns`. - -.. code-block:: python - :emphasize-lines: 5 - - >>> await Band.insert( - ... Band(id=1, name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO UPDATE", - ... values=Band.all_columns() - ... ) - -Multiple ``on_conflict`` clauses -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -SQLite allows you to specify multiple ``ON CONFLICT`` clauses, but Postgres and -Cockroach don't. - -.. code-block:: python - - >>> await Band.insert( - ... Band(name="Pythonistas", popularity=1200) - ... ).on_conflict( - ... action="DO UPDATE", - ... ... - ... ).on_conflict( - ... action="DO NOTHING", - ... ... - ) - -Source -~~~~~~ - -.. currentmodule:: piccolo.query.methods.insert - -.. automethod:: Insert.on_conflict +Query clauses +------------- -.. autoclass:: OnConflictAction - :members: - :undoc-members: +on_conflict +~~~~~~~~~~~ -------------------------------------------------------------------------------- +See :ref:`On_Conflict`. -Query clauses -------------- returning ~~~~~~~~~ From 8c4a99c2abb8b8a29e59d8e81ba319843b466d14 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Wed, 3 May 2023 16:38:57 +0100 Subject: [PATCH 31/31] refactor the `where` clause --- piccolo/query/methods/insert.py | 5 ++++- piccolo/query/mixins.py | 19 ++++++++++++------- tests/table/test_insert.py | 19 ++++++++++++++++++- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/piccolo/query/methods/insert.py b/piccolo/query/methods/insert.py index fa68d92cf..283bdde0b 100644 --- a/piccolo/query/methods/insert.py +++ b/piccolo/query/methods/insert.py @@ -74,7 +74,10 @@ def on_conflict( ) self.on_conflict_delegate.on_conflict( - target=target, where=where, action=action, values=values + target=target, + action=action, + values=values, + where=where, ) return self diff --git a/piccolo/query/mixins.py b/piccolo/query/mixins.py index 4637221fa..43d126436 100644 --- a/piccolo/query/mixins.py +++ b/piccolo/query/mixins.py @@ -640,11 +640,11 @@ class OnConflictAction(str, Enum): @dataclass class OnConflictItem: target: t.Optional[t.Union[str, Column, t.Tuple[Column, ...]]] = None - where: t.Optional[Combinable] = None action: t.Optional[OnConflictAction] = None values: t.Optional[ t.Sequence[t.Union[Column, t.Tuple[Column, t.Any]]] ] = None + where: t.Optional[Combinable] = None @property def target_string(self) -> str: @@ -707,14 +707,14 @@ def querystring(self) -> QueryString: if self.target: query += f" {self.target_string}" - if self.where: - query += " WHERE {}" - values.append(self.where.querystring) - if self.action: query += " {}" values.append(self.action_string) + if self.where: + query += " WHERE {}" + values.append(self.where.querystring) + return QueryString(query, *values) def __str__(self) -> str: @@ -760,13 +760,13 @@ class OnConflictDelegate: def on_conflict( self, target: t.Optional[t.Union[str, Column, t.Tuple[Column, ...]]] = None, - where: t.Optional[Combinable] = None, action: t.Union[ OnConflictAction, Literal["DO NOTHING", "DO UPDATE"] ] = OnConflictAction.do_nothing, values: t.Optional[ t.Sequence[t.Union[Column, t.Tuple[Column, t.Any]]] ] = None, + where: t.Optional[Combinable] = None, ): action_: OnConflictAction if isinstance(action, OnConflictAction): @@ -776,8 +776,13 @@ def on_conflict( else: raise ValueError("Unrecognised `on conflict` action.") + if where and action_ == OnConflictAction.do_nothing: + raise ValueError( + "The `where` option can only be used with DO NOTHING." + ) + self._on_conflict.on_conflict_items.append( OnConflictItem( - target=target, where=where, action=action_, values=values + target=target, action=action_, values=values, where=where ) ) diff --git a/tests/table/test_insert.py b/tests/table/test_insert.py index 6b576f5c3..1c5fab732 100644 --- a/tests/table/test_insert.py +++ b/tests/table/test_insert.py @@ -284,9 +284,9 @@ def test_where(self): Band(name=self.band.name, popularity=new_popularity) ).on_conflict( target=Band.name, - where=Band.popularity < self.band.popularity, action="DO UPDATE", values=[Band.popularity], + where=Band.popularity < self.band.popularity, ) self.assertIn( @@ -296,6 +296,23 @@ def test_where(self): query.run_sync() + def test_do_nothing_where(self): + """ + Make sure an error is raised if `where` is used with `DO NOTHING`. + """ + Band = self.Band + + with self.assertRaises(ValueError) as manager: + Band.insert(Band()).on_conflict( + action="DO NOTHING", + where=Band.popularity < self.band.popularity, + ) + + self.assertEqual( + manager.exception.__str__(), + "The `where` option can only be used with DO NOTHING.", + ) + def test_do_nothing(self): """ Make sure that `DO NOTHING` works.