Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#30621 closed Bug (fixed)

CheckConstraint crash with __contains lookup on DateTimeRangeFields.

Reported by: Tilman Koschnick Owned by: felixxm
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 15 months ago.

Download all attachments as: .zip

Change History (12)

Changed 15 months ago by Tilman Koschnick

Attachment: ranges.diff added

comment:1 Changed 15 months ago by felixxm

Cc: Ian Foote added
Has patch: unset
Severity: NormalRelease blocker
Summary: CheckConstraint: outer range contains inner range casts rhs field to lhs.inner_typeCheckConstraint crash with __contains lookup on DateTimeRangeFields.
Triage Stage: UnreviewedAccepted

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

Reproduced at 34a88b21dae71a26a9b136b599e5cbcf817eae16.

comment:2 Changed 15 months ago by Tilman Koschnick

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 Changed 15 months ago by felixxm

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

comment:4 in reply to:  3 Changed 15 months ago by Tilman Koschnick

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 Changed 15 months ago by felixxm

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

comment:6 Changed 15 months ago by felixxm

Owner: changed from nobody to felixxm
Status: newassigned

comment:7 Changed 15 months ago by felixxm

Has patch: set

comment:8 Changed 15 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:9 Changed 15 months ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

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 Changed 15 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 15 months ago by Tilman Koschnick

Thanks a lot for fixing this!

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