Add support for defining bounds in postgres range fields

[Kirill Stepanov]

Django supports postgres range types in the contrib package, but the bounds are restricted to the default value of "[)" (inclusive low value, exclusive high value) in psycopg2. I propose that django add support for all bounds types - "[]", "(]", "[)" and "()".

Psycopg2 documentation on the matter: ​http://initd.org/psycopg/docs/extras.html?highlight=range#range-data-types
Postgres documentation on the syntax: ​https://www.postgresql.org/docs/9.2/static/rangetypes.html#RANGETYPES-IO

I think this could easily be implemented via another argument to the range field type contructor which could be passed down to the psycogp2 range type whenever it's created. Everything else should be automatic after that. Some validation is probably a good idea too.

If people think this is a useful feature, I'll try to write a patch.

[Tim Graham]

Can you give a use case to demonstrate exactly how this would work? See also #26345.

[Kirill Stepanov]

Replying to timgraham:

Can you give a use case to demonstrate exactly how this would work? See also #26345.

That ticket only refers to the documentation, not adding ability to override.

My use-case is for date ranges. It's often more natural to have inclusive ranges ([]) than the default [). For example it would make more sense that is something active for a month to start on the first and end on the last day of the month rather than the first of the next month, no?

I recently converted a model from a lower/upper bound pair of date fields to a DateRange to take advantage of the gist index overlap exclusion but had to give up this natural property. I was hoping to implement the ability to do any type of bounds in django to bring the functionality back. Of course even for dates this is not always most optimal - hotel reservations make more sense with a [) range as people can check-in on the same day that others check out in the same room.

Anyway that's my 2¢.

[Tim Graham]

Is this about form fields and/or model fields? How does it interact with the fact that for some types, "PostgreSQL always returns a range in a canonical form that includes the lower bound and excludes the upper bound; that is [)." as the documentation ticket says. Accepting because the idea seems sensible, however, I would like to see a patch to make a final evaluation of the idea.

[Kirill Stepanov]

Replying to timgraham:

Is this about form fields and/or model fields? How does it interact with the fact that for some types, "PostgreSQL always returns a range in a canonical form that includes the lower bound and excludes the upper bound; that is [)." as the documentation ticket says. Accepting because the idea seems sensible, however, I would like to see a patch to make a final evaluation of the idea.

Ah, I saw that but misunderstood as "no matter how it was originally inserted, django will always interpret incoming data as [) because it doesn't know any better. I now see that postgres canonicalizes discrete ranges but I'll see what I can do about it. The proposal still at least makes sense for non-discrete ranges.

[Matthew Schinckel]

There's two parts: ensuring that the user-entered-data is upper-bounds-inclusive, and ensuring that the saved data is presented back to the user as upper-bounds-inclusive.

The first part of this is trivial - just have a bounds value of '[]' passed in to the constructor. In my case, I have an InclusiveRangeMixin that does this in compress.

The second part is less clear - because in the case of distinct range types there is a "unit value" that needs to be removed from the upper bounds, but in the case of continuous ranges, this is not possible (but in that case, postgres will not have normalised it, so it's all good).

Anyway, code speaks louder than words here, so here's how I've approached this in the past:

class InclusiveRangeMixin(object):
    _unit_value = None

    def compress(self, values):
        range_value = super(InclusiveRangeMixin, self).compress(values)
        if range_value:
            return self.range_type(range_value.lower, range_value.upper, bounds='[]')

    def prepare_value(self, value):
        value = super(InclusiveRangeMixin, self).prepare_value(value)
        # We need to clean both fields.
        value = [field.clean(val) for field, val in zip(self.fields, value)]
        if value[1] is not None:
            value[1] = value[1] - self._unit_value
        return value


class InclusiveDateRangeField(InclusiveRangeMixin, DateRangeField):
    _unit_value = datetime.timedelta(1)


class InclusiveIntegerRangeField(InclusiveRangeMixin, IntegerRangeField):
    _unit_value = 1

I think this could be encapsulated better by a kwarg to the field constructor (inclusive=False by default, and allow setting it to True).

[Matthew Schinckel]

Oh, I did this at the form field level.

[Matthew Schinckel]

Okay, that would not quite meet the requirements of this ticket (which suggest any range type should be selectable). That makes it a bit more complicated, but I think maybe defining a target bounds in the field definition is still the best approach.

We'd want to be applying a fixed bounds to the incoming data (unless the widget had some way to represent the bounds types, but that's probably less useful for real humans to interact with), and the data fetched from the db should be denormalised (?) to match those bounds.

