[Django] #34553: Can't create CheckConstraint with percent characters in values on postgresql due to broken quoting
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas | Owner: nobody
Kolar |
Type: Bug | Status: new
Component: Database | Version: 4.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
It is currently impossible to create check constraints that reference
values containing percent characters (`%`).
Consider the following model definition:
{{{#!python
from django.db import models
class Dolor(models.Model):
sit = models.CharField(max_length=42069)
class Meta:
constraints = (
models.CheckConstraint(
check=models.Q(sit="%"),
name="amet",
),
)
}}}
This will indeed result in a constraint being created. However, the
percent character will be doubled, and constrain `sit` to contain `%%`,
not `%`:
{{{#!python
>>> from ipsum.models import *
>>> Dolor.objects.create(sit="%")
Traceback (most recent call last):
(... snip ...)
psycopg2.errors.CheckViolation: new row for relation "ipsum_dolor"
violates check constraint "amet"
DETAIL: Failing row contains (1, %).
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
(... snip ...)
django.db.utils.IntegrityError: new row for relation "ipsum_dolor"
violates check constraint "amet"
DETAIL: Failing row contains (1, %).
>>> Dolor.objects.create(sit="%%")
<Dolor: Dolor object (2)>
>>>
}}}
This may be a super contrived example, but using an `__in` lookup in the
check with a list of allowed values also does not work (`choices` are not
a suitable alternative here as that isn't enforced at the DB level).
This was likely introduced with the fix for #30484 - here, the schema
editor's `quote_value` method was changed to always double percent
characters.
I'd also like to note that creating a `CheckConstraint` like this DOES
work in Django 2.2. Likely, that stopped being the case when #30060 was
fixed. I can attest that this is a serious roadblock when moving a project
from 2.2 to a modern and supported version of Django.
Honestly, I'm surprised that this doesn't break in way more instances.
"Quoting" like this should only be correct specifically for `LIKE` (and
the likes of it).
Suggested fix: remove the percentage character doubling from the general-
purpose escaping method, and reintroduce it in a targeted way where
needed.
I unfortunately don't know enough about the ORM's internals to make a more
detailed suggestion, or know for just how much headache creation I am to
apologize. But the current behavior is obviously incorrect.
Additional notes:
- This affects django 3.2 as well
- This almost certainly affects the oracle backend as well
- This works as expected on sqlite3
I have attached a sample django project that reproduces the issue.
Steps to reproduce (require docker installation):
- Spin up a postgres instance: `docker run -e POSTGRES_USER=lorem -e
POSTGRES_PASSWORD=lorem -e POSTGRES_DB=lorem -p 5432:5432 postgres`
- Extract `lorem.zip`
- Create a virtual environment
- Install `django==4.2.1`, `psycopg2-binary==2.9.6`
- Activate the virtual environment
- Extract `lorem.zip` and change into django directory
- `python -m manage migrate`
- `python -m manage shell`
- `from ipsum.models import *`
- `Dolor.objects.create(sit="%")`
- expected behavior: This should return a model instance
- actual behavior: exception (as described above)
- `Dolor.objects.create(sit="%%").sit`
- expected behavior: exception (as described above)
- actual behavior: returns "%%"
--
Ticket URL: <https://code.djangoproject.com/ticket/34553>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "lorem.zip" added.
sample django project that demonstrates the issue
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Thomas Kolar):
I have discovered a very hacky workaround that should cover quite a few
use cases, in case anyone else is affected by this and needs a fix ASAP.
The workaround is to combine a `__contains` check with a `__length` check
- a string that contains another string and has the same length must be
equal.
`__contains` with a `%` IS supported - as it happens, the fact that
support for this was added is what presumably caused this issue (see
above).
In order to do this, the `__length` lookup must be registered as described
in this stackoverflow answer https://stackoverflow.com/a/68274322 . This
must obviously happen before migrations are run.
For obvious reasons, I do not endorse this abstruse tomfoolery as an
intended solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:1>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Thanks for the report 🏆
I've verified the issue on 59262c294d26d2aa9346284519545c0f988bf353
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:2>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
Looks like regression in a8b3f96f6acfa082f99166e0a1cfb4b0fbc0eace
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:3>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:4>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
The usage of `quote_value` in `ddl_references.Statement.__str__` is
problematic because the later doubles `%`. I haven't fully investigated
what ought to be done to resolve the issue here but it relates to how most
databases don't support parametrized DDL.
The issue here relates to a8b3f96f6acfa082f99166e0a1cfb4b0fbc0eace /
#30408 as well. Now that we explicitly pass `params=None` I suspect
that'll we'll simply want to remove the `%` doubling that takes place in
`SchemaEditor.quote_value`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:5>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:6>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:7>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:8>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"e0f8104a968d316925f736e961a63bef73a686b4" e0f8104a]:
{{{
#!CommitTicketReference repository=""
revision="e0f8104a968d316925f736e961a63bef73a686b4"
Refs #34553 -- Split constraint escaping test in subtests.
This ensures that constraint violations are tested in isolation from
each other as an IntegrityError only ensures a least one constraint is
violated.
For example, the assertion added in 42e8cf4 break both the
name_constraint_rhs and the rebate_constraint constraints and thus
doesn't constitute a proper regression test. Refs #32369.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:9>
Django
postgresql due to broken quoting
-------------------------------------+-------------------------------------
Reporter: Thomas Kolar | Owner: Simon
| Charette
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ffff17d4b0117cce59f65c9f56fa164694eafd23" ffff17d4]:
{{{
#!CommitTicketReference repository=""
revision="ffff17d4b0117cce59f65c9f56fa164694eafd23"
Fixed #34553 -- Fixed improper % escaping of literal in constraints.
Proper escaping of % in string literals used when defining constaints
was attempted (a8b3f96f6) by overriding quote_value of Postgres and
Oracle schema editor. The same approach was used when adding support for
constraints to the MySQL/MariaDB backend (1fc2c70).
Later on it was discovered that this approach was not appropriate and
that a preferable one was to pass params=None when executing the
constraint creation DDL to avoid any form of interpolation in the first
place (42e8cf47).
When the second patch was applied the corrective of the first were not
removed which caused % literals to be unnecessary doubled. This flew
under the radar because the existings test were crafted in a way that
consecutive %% didn't catch regressions.
This commit introduces an extra test for __exact lookups which
highlights more adequately % doubling problems but also adjust a
previous __endswith test to cover % doubling problems (%\% -> %%\%%).
Thanks Thomas Kolar for the report.
Refs #32369, #30408, #30593.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34553#comment:10>