From a6907571e10a0d9b841b54dd87ea270eb846a85b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Tue, 10 Nov 2026 10:39:34 +0100 Subject: [PATCH] chore(spanner-django): add WHERE 1=1 to unqualified DELETE and UPDATE Cloud Spanner requires a WHERE clause for all DELETE and UPDATE statements to prevent accidental table-wide data modifications. Previously, unqualified queries and schema migration backfills relied on the low-level DB-API driver to rewrite SQL strings and append "WHERE 1=1". The Django backend now generates this clause directly during query compilation and migration execution. Ensuring that Django generates compliant SQL on its own allows us to safely remove automatic SQL string rewriting from the DB-API driver in a future release without breaking Django applications. --- .../django_spanner/compiler.py | 14 ++- .../django_spanner/schema.py | 2 +- .../django_spanner/utils.py | 7 +- .../tests/mockserver_tests/test_basics.py | 43 ++++++++ .../unit/django_spanner/test_compiler.py | 99 ++++++++++++++++++- .../tests/unit/django_spanner/test_schema.py | 14 +++ .../tests/unit/django_spanner/test_utils.py | 28 ++++++ 7 files changed, 199 insertions(+), 8 deletions(-) diff --git a/packages/django-google-spanner/django_spanner/compiler.py b/packages/django-google-spanner/django_spanner/compiler.py index 9921dd252cdf..6a01a37f0483 100644 --- a/packages/django-google-spanner/django_spanner/compiler.py +++ b/packages/django-google-spanner/django_spanner/compiler.py @@ -22,6 +22,8 @@ ) from django.db.utils import DatabaseError +from django_spanner.utils import add_dummy_where + class SQLCompiler(BaseSQLCompiler): """ @@ -136,15 +138,19 @@ class SQLInsertCompiler(BaseSQLInsertCompiler, SQLCompiler): class SQLDeleteCompiler(BaseSQLDeleteCompiler, SQLCompiler): - """A wrapper class for compatibility with Django specifications.""" + """A variation of the Django SQL delete compiler for Spanner.""" - pass + def as_sql(self): + sql, parameters = super().as_sql() + return (add_dummy_where(sql), parameters) if sql else (sql, parameters) class SQLUpdateCompiler(BaseSQLUpdateCompiler, SQLCompiler): - """A wrapper class for compatibility with Django specifications.""" + """A variation of the Django SQL update compiler for Spanner.""" - pass + def as_sql(self): + sql, parameters = super().as_sql() + return (add_dummy_where(sql), parameters) if sql else (sql, parameters) class SQLAggregateCompiler(BaseSQLAggregateCompiler, SQLCompiler): diff --git a/packages/django-google-spanner/django_spanner/schema.py b/packages/django-google-spanner/django_spanner/schema.py index 15c5b3ce1816..3ed8d8359777 100644 --- a/packages/django-google-spanner/django_spanner/schema.py +++ b/packages/django-google-spanner/django_spanner/schema.py @@ -265,7 +265,7 @@ def add_field(self, model, field): effective_default = self.effective_default(field) if effective_default is not None: self.execute( - "UPDATE %(table)s SET %(column)s=%%s" + "UPDATE %(table)s SET %(column)s=%%s WHERE 1=1" % { "table": self.quote_name(model._meta.db_table), "column": self.quote_name(field.column), diff --git a/packages/django-google-spanner/django_spanner/utils.py b/packages/django-google-spanner/django_spanner/utils.py index 877de815b057..163b29051625 100644 --- a/packages/django-google-spanner/django_spanner/utils.py +++ b/packages/django-google-spanner/django_spanner/utils.py @@ -37,6 +37,11 @@ def add_dummy_where(sql): :rtype: str :returns: A SQL statement with dummy WHERE clause. """ - if any(isinstance(token, sqlparse.sql.Where) for token in sqlparse.parse(sql)[0]): + if not sql or not sql.strip(): + return sql + if "where" not in sql.lower(): + return sql + " WHERE 1=1" + parsed = sqlparse.parse(sql) + if not parsed or any(isinstance(token, sqlparse.sql.Where) for token in parsed[0]): return sql return sql + " WHERE 1=1" diff --git a/packages/django-google-spanner/tests/mockserver_tests/test_basics.py b/packages/django-google-spanner/tests/mockserver_tests/test_basics.py index 209f831c704c..f435cad54b53 100644 --- a/packages/django-google-spanner/tests/mockserver_tests/test_basics.py +++ b/packages/django-google-spanner/tests/mockserver_tests/test_basics.py @@ -11,6 +11,8 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from unittest import mock + from django.db import connection, models from google.cloud.spanner_v1 import ( BatchCreateSessionsRequest, @@ -25,6 +27,7 @@ add_select1_result, add_singer_query_result, add_single_result, + add_update_count, ) from tests.mockserver_tests.models import Singer from tests.settings import DATABASES @@ -140,3 +143,43 @@ class LocalSinger(models.Model): for db, config in DATABASES.items(): if config["ENGINE"] == "django_spanner": config.pop("DISABLE_RANDOM_ID_GENERATION", None) + + def test_update_all_without_dbapi_intervention(self): + """Verify Django generates WHERE 1=1 even when DB-API injection is disabled.""" + with mock.patch( + "google.cloud.spanner_dbapi.parse_utils.ensure_where_clause", + side_effect=lambda sql: sql, + ): + add_update_count("UPDATE tests_singer SET last_name = @a0 WHERE 1=1", 1) + Singer.objects.all().update(last_name="Test") + + requests = [ + request + for request in self.spanner_service.requests + if isinstance(request, ExecuteSqlRequest) + ] + self.assertEqual(len(requests), 1) + self.assertEqual( + requests[0].sql, + "UPDATE tests_singer SET last_name = @a0 WHERE 1=1", + ) + + def test_delete_all_without_dbapi_intervention(self): + """Verify Django generates WHERE 1=1 even when DB-API injection is disabled.""" + with mock.patch( + "google.cloud.spanner_dbapi.parse_utils.ensure_where_clause", + side_effect=lambda sql: sql, + ): + add_update_count("DELETE FROM tests_singer WHERE 1=1", 1) + Singer.objects.all().delete() + + requests = [ + request + for request in self.spanner_service.requests + if isinstance(request, ExecuteSqlRequest) + ] + self.assertEqual(len(requests), 1) + self.assertEqual( + requests[0].sql, + "DELETE FROM tests_singer WHERE 1=1", + ) diff --git a/packages/django-google-spanner/tests/unit/django_spanner/test_compiler.py b/packages/django-google-spanner/tests/unit/django_spanner/test_compiler.py index 328dee8b1fbf..4a618a071832 100644 --- a/packages/django-google-spanner/tests/unit/django_spanner/test_compiler.py +++ b/packages/django-google-spanner/tests/unit/django_spanner/test_compiler.py @@ -5,13 +5,19 @@ # https://developers.google.com/open-source/licenses/bsd from django.core.exceptions import EmptyResultSet +from django.db.models import F, Q, Subquery from django.db.models.query import QuerySet +from django.db.models.sql import DeleteQuery, UpdateQuery from django.db.utils import DatabaseError -from django_spanner.compiler import SQLCompiler +from django_spanner.compiler import ( + SQLCompiler, + SQLDeleteCompiler, + SQLUpdateCompiler, +) from tests.unit.django_spanner.simple_test import SpannerSimpleTestClass -from .models import Number +from .models import Item, Number class TestCompiler(SpannerSimpleTestClass): @@ -173,3 +179,92 @@ def test_get_combinator_sql_empty_queryset_raises_exception(self): compiler = SQLCompiler(QuerySet().query, self.connection, "default") with self.assertRaises(EmptyResultSet): compiler.get_combinator_sql("union", False) + + def test_unqualified_delete_appends_dummy_where(self): + """Test that unqualified DELETE statements automatically append WHERE 1=1.""" + delete_query = Number.objects.all().query.chain(DeleteQuery) + compiler = SQLDeleteCompiler(delete_query, self.connection, "default") + sql, _ = compiler.as_sql() + self.assertEqual(sql, "DELETE FROM tests_number WHERE 1=1") + + def test_qualified_delete_preserves_where(self): + """Test that qualified DELETE statements do not append extra WHERE 1=1.""" + delete_query = Number.objects.filter(num=1).query.chain(DeleteQuery) + compiler = SQLDeleteCompiler(delete_query, self.connection, "default") + sql, parameters = compiler.as_sql() + self.assertEqual(sql, "DELETE FROM tests_number WHERE tests_number.num = %s") + self.assertEqual(parameters, (1,)) + + def test_unqualified_update_appends_dummy_where(self): + """Test that unqualified UPDATE statements automatically append WHERE 1=1.""" + update_query = Number.objects.all().query.chain(UpdateQuery) + update_query.add_update_values({"num": 5}) + compiler = SQLUpdateCompiler(update_query, self.connection, "default") + sql, _ = compiler.as_sql() + self.assertEqual(sql, "UPDATE tests_number SET num = %s WHERE 1=1") + + def test_qualified_update_preserves_where(self): + """Test that qualified UPDATE statements do not append extra WHERE 1=1.""" + update_query = Number.objects.filter(num=1).query.chain(UpdateQuery) + update_query.add_update_values({"num": 5}) + compiler = SQLUpdateCompiler(update_query, self.connection, "default") + sql, parameters = compiler.as_sql() + self.assertEqual( + sql, "UPDATE tests_number SET num = %s WHERE tests_number.num = %s" + ) + self.assertEqual(parameters, (5, 1)) + + def test_full_result_set_delete_appends_dummy_where(self): + """Test that DELETE raising FullResultSet automatically appends WHERE 1=1.""" + delete_query = Number.objects.filter(~Q(num__in=[])).query.chain(DeleteQuery) + compiler = SQLDeleteCompiler(delete_query, self.connection, "default") + sql, _ = compiler.as_sql() + self.assertEqual(sql, "DELETE FROM tests_number WHERE 1=1") + + def test_full_result_set_update_appends_dummy_where(self): + """Test that UPDATE raising FullResultSet automatically appends WHERE 1=1.""" + update_query = Number.objects.filter(~Q(num__in=[])).query.chain(UpdateQuery) + update_query.add_update_values({"num": 5}) + compiler = SQLUpdateCompiler(update_query, self.connection, "default") + sql, _ = compiler.as_sql() + self.assertEqual(sql, "UPDATE tests_number SET num = %s WHERE 1=1") + + def test_joined_delete_does_not_duplicate_where(self): + """Test that DELETE with joins/annotations retains single WHERE clause.""" + delete_query = Number.objects.annotate(item_name=F("item__name")).query.chain( + DeleteQuery + ) + compiler = SQLDeleteCompiler(delete_query, self.connection, "default") + sql, _ = compiler.as_sql() + self.assertIn(" WHERE ", sql) + self.assertEqual(sql.count(" WHERE "), 1) + self.assertNotIn("WHERE 1=1", sql) + + def test_update_without_values_returns_empty(self): + """Test that UPDATE without update values compiles to empty string.""" + update_query = Number.objects.all().query.chain(UpdateQuery) + compiler = SQLUpdateCompiler(update_query, self.connection, "default") + sql, parameters = compiler.as_sql() + self.assertEqual(sql, "") + self.assertEqual(parameters, ()) + + def test_select_does_not_append_dummy_where(self): + """Test that unqualified SELECT statements do not append WHERE 1=1.""" + compiler = SQLCompiler(Number.objects.all().query, self.connection, "default") + sql, _ = compiler.as_sql() + self.assertTrue(sql.startswith("SELECT ")) + self.assertNotIn("WHERE", sql) + + def test_update_with_subquery_does_not_add_where_to_subquery(self): + """Test that subqueries in UPDATE do not get dummy WHERE, but outer UPDATE does.""" + subquery = Item.objects.values("name")[:1] + update_query = Number.objects.all().query.chain(UpdateQuery) + update_query.add_update_values({"num": Subquery(subquery)}) + compiler = SQLUpdateCompiler(update_query, self.connection, "default") + sql, _ = compiler.as_sql() + self.assertIn(" WHERE 1=1", sql) + self.assertEqual(sql.count(" WHERE "), 1) + start_index = sql.index("(") + end_index = sql.index(")") + subquery_sql = sql[start_index:end_index] + self.assertNotIn("WHERE", subquery_sql) diff --git a/packages/django-google-spanner/tests/unit/django_spanner/test_schema.py b/packages/django-google-spanner/tests/unit/django_spanner/test_schema.py index 754baa60c735..ea4888e6f4a8 100644 --- a/packages/django-google-spanner/tests/unit/django_spanner/test_schema.py +++ b/packages/django-google-spanner/tests/unit/django_spanner/test_schema.py @@ -154,6 +154,20 @@ def test_add_field(self): "ALTER TABLE tests_author ADD COLUMN age INT64", [] ) + def test_add_field_with_default(self): + """Tests adding fields with default values executes UPDATE with WHERE 1=1.""" + with DatabaseSchemaEditor(self.connection) as schema_editor: + schema_editor.execute = mock.MagicMock() + new_field = IntegerField(default=42, null=True) + new_field.set_attributes_from_name("age") + new_field.model = Author + schema_editor.add_field(Author, new_field) + + schema_editor.execute.assert_any_call( + "UPDATE tests_author SET age=%s WHERE 1=1", + (42,), + ) + def test_remove_field(self): """ Tests remove fields from models diff --git a/packages/django-google-spanner/tests/unit/django_spanner/test_utils.py b/packages/django-google-spanner/tests/unit/django_spanner/test_utils.py index 341eae3e4161..6b48f07e11d5 100644 --- a/packages/django-google-spanner/tests/unit/django_spanner/test_utils.py +++ b/packages/django-google-spanner/tests/unit/django_spanner/test_utils.py @@ -50,3 +50,31 @@ def test_add_dummy_where_with_where_not_present_and_added(self): """ updated_sql = add_dummy_where(self.SQL_WITHOUT_WHERE) self.assertEqual(updated_sql, self.SQL_WITH_WHERE) + + def test_add_dummy_where_idempotent(self): + """Test that repeated calls to add_dummy_where do not duplicate WHERE clause.""" + sql = "DELETE FROM t" + first_pass = add_dummy_where(sql) + second_pass = add_dummy_where(first_pass) + self.assertEqual(first_pass, "DELETE FROM t WHERE 1=1") + self.assertEqual(second_pass, "DELETE FROM t WHERE 1=1") + + def test_add_dummy_where_empty_and_whitespace(self): + """Test that empty or whitespace input is returned unaltered.""" + self.assertEqual(add_dummy_where(""), "") + self.assertIsNone(add_dummy_where(None)) + self.assertEqual(add_dummy_where(" "), " ") + + def test_add_dummy_where_literal_where_in_statement(self): + """Test that SQL containing the word 'where' as identifier or literal is handled.""" + sql_with_literal = "UPDATE t SET note = 'where is it'" + self.assertEqual( + add_dummy_where(sql_with_literal), + "UPDATE t SET note = 'where is it' WHERE 1=1", + ) + + sql_with_identifier = "UPDATE t SET `where` = 1" + self.assertEqual( + add_dummy_where(sql_with_identifier), + "UPDATE t SET `where` = 1 WHERE 1=1", + )