[Jakub Skałecki]

In case anyone has this problem, I've prepared a fix. Of course better if appropriate fields natively supported bounds argument, but this is a drop-in replacement:

# db_utils.py
import sys
from datetime import timedelta
from functools import partial, wraps
from django.contrib.postgres.fields import ranges


def create_bounded_range_field(cls, unit_diff):
    class RangeField(cls):
        def __init__(self, *args, **kwargs):
            self.bounds = kwargs.pop('bounds', '[)')
            self.range_type = partial(self.range_type, bounds=self.bounds)
            super(RangeField, self).__init__(*args, **kwargs)

        def from_db_value(self, value, expression, connection):
            # we're checking if bounds differ from default value
            if self.bounds[0] == '(' and value.lower:
                value._lower = value.lower - unit_diff
            if self.bounds[1] == ']' and value.upper:
                value._upper = value.upper - unit_diff
            value._bounds = self.bounds
            return value


    RangeField.__name__ = cls.__name__
    return RangeField


DateRangeField = create_bounded_range_field(ranges.DateRangeField, timedelta(days=1))
DateTimeRangeField = create_bounded_range_field(ranges.DateTimeRangeField, timedelta(milliseconds=1))
BigIntegerRangeField = create_bounded_range_field(ranges.BigIntegerRangeField, 1)
FloatRangeField = create_bounded_range_field(ranges.FloatRangeField, sys.float_info.min)
IntegerRangeField = create_bounded_range_field(ranges.IntegerRangeField, 1)

Not it's possible to declare field like this:

from db_utils import DateRangeField

class MyModel(Model):
    dates = DateRangeField(bounds='[]')

If this solution has drawbacks, let me know.

[Jon Dufresne]

Thanks Jakub Skałecki for the initial code.

​PR

[Guilherme Martins Crocetti]

Hi, I'm a long time Django's user but first time commenting here.

I'm currently facing a problem related to this ticket: I have a DateTimeRangeField field and there's no easy way, other than writing a custom ModelForm and subclassing forms.DateTimeRangeField, to customize a bound other than the default "[)". These values are represented by tstzrange, one of the types that does not "suffer" of the aforementioned canonical representation ​https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE.

Is this issue still relevant for the current state of Django ? IMO it's a good feature to add...

Looking forward for opinions.

[Carlton Gibson]

Hi Guilherme, welcome!

It still looks like a reasonable suggestion yes.

I'd review Matthew's comments and the patch from Jakub and Jon, and then see what the remaining issue are.
From the PR, I'd guess that making sure you have a good range of test cases will be key.

[Guilherme Martins Crocetti]

​https://github.com/django/django/pull/14538

[Guilherme Martins Crocetti]

Replying to Carlton Gibson:

Hi Guilherme, welcome!

It still looks like a reasonable suggestion yes.

I'd review Matthew's comments and the patch from Jakub and Jon, and then see what the remaining issue are.
From the PR, I'd guess that making sure you have a good range of test cases will be key.

Hey Carlton, I've asked help for contributors in both Github and Django's mailing group without success. The PR has been opened for two months and I'm starting to loose context of what I had done. Is there any chance of you giving it a shot (I'm mentioning you cause you gave the starting tip) ? IMO the overall solution looks solid but need a green light before spending energy writing documentation.

The PR is quite short :). Looking forward for a review.

[Carlton Gibson]

Hi Guilherme. With the Needs Documentation flag the ticket doesn't show up on our review list. I've unchecked that now.

[Mariusz Felisiak <felisiak.mariusz@…>]

In fc565cb:

Fixed #27147 -- Allowed specifying bounds of tuple inputs for non-discrete range fields.

[Jared Ahern]

Hi Guilherme (and others)!

I'm trying to follow this issue, and I'm slightly confused. The changes for continuous fields look great and are much appreciated! But I thought the original intent of the ticket was to enable a "default_bounds" argument for all range fields. Since Jacob and Jon's PR didn't get merged, I don't believe there presently any logic for handling the "+/- step" for discrete range fields.

My specific use case is to have things like DateRangeField and IntegerRangeField be editable in the admin by users that expect a "[]" format. Am I correct in thinking that if I wanted to have a "[]" bounded DateRangeField, I would still need to modify the field and/or form field appropriately? If so, would a further patch be accepted that would facilitate this at the model or form field level?

Apologies if I'm missing something, or if I should be raising this outside of this ticket!