Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#31197 closed Bug (fixed)

CheckConstraints with F objects don't apply on SQLite.

Reported by: Adam Johnson Owned by: nobody
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Take this model with CheckConstraint:

class Book(models.Model):
    percent_read = models.PositiveIntegerField()
    percent_unread = models.PositiveIntegerField()
    percent_ignored = models.PositiveIntegerField()

    def __str__(self):
        return f"{self.id} - {self.percent_read}% read"

    class Meta:
        constraints = [
            models.CheckConstraint(
                check=models.Q(
                    percent_read=(
                        100
                        - models.F("percent_unread")
                        - models.F("percent_ignored")
                    )
                ),
                name="percentages_sum_100",
            )
        ]

Applying the migration to create the constraint works on MariaDB but fails on SQLite.

The sqlmigrate output on SQLite shows the F objects get resolved to the full table name, which is what breaks during renaming phase in the migration:

$ python manage.py sqlmigrate core 0002
BEGIN;
--
-- Create constraint percentages_sum_100 on model book
--
CREATE TABLE "new__core_book" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "percent_read" integer unsigned NOT NULL CHECK ("percent_read" >= 0), "percent_unread" integer unsigned NOT NULL CHECK ("percent_unread" >= 0), "percent_ignored" integer unsigned NOT NULL CHECK ("percent_ignored" >= 0), CONSTRAINT "percentages_sum_100" CHECK ("percent_read" = ((100 - "new__core_book"."percent_unread") - "new__core_book"."percent_ignored")));
INSERT INTO "new__core_book" ("id", "percent_read", "percent_unread", "percent_ignored") SELECT "id", "percent_read", "percent_unread", "percent_ignored" FROM "core_book";
DROP TABLE "core_book";
ALTER TABLE "new__core_book" RENAME TO "core_book";
COMMIT;
$ python manage.py migrate
...
django.db.utils.DatabaseError: malformed database schema (core_book) - no such column: new__core_book.percent_unread

Full repo for reproduction: https://github.com/adamchainz/django-issue-check-constraint-reference

Change History (5)

comment:1 by Adam Johnson, 4 years ago

If I apply this patch, it works:

diff --git django/db/models/expressions.py django/db/models/expressions.py
index 2b59dd301a..5ee3baf6f1 100644
--- django/db/models/expressions.py
+++ django/db/models/expressions.py
@@ -528,6 +528,8 @@ class F(Combinable):
 
     def resolve_expression(self, query=None, allow_joins=True, reuse=None,
                            summarize=False, for_save=False, simple_col=False):
+        # Override
+        simple_col = True
         return query.resolve_ref(self.name, allow_joins, reuse, summarize, simple_col)
 
     def asc(self, **kwargs):

It looks like CheckConstraint._get_check_sql ends up passing simple_col=True in Query.build_filter but this doesn't get passed down to the F objects because the intermediate CombinedExpressions don't pass on the parameter?

comment:2 by Simon Charette, 4 years ago

Triage Stage: UnreviewedAccepted
Version: master2.2

This looks similar to #30754.

We don't pass simple_col anymore in master hence why I wasn't able to reproduce. It's an attribute of the Query (306b6875209cfedce2536a6679e69adee7c9bc6a).

this doesn't get passed down to the F objects because the intermediate CombinedExpressions don't pass on the parameter

There could a few things involved but CombinedExpressions effectively doesn't pass along simple_col, in fact it doesn't even accepts it.

https://github.com/django/django/blob/ef22cf41af092d598208b4d373fe40403c2366dd/django/db/models/expressions.py#L465-L470

comment:3 by Mariusz Felisiak, 4 years ago

Resolution: fixed
Status: newclosed
Summary: CheckConstraints with F objects don't apply on SQLiteCheckConstraints with F objects don't apply on SQLite.

This is already fixed on master. Django 2.2 is in extended support and it's not a regression in Django 3.0 so fix doesn't qualify for a backport. I'm going to add a test case to avoid regressions in the future.

Fixed in 306b6875209cfedce2536a6679e69adee7c9bc6a.

Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:4 by GitHub <noreply@…>, 4 years ago

In b423873c:

Refs #31197 -- Added tests for combined expressions in CheckConstraint.check.

Thanks Adam Johnson for the report.

Fixed in 306b6875209cfedce2536a6679e69adee7c9bc6a.

comment:5 by Adam Johnson, 4 years ago

Thanks both. I checked only with stable/3.0.x branch and forgot to see if fixed in master.

For anyone finding this issue, the workaround would be to use SeparateDatabaseAndState with the autodetected operations in the state_operations and a RunSQL in the database_operations that executes the fixed SQL.

Note: See TracTickets for help on using tickets.
Back to Top