Opened 19 months ago
Closed 19 months ago
#34553 closed Bug (fixed)
Can't create CheckConstraint with percent characters in values on postgresql due to broken quoting
Reported by: | Thomas Kolar | Owned by: | Simon Charette |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.2 |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
It is currently impossible to create check constraints that reference values containing percent characters (%
).
Consider the following model definition:
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 %
:
>>> 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 "%%"
Attachments (1)
Change History (11)
by , 19 months ago
comment:1 by , 19 months ago
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.
comment:2 by , 19 months ago
Triage Stage: | Unreviewed → Accepted |
---|
Thanks for the report 🏆
I've verified the issue on 59262c294d26d2aa9346284519545c0f988bf353
Edit: Verified issue with Postgres & MySQL. SQLite works. Oracle untested.
comment:3 by , 19 months ago
Looks like regression in a8b3f96f6acfa082f99166e0a1cfb4b0fbc0eace
Edit: actually bisected to commit 42e8cf47c7ee2db238bf91197ea398126c546741
comment:4 by , 19 months ago
Cc: | added |
---|
comment:5 by , 19 months ago
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
.
comment:6 by , 19 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 19 months ago
Has patch: | set |
---|
comment:8 by , 19 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
sample django project that demonstrates the issue