Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #35336 -- Addressed crash when adding a GeneratedField with % literals. #18027

Merged
merged 1 commit into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Member Author

@charettes charettes Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crux of the issue here is that

  1. This part was never % escaped (% -> %%) which mean that things were working properly during model creation unless the model included a database default which resulted in table_sql returning params and then interpolation kicked (see the params or None in create_model). As for add_field it systematically crashed because a list was always passed to execute(params) which triggered interpolation which is what is what was reported on ticket-35336.
  2. Postgres Some backends simply don't support parametrized DD so we should likely have a generic feature flag for it and do the quote_value dance when it's not the case in a single location. That would require changing the Constraint interfaces though as it currently return already interpolated SQL under some circumstances.


@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)
charettes marked this conversation as resolved.
Show resolved Hide resolved

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