Skip to content

Commit

Permalink
Fixed #35336 -- Addressed crash when adding a GeneratedField with % l…
Browse files Browse the repository at this point in the history
…iterals.

A longer term solution is likely to have a better separation of parametrized
DDL altogether to handle checks, constraints, defaults, and generated fields
but such a change would require a significant refactor that isn't suitable
for a backport.

Thanks Adrian Garcia for the report.
  • Loading branch information
charettes committed Apr 2, 2024
1 parent 5f18021 commit 888b904
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 13 deletions.
23 changes: 14 additions & 9 deletions django/db/backends/base/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,9 @@ def _iter_column_sql(
if default_value is not None:
column_default = "DEFAULT " + self._column_default_sql(field)
if self.connection.features.requires_literal_defaults:
# Some databases can't take defaults as a parameter (Oracle).
# If this is the case, the individual schema backend should
# implement prepare_default().
# Some databases can't take defaults as a parameter
# (Oracle, SQLite). If this is the case, the individual
# schema backend should implement prepare_default().
yield column_default % self.prepare_default(default_value)
else:
yield column_default
Expand All @@ -333,7 +333,9 @@ def _iter_column_sql(
):
null = True
if field.generated:
yield self._column_generated_sql(field)
generated_sql, generated_params = self._column_generated_sql(field)
params.extend(generated_params)
yield generated_sql
elif not null:
yield "NOT NULL"
elif not self.connection.features.implied_column_null:
Expand Down Expand Up @@ -420,7 +422,7 @@ def db_default_sql(self, field):
compiler = query.get_compiler(connection=self.connection)
default_sql, params = compiler.compile(db_default)
if self.connection.features.requires_literal_defaults:
# Some databases doesn't support parameterized defaults (Oracle,
# Some databases don't support parameterized defaults (Oracle,
# SQLite). If this is the case, the individual schema backend
# should implement prepare_default().
default_sql %= tuple(self.prepare_default(p) for p in params)
Expand All @@ -431,9 +433,10 @@ def _column_generated_sql(self, field):
"""Return the SQL to use in a GENERATED ALWAYS clause."""
expression_sql, params = field.generated_sql(self.connection)
persistency_sql = "STORED" if field.db_persist else "VIRTUAL"
if params:
if self.connection.features.requires_literal_defaults:
expression_sql = expression_sql % tuple(self.quote_value(p) for p in params)
return f"GENERATED ALWAYS AS ({expression_sql}) {persistency_sql}"
params = ()
return f"GENERATED ALWAYS AS ({expression_sql}) {persistency_sql}", params

@staticmethod
def _effective_default(field):
Expand Down Expand Up @@ -484,7 +487,7 @@ def create_model(self, model):
"""
sql, params = self.table_sql(model)
# Prevent using [] as params, in the case a literal '%' is used in the
# definition.
# definition on backends that don't support parametrized DDL.
self.execute(sql, params or None)

if self.connection.features.supports_comments:
Expand Down Expand Up @@ -746,7 +749,9 @@ def add_field(self, model, field):
"column": self.quote_name(field.column),
"definition": definition,
}
self.execute(sql, params)
# Prevent using [] as params, in the case a literal '%' is used in the
# definition on backends that don't support parametrized DDL.
self.execute(sql, params or None)
# Drop the default if we need to
if (
field.db_default is NOT_PROVIDED
Expand Down
4 changes: 1 addition & 3 deletions django/db/backends/oracle/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,7 @@ def _generate_temp_name(self, for_name):
return self.normalize_name(for_name + "_" + suffix)

def prepare_default(self, value):
# Replace % with %% as %-formatting is applied in
# FormatStylePlaceholderCursor._fix_for_params().
return self.quote_value(value).replace("%", "%%")
return self.quote_value(value)

def _field_should_be_indexed(self, model, field):
create_index = super()._field_should_be_indexed(model, field)
Expand Down
4 changes: 4 additions & 0 deletions docs/releases/5.0.4.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,7 @@ Bugfixes

* Fixed a crash in Django 5.0 when performing queries involving table aliases
and lookups on a ``GeneratedField`` of the aliased table (:ticket:`35344`).

* Fixed a bug in Django 5.0 that caused a migration crash when adding a
``GeneratedField`` relying on the ``__contains`` or ``__icontains``
lookups or using a ``Value`` containing a ``"%"`` (:ticket:`35336`).
44 changes: 43 additions & 1 deletion tests/schema/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,16 @@
Value,
)
from django.db.models.fields.json import KT, KeyTextTransform
from django.db.models.functions import Abs, Cast, Collate, Lower, Random, Round, Upper
from django.db.models.functions import (
Abs,
Cast,
Collate,
Concat,
Lower,
Random,
Round,
Upper,
)
from django.db.models.indexes import IndexExpression
from django.db.transaction import TransactionManagementError, atomic
from django.test import TransactionTestCase, skipIfDBFeature, skipUnlessDBFeature
Expand Down Expand Up @@ -886,6 +895,39 @@ class Meta:
with connection.schema_editor() as editor:
editor.create_model(GeneratedFieldOutputFieldModel)

@isolate_apps("schema")
@skipUnlessDBFeature("supports_stored_generated_columns")
def test_add_generated_field_contains(self):
class GeneratedFieldContainsModel(Model):
text = TextField(default="foo")
generated = GeneratedField(
expression=Concat("text", Value("%")),
db_persist=True,
output_field=TextField(),
)

class Meta:
app_label = "schema"

with connection.schema_editor() as editor:
editor.create_model(GeneratedFieldContainsModel)

field = GeneratedField(
expression=Q(text__icontains="FOO"),
db_persist=True,
output_field=BooleanField(),
)
field.contribute_to_class(GeneratedFieldContainsModel, "contains_foo")

with connection.schema_editor() as editor:
editor.add_field(GeneratedFieldContainsModel, field)

obj = GeneratedFieldContainsModel.objects.create()
obj.refresh_from_db()
self.assertEqual(obj.text, "foo")
self.assertEqual(obj.generated, "foo%")
self.assertIs(obj.contains_foo, True)

@isolate_apps("schema")
def test_add_auto_field(self):
class AddAutoFieldModel(Model):
Expand Down

0 comments on commit 888b904

Please sign in to comment.