Opened 2 years ago

Last modified 5 months ago

#29622 new New feature

Support string transforms in functional indexes.

Reported by: James Howe Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: jmh205@…, Hannes Ljungberg Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am using PostgreSQL and have some JSONField-type fields in my models.
I had added some indexes to these, and they worked fine previously.

class Meta:
    indexes = [
        models.Index(fields=['json_field__property'], name='my_idx'])
    ]

However, having just upgraded to Django 2.1, the checks on startup now fail:

ERRORS:
myapp.MyModel: (models.E012) 'indexes' refers to the nonexistent field 'json_field__property'.

My tests (run with pytest-django) all still work, so it seems to only be the system check that's the problem.

Change History (6)

comment:1 Changed 2 years ago by Tim Graham

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

The system check is new in Django 2.1 (#28714).

comment:2 Changed 2 years ago by Simon Charette

The check probably needs to be adjusted to use get_lookups.

comment:3 Changed 2 years ago by Josh Schneier

How did this ever work & what version of Django are you upgrading from? set_name_with_model uses ._meta.get_field which would fail but I see you've worked around that by explicitly naming your index; however create_sql uses the same logic.

I was able to get the system check to pass & the migration to generate by: splitting on LOOKUP_SEP, checking if it's a JSONField, and ensuring that none of the lookup paths is in .get_lookups. Attempting to apply the migration failed because of the aforementioned issues in .create_sql. Obviously that also required importing JSONField which is a no-go.

I think one approach to fixing this is an Index subclass that works specifically for JSONField since it is pretty different, happy to work on it if that sounds reasonable but I'll let someone else more familiar with the system take a look . We'd have to call this previous behavior unsupported I think.

comment:4 Changed 2 years ago by James Howe

How did this ever work

Ah, I also have a custom SQL migration so I could make it partial.

Adding 'models.E012' to SILENCED_SYSTEM_CHECKS should give exactly the same behaviour as in 2.0?

comment:5 Changed 2 years ago by Simon Charette

Severity: Release blockerNormal

This is not a release blocker in this case.

@Josh I don't think we should ship a special index for this case. #26167 will add support for functional indices where the above will be expressible using KeyTransforms.

Not sure if this ticket should be closed as duplicate of #26167 or a new ticket should be opened (or this one renamed) to add support for string transforms to Index to reduce the boiler plate required to define a functional index.

e.g.

  • Index('join_date__year') instead of Index(ExtractYear('join_date'))
  • Index('json_field__property') instead of Index(KeyTransform('property', 'json_field')).

comment:6 Changed 5 months ago by felixxm

Cc: Hannes Ljungberg added
Component: Core (System checks)Database layer (models, ORM)
Summary: Indexes on JSONB paths fail system checks in 2.1Support string transforms in functional indexes.
Triage Stage: AcceptedSomeday/Maybe
Type: BugNew feature
Version: 2.1master

I agree with Simon, this ticket is an improvement to the #26167. Functional indexes are complicated even without this, so we should wait for #26167 to land.

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