Opened 3 years ago
Closed 3 years ago
#32770 closed New feature (needsinfo)
Add system check for django.contrib.postgres in INSTALLED_APPS when using OpClass().
Reported by: | Seth Yastrov | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 3.2 |
Severity: | Normal | Keywords: | |
Cc: | Hannes Ljungberg | Triage Stage: | Unreviewed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Given the following model + index:
from django.db import models from django.contrib.postgres.indexes import GinIndex, OpClass from django.db.models.functions import Cast class MyModel(models.Model): class Meta: indexes = [ GinIndex(OpClass(Cast("id", output_field=models.TextField()), name='gin_trgm_ops'), name='foobar') ]
After running makemigrations and running the migration, it produces the SQL:
CREATE INDEX "foobar" ON "myapp_mymodel" USING gin ((CAST("id" AS text) gin_trgm_ops));
Which is a syntax error on Postgres (version 12). The reason is the extra parentheses around CAST.
Change History (4)
comment:1 by , 3 years ago
Cc: | added |
---|---|
Component: | Migrations → Database layer (models, ORM) |
Easy pickings: | unset |
Resolution: | → worksforme |
Status: | new → closed |
comment:2 by , 3 years ago
I'm pretty sure that you're seeing this error because of not adding django.contrib.postgres
to INSTALLED_APPS
, see: https://docs.djangoproject.com/en/3.2/ref/contrib/postgres/indexes/#opclass-expressions
comment:3 by , 3 years ago
Resolution: | worksforme |
---|---|
Status: | closed → new |
Summary: | GinIndex with OpClass and Cast results in Postgres syntax error in migration → GinIndex with OpClass and Cast results in Postgres syntax error in migration when django.contrib.postgres is not part of INSTALLED_APPS |
Mariusz Felisiak, thanks for your comment. If you'll see my example, the extra parentheses are surrounding gin_trgm_ops
as well, not just the CAST
expression. This seems to be the problem.
Thank you Hannes Ljungberg!
I did not notice that the documentation says to add django.contrib.postgres
to INSTALLED_APPS
. That fixes the problem.
Would it be an idea to make a check that django.contrib.postgres
is in INSTALLED_APPS
when making use of OpClass
so that instead of encountering a Postgres syntax error and not knowing what to do (and assuming there is a bug in Django), you would get a helpful error message telling how to correct the problem?
Therefore I'm reopening the issue, and hoping that this check can be made, as despite this requirement being mentioned in the docs, the current behavior is quite unintuitive.
comment:4 by , 3 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
Summary: | GinIndex with OpClass and Cast results in Postgres syntax error in migration when django.contrib.postgres is not part of INSTALLED_APPS → Add system check for django.contrib.postgres in INSTALLED_APPS when using OpClass(). |
Type: | Bug → New feature |
Therefore I'm reopening the issue, and hoping that this check can be made, as despite this requirement being mentioned in the docs, the current behavior is quite unintuitive.
It would be really complicated. First of all, we would need to mix-up logic from a contrib app and the ORM. Secondly OpClass()
don't need to be the topmost expression, so the flatten list of expression would be necessary. Thirdly we don't have similar checks for fields from django.contrib.postgres
app which also require including 'django.contrib.postgres'
in INSTALLED_APPS
, e.g. HStoreField
. I don't think it's worth complexity, however we can reconsider this decision if someone provides PoC.
Extra parentheses shouldn't be an issue on PostgreSQL. Also I cannot reproduce this crash on PostgreSQL 12.6, see the following test.