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", + )