fix(django-spanner): resolve review feedback for returning_columns and lookups parameter indexing

Author: sakthivelmanii

packages/django-google-spanner/django_spanner/lookups.py

@@ -47,14 +47,15 @@ def contains(self, compiler, connection):
     params = list(lhs_params)
     params.extend(rhs_params)
     is_icontains = self.lookup_name.startswith("i")
-    if self.rhs_is_direct_value() and params and not self.bilateral_transforms:
+    if self.rhs_is_direct_value() and rhs_params and not self.bilateral_transforms:
         rhs_sql = self.get_rhs_op(connection, rhs_sql)
+        rhs_idx = len(lhs_params)
         # Chop the leading and trailing percent signs that Django adds to the
         # param since this isn't a LIKE query as Django expects.
-        params[0] = params[0][1:-1]
+        params[rhs_idx] = params[rhs_idx][1:-1]
         # Add the case insensitive flag for icontains.
         if is_icontains:
-            params[0] = "(?i)" + params[0]
+            params[rhs_idx] = "(?i)" + params[rhs_idx]
         # rhs_sql is REGEXP_CONTAINS(%s, %%s), and lhs_sql is the column name.
         return rhs_sql % lhs_sql, tuple(params)
     else:
@@ -96,8 +97,9 @@ def iexact(self, compiler, connection):
     params.extend(rhs_params)
     rhs_sql = self.get_rhs_op(connection, rhs_sql)
     # Wrap the parameter in ^ and $ to restrict the regex to an exact match.
-    if self.rhs_is_direct_value() and params and not self.bilateral_transforms:
-        params[0] = "^(?i)%s$" % params[0]
+    if self.rhs_is_direct_value() and rhs_params and not self.bilateral_transforms:
+        rhs_idx = len(lhs_params)
+        params[rhs_idx] = "^(?i)%s$" % params[rhs_idx]
     else:
         # lhs_sql is the expression/column to use as the regular expression.
         # Use concat to make the value case-insensitive.
@@ -143,12 +145,13 @@ def regex(self, compiler, connection):
     params = list(lhs_params)
     params.extend(rhs_params)
     is_iregex = self.lookup_name.startswith("i")
-    if self.rhs_is_direct_value() and params and not self.bilateral_transforms:
+    if self.rhs_is_direct_value() and rhs_params and not self.bilateral_transforms:
         rhs_sql = self.get_rhs_op(connection, rhs_sql)
+        rhs_idx = len(lhs_params)
         if is_iregex:
-            params[0] = "(?i)%s" % params[0]
+            params[rhs_idx] = "(?i)%s" % params[rhs_idx]
         else:
-            params[0] = str(params[0])
+            params[rhs_idx] = str(params[rhs_idx])
         # rhs_sql is REGEXP_CONTAINS(%s, %%s), and lhs_sql is the column name.
         return rhs_sql % lhs_sql, tuple(params)
     else:
@@ -191,15 +194,16 @@ def startswith_endswith(self, compiler, connection):
     is_insensitive = self.lookup_name.startswith("i")
     # Chop the leading (endswith) or trailing (startswith) percent sign that
     # Django adds to the param since this isn't a LIKE query as Django expects.
-    if self.rhs_is_direct_value() and params and not self.bilateral_transforms:
+    if self.rhs_is_direct_value() and rhs_params and not self.bilateral_transforms:
         rhs_sql = self.get_rhs_op(connection, rhs_sql)
+        rhs_idx = len(lhs_params)
         if is_endswith:
-            params[0] = str(params[0][1:]) + "$"
+            params[rhs_idx] = str(params[rhs_idx][1:]) + "$"
         else:
-            params[0] = "^" + str(params[0][:-1])
+            params[rhs_idx] = "^" + str(params[rhs_idx][:-1])
         # Add the case insensitive flag for istartswith or iendswith.
         if is_insensitive:
-            params[0] = "(?i)" + params[0]
+            params[rhs_idx] = "(?i)" + params[rhs_idx]
         # rhs_sql is REGEXP_CONTAINS(%s, %%s), and lhs_sql is the column name.
         return rhs_sql % lhs_sql, tuple(params)
     else:

packages/django-google-spanner/django_spanner/operations.py

@@ -46,7 +46,9 @@ class DatabaseOperations(BaseDatabaseOperations):
     def returning_columns(self, fields):
         if not fields:
             return "", ()
-        columns = [self.quote_name(field.column) for field in fields]
+        columns = [
+            self.quote_name(getattr(field, "column", str(field))) for field in fields
+        ]
         return "THEN RETURN %s" % ", ".join(columns), ()
 
     # In Django <= 5.2, this method was named return_insert_columns

packages/django-google-spanner/tests/unit/django_spanner/test_lookups.py

@@ -6,7 +6,8 @@
 
 from decimal import Decimal
 
-from django.db.models import F
+from django.db.models import F, Value
+from django.db.models.functions import Concat
 
 from django_spanner.compiler import SQLCompiler
 from tests.unit.django_spanner.simple_test import SpannerSimpleTestClass
@@ -271,3 +272,53 @@ def test_iexact_sql_query_case_insensitive_value_match(self):
         )
         self.assertEqual(sql_compiled, expected_sql)
         self.assertEqual(params, ("abc",))
+
+    def test_icontains_with_lhs_params(self):
+        qs = (
+            Author.objects.annotate(greeting=Concat(Value("User: "), "name"))
+            .filter(greeting__icontains="john")
+            .values("name")
+        )
+        compiler = SQLCompiler(qs.query, self.connection, "default")
+        sql, params = compiler.as_sql()
+        self.assertEqual(params, ("User: ", "", "", "(?i)john"))
+
+    def test_iexact_with_lhs_params(self):
+        qs = (
+            Author.objects.annotate(greeting=Concat(Value("User: "), "name"))
+            .filter(greeting__iexact="john")
+            .values("name")
+        )
+        compiler = SQLCompiler(qs.query, self.connection, "default")
+        sql, params = compiler.as_sql()
+        self.assertEqual(params, ("User: ", "", "", "^(?i)john$"))
+
+    def test_istartswith_with_lhs_params(self):
+        qs = (
+            Author.objects.annotate(greeting=Concat(Value("User: "), "name"))
+            .filter(greeting__istartswith="john")
+            .values("name")
+        )
+        compiler = SQLCompiler(qs.query, self.connection, "default")
+        sql, params = compiler.as_sql()
+        self.assertEqual(params, ("User: ", "", "", "(?i)^john"))
+
+    def test_iendswith_with_lhs_params(self):
+        qs = (
+            Author.objects.annotate(greeting=Concat(Value("User: "), "name"))
+            .filter(greeting__iendswith="john")
+            .values("name")
+        )
+        compiler = SQLCompiler(qs.query, self.connection, "default")
+        sql, params = compiler.as_sql()
+        self.assertEqual(params, ("User: ", "", "", "(?i)john$"))
+
+    def test_iregex_with_lhs_params(self):
+        qs = (
+            Author.objects.annotate(greeting=Concat(Value("User: "), "name"))
+            .filter(greeting__iregex="^john")
+            .values("name")
+        )
+        compiler = SQLCompiler(qs.query, self.connection, "default")
+        sql, params = compiler.as_sql()
+        self.assertEqual(params, ("User: ", "", "", "(?i)^john"))

packages/django-google-spanner/tests/unit/django_spanner/test_operations.py

@@ -275,6 +275,11 @@ def test_returning_columns(self):
         self.assertEqual(sql, "THEN RETURN id, name")
         self.assertEqual(params, ())
 
+    def test_returning_columns_with_strings(self):
+        sql, params = self.db_operations.returning_columns(["id", "created_at"])
+        self.assertEqual(sql, "THEN RETURN id, created_at")
+        self.assertEqual(params, ())
+
     def test_returning_columns_empty(self):
         sql, params = self.db_operations.returning_columns([])
         self.assertEqual(sql, "")