Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30621 closed Bug (fixed)

CheckConstraint crash with __contains lookup on DateTimeRangeFields.

Reported by: Tilman Koschnick Owned by: Mariusz Felisiak
Component: Database layer (models, ORM) Version: 2.2
Severity: Release blocker Keywords:
Cc: Ian Foote 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

I have a CheckConstraint:

CheckConstraint(check=m.Q(outer__contains=m.F('inner')), name='outer_contains_inner')

where both outer and inner are DateTimeRangeFields. During migration, the inner range gets cast to its underlying type:

ALTER TABLE "test" ADD CONSTRAINT "outer_contains_inner" CHECK ("outer" @> ("inner")::timestamp with time zone)

The attached patch avoids the cast if both ranges are of compatible type (have not checked what happens if one is DateRange and the other is DateTimeRange, though).

Another workaround would be to switch the relation around:

CheckConstraint(check=m.Q(inner__contained_by=m.F('outer')), name='inner_contained_by_outer')

but I feel since both give equal results, they should both be possible.

Attachments (1)

ranges.diff​ (713 bytes ) - added by Tilman Koschnick 5 years ago.

Download all attachments as: .zip

Change History (12)

by Tilman Koschnick, 5 years ago

Attachment: ranges.diff​ added

comment:1 by Mariusz Felisiak, 5 years ago

Cc: Ian Foote added
Has patch: unset
Severity: Normal → Release blocker
Summary: CheckConstraint: outer range contains inner range casts rhs field to lhs.inner_type → CheckConstraint crash with __contains lookup on DateTimeRangeFields.
Triage Stage: Unreviewed → Accepted

Thanks for the report. Would you like to submit a patch (with release notes and tests) via GitHub?

Reproduced at 34a88b21dae71a26a9b136b599e5cbcf817eae16.

comment:2 by Tilman Koschnick, 5 years ago

To be honest, I am not quite sure why and when you would cast the expression in the first place. Shouldn't the programmer make sure to pass the right kind of value?

comment:3 by Mariusz Felisiak, 5 years ago

For me it is a bug and you're patch looks reasonable at first glance.

in reply to:  3 comment:4 by Tilman Koschnick, 5 years ago

Replying to felixxm:

For me it is a bug and you're patch looks reasonable at first glance.

It's a bug for sure - but I believe a better fix would be to drop the casting bit entirely, but I don't know about the reasoning for putting it in in the first place, so would be reluctant to remove it.

comment:5 by Mariusz Felisiak, 5 years ago

You can check #26903 and 6b048b364ca1e0e56a0d3815bf2be33ac9998355 for the reasoning.

comment:6 by Mariusz Felisiak, 5 years ago

Owner: changed from nobody to Mariusz Felisiak
Status: new → assigned

comment:7 by Mariusz Felisiak, 5 years ago

Has patch: set

​PR

comment:8 by Carlton Gibson, 5 years ago

Triage Stage: Accepted → Ready for checkin

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

Resolution: → fixed
Status: assigned → closed

In 7991111:

Fixed #30621 -- Fixed crash of contains lookup for Date/DateTimeRangeField when the right hand side is the same type.

Thanks Tilman Koschnick for the report and initial patch.
Thanks Carlton Gibson the review.

Regression in 6b048b364ca1e0e56a0d3815bf2be33ac9998355.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 1088a977:

[2.2.x] Fixed #30621 -- Fixed crash of contains lookup for Date/DateTimeRangeField when the right hand side is the same type.

Thanks Tilman Koschnick for the report and initial patch.
Thanks Carlton Gibson for the review.

Regression in 6b048b364ca1e0e56a0d3815bf2be33ac9998355.
Backport of 7991111af12056ec9a856f35935d273526338c1f from master

comment:11 by Tilman Koschnick, 5 years ago

Thanks a lot for fixing this!

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