diff --git a/docs/changelog.rst b/docs/changelog.rst index 2c7e2f4..3a36722 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -14,6 +14,7 @@ Unreleased - The ``sqlite-utils drop-table`` command now refuses to drop a view, and ``drop-view`` refuses to drop a table. Previously each would silently drop the wrong type of object if the name matched. Both now exit with an error suggesting the correct command to use. - Migrations applied by the new :ref:`migrations system ` now run inside a transaction, together with the record of the migration having been applied. If a migration raises an exception its changes are rolled back and it stays pending, so it can be safely re-applied after the error is fixed. Migrations that cannot run inside a transaction, such as those executing ``VACUUM``, can opt out using ``@migrations(transactional=False)`` - see :ref:`migrations_transactions`. - ``table.upsert()`` and ``table.upsert_all()`` now detect the primary key or compound primary key of an existing table, so the ``pk=`` argument is no longer required when upserting into a table that already has a primary key. +- ``table.upsert()`` and ``table.upsert_all()`` now raise ``PrimaryKeyRequired`` if a record is missing a value for any primary key column, or has a value of ``None`` for one. Previously such records - which can never match an existing row - were quietly inserted as brand new rows, or triggered a confusing ``KeyError`` after the insert had already taken place. - ``db.table(table_name).insert({})`` can now be used to insert a row consisting entirely of default values into an existing table, using ``INSERT INTO ... DEFAULT VALUES``. (:issue:`759`) .. _v4_0rc1: diff --git a/docs/python-api.rst b/docs/python-api.rst index 39361c9..ae2c0cb 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -1021,6 +1021,8 @@ Note that the ``pk`` and ``column_order`` parameters here are optional if you ar An ``upsert_all()`` method is also available, which behaves like ``insert_all()`` but performs upserts instead. +Every record passed to ``upsert()`` or ``upsert_all()`` must include a value for each primary key column - a record without one could never match an existing row, so a ``sqlite_utils.db.PrimaryKeyRequired`` exception is raised instead of quietly inserting a new row. + .. note:: ``.upsert()`` and ``.upsert_all()`` in sqlite-utils 1.x worked like ``.insert(..., replace=True)`` and ``.insert_all(..., replace=True)`` do in 2.x. See `issue #66 `__ for details of this change. diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 6f947e1..b4f5dcc 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -3199,6 +3199,11 @@ class Table(Queryable): # Dict-mode insert({}) has no explicit columns; SQLite spells that as # DEFAULT VALUES. List mode with no columns is a different input shape. if not list_mode and not all_columns: + if upsert: + raise PrimaryKeyRequired( + "upsert() requires a value for the primary key - " + "an empty record cannot be upserted" + ) or_clause = "" if replace: or_clause = " OR REPLACE" @@ -3286,6 +3291,26 @@ class Table(Queryable): # Everything from here on is for upsert=True pk_cols = [pk] if isinstance(pk, str) else list(pk) + # Every record must provide a value for every primary key column - a + # NULL primary key never matches ON CONFLICT, so the record would be + # inserted as a brand new row instead of upserted + missing_pk_cols = [c for c in pk_cols if c not in all_columns] + if missing_pk_cols: + raise PrimaryKeyRequired( + "upsert() requires a value for the primary key column{}: {}".format( + "s" if len(missing_pk_cols) > 1 else "", + ", ".join(missing_pk_cols), + ) + ) + pk_indexes = [all_columns.index(c) for c in pk_cols] + for record_values in values: + if any(record_values[i] is None for i in pk_indexes): + raise PrimaryKeyRequired( + "upsert() requires a value for the primary key column{}: {}".format( + "s" if len(pk_cols) > 1 else "", + ", ".join(pk_cols), + ) + ) non_pk_cols = [c for c in all_columns if c not in pk_cols] conflict_sql = ", ".join(quote_identifier(c) for c in pk_cols) diff --git a/tests/test_upsert.py b/tests/test_upsert.py index 09783bb..a782b26 100644 --- a/tests/test_upsert.py +++ b/tests/test_upsert.py @@ -49,6 +49,42 @@ def test_upsert_error_if_no_pk(fresh_db): table.upsert({"id": 1, "name": "Cleo"}) +@pytest.mark.parametrize("use_old_upsert", (False, True)) +def test_upsert_empty_record_errors(use_old_upsert): + db = Database(memory=True, use_old_upsert=use_old_upsert) + table = db["table"] + table.insert({"id": 1, "name": "Cleo"}, pk="id") + with pytest.raises(PrimaryKeyRequired): + table.upsert({}, pk="id") + with pytest.raises(PrimaryKeyRequired): + table.upsert_all([{}, {}], pk="id") + # No rows can have been inserted + assert table.count == 1 + + +@pytest.mark.parametrize("use_old_upsert", (False, True)) +def test_upsert_missing_pk_value_errors(use_old_upsert): + db = Database(memory=True, use_old_upsert=use_old_upsert) + table = db["table"] + table.insert({"id": 1, "name": "Cleo"}, pk="id") + # Records that omit the pk column entirely + with pytest.raises(PrimaryKeyRequired): + table.upsert_all([{"name": "Pancakes"}, {"name": "Marnie"}], pk="id") + # A record with an explicit None pk value can never conflict + with pytest.raises(PrimaryKeyRequired): + table.upsert({"id": None, "name": "Pancakes"}, pk="id") + assert list(table.rows) == [{"id": 1, "name": "Cleo"}] + + +def test_upsert_missing_compound_pk_value_errors(fresh_db): + table = fresh_db["table"] + table.insert({"a": "x", "b": "y", "v": 1}, pk=("a", "b")) + # Missing one component of the detected compound primary key + with pytest.raises(PrimaryKeyRequired): + table.upsert({"a": "x", "v": 2}) + assert list(table.rows) == [{"a": "x", "b": "y", "v": 1}] + + def test_upsert_error_if_existing_table_has_no_pk(fresh_db): table = fresh_db.create_table("table", {"id": int, "name": str}) with pytest.raises(PrimaryKeyRequired):