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

[gmcrocetti]

This PR fixes #27147.

I could think of two UIs to fix the stated problem:

1. We can add a default_bounds as an argument to RangeField class, and thus all subclasses. The problem of this approach is that we may add some cutler for discrete ranges. For example, IMO it makes no sense to set a default_bounds value for a IntegerRangeField as it'll always be converted to [), cluttering user's experience. Please, let me know if I'm missing something.
2. We can specialize RangeField for continuous ranges. With this approach we're preventing misuse.

I implemented approach 2.

I'm following a conservative approach for default_bounds. For example:

from psycopg2.extras import DateTimeTZRange

lower_date = datetime.date(2014, 1, 1)
upper_date = datetime.date(2014, 2, 2)

class DummyRange(models.Model):
        timestamps = DateTimeRangeField(blank=True, null=True, default_bounds='[]')

DummyRange.objects.create(timestamps=DateTimeTZRange(lower_date, upper_date))  # saved with bounds='[)'
DummyRange.objects.create(timestamps=DateTimeTZRange(lower_date, upper_date, '()'))  # saved with bounds='()'
DummyRange.objects.create(timestamps=(lower_date, upper_date))  # saved with bounds='[]' - default_bounds

The reasoning behind is that a user knows what's he's doing if he created a Range object (Adding a validator sounds like a good idea). I still need to document whichever behavior we choose.

Looking forward for feedback before finishing this feature.

[github-actions]

Hello @gmcrocetti! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

[gmcrocetti]

I'd appreciate any enlightenment on why CI is failing. tox -e py39-postgres -- --settings=tests.test_pg is running without errors while tox is failing, both on my machine.

[charettes]

@gmcrocetti not sure you got tox passing tests locally but the error on Jenkins is the following

https://djangoci.com/job/pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.8/12028/console

Traceback (most recent call last):
  File "./runtests.py", line 646, in <module>
    failures = django_tests(
  File "./runtests.py", line 364, in django_tests
    test_labels, state = setup_run_tests(verbosity, start_at, start_after, test_labels)
  File "./runtests.py", line 298, in setup_run_tests
    apps.set_installed_apps(settings.INSTALLED_APPS)
  File "/home/jenkins/workspace/pull-requests-bionic/database/sqlite3/label/bionic-pr/python/python3.8/django/apps/registry.py", line 355, in set_installed_apps
    self.populate(installed)
  File "/home/jenkins/workspace/pull-requests-bionic/database/sqlite3/label/bionic-pr/python/python3.8/django/apps/registry.py", line 114, in populate
    app_config.import_models()
  File "/home/jenkins/workspace/pull-requests-bionic/database/sqlite3/label/bionic-pr/python/python3.8/django/apps/config.py", line 300, in import_models
    self.models_module = import_module(models_module_name)
  File "/usr/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 783, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/jenkins/workspace/pull-requests-bionic/database/sqlite3/label/bionic-pr/python/python3.8/tests/postgres_tests/models.py", line 132, in <module>
    class RangesModel(PostgreSQLModel):
  File "/home/jenkins/workspace/pull-requests-bionic/database/sqlite3/label/bionic-pr/python/python3.8/tests/postgres_tests/models.py", line 138, in RangesModel
    timestamps_with_closed_bounds = DateTimeRangeField(blank=True, null=True, default_bounds='[]')
TypeError: __init__() got an unexpected keyword argument 'default_bounds'

[gmcrocetti]

Yes, tox wasn't working as I said. It was working only for the postgres environment. Nonetheless the problem was a missing DummyField with default_bounds for non postgres setups.

[charettes]

@gmcrocetti I've seen that you managed to get things working but for the future https://github.com/django/django-docker-box is usually the easiest way to get test running against different backends locally.

[gmcrocetti]

@charettes . I'm not familiar with the django's revision queue. Is there anything else expected from me or you folks handle it now ?

[charettes]

@gmcrocetti the ticket still has the needs documentation checkbox ticked hence why it doesn't show up in the review queue.

Since there's still unanswered questions about the approach to take I'd try to gather feedback from the reporter as well as the folks involved in #10484. If that doesn't yield any result I'd post to the developer mailing list to reach a wider audience.

[gmcrocetti]

@jdufresne , would you like taking a look ?

[felixxm]

@gmcrocetti Thanks for this patch 👍

I could think of two UIs to fix the stated problem:

1. We can add a default_bounds as an argument to RangeField class, and thus all subclasses. The problem of this approach is that we may add some cutler for discrete ranges. For example, IMO it makes no sense to set a default_bounds value for a IntegerRangeField as it'll always be converted to [), cluttering user's experience. Please, let me know if I'm missing something.

2. We can specialize RangeField for continuous ranges. With this approach we're preventing misuse.

I'm fine with the 2nd approach, however we should raise TypeError when a default_bounds is passed to the RangeField because it accepts all keyword arguments so folks may not be aware that it's ignored for other types.

DummyRange.objects.create(timestamps=DateTimeTZRange(lower_date, upper_date)) # saved with bounds='[)'

This example is tricky, I'm not sure if we can distinguish that bounds were passed to the DateTimeTZRange but I would expect default_bounds to be used in this case.

[gmcrocetti]

@felixxm . Thanks a lot for your review !
Before writing more code/fixing your comments I'd like to have an agreement in the interface of this new API.

I'm fine with the 2nd approach, however we should raise TypeError when a default_bounds is passed to the RangeField because it accepts all keyword arguments so folks may not be aware that it's ignored for other types.

I'm ok with raising a TypeError. Could you point to an existing example in the codebase ?

This example is tricky, I'm not sure if we can distinguish that bounds were passed to the DateTimeTZRange but I would expect default_bounds to be used in this case.

Yeah. That's mostly why I was expecting a feedback before writing more code/documentation.
No, we can't distinguish if bounds were passed or not.
We can definitely "intercept" it and force to default_bounds. But I can foresight problems if one wants to force a bound other than the one in default_bounds, going back to the original example:

# Considering the value is being "intercepted"
from psycopg2.extras import DateTimeTZRange

lower_date = datetime.date(2014, 1, 1)
upper_date = datetime.date(2014, 2, 2)

class DummyRange(models.Model):
        timestamps = DateTimeRangeField(blank=True, null=True, default_bounds='[]')

DummyRange.objects.create(timestamps=DateTimeTZRange(lower_date, upper_date))  # OK. saved with bounds='[]'
DummyRange.objects.create(timestamps=DateTimeTZRange(lower_date, upper_date, bounds='(]'))  # NOK. saved with bounds=[], but the user thought it was '(]'
DummyRange.objects.create(timestamps=DateTimeTZRange(lower_date, upper_date, bounds='[)'))  # NOK. saved with bounds='[]'

I agree that it's non-conventional to have this distinction between list and tuples x Range objects. Whoever is using the Range object must be aware that there's a default value for the bounds argument. That's why I assumed the bound attribute must take precedence over default_bounds.
I'm just brainstorming and definitely want to hear your feedback regarding this API. I can see two options:

1. Force bounds to whatever is the value of default_bounds;
2. Leave it to the user and maybe raise a warning saying that input_range.bounds != field.default_bounds

[pauloxnet]

@gmcrocetti thanks for working on this issue.

I think a perfect addition to this PR would be, let people optionally pass the bounds in the list without importing the psycopg2 range types.

Using a different code example:

from django.contrib.postgres.fields import DecimalRangeField
from django.db import models

class Offer(models.Model)
    """An offer with a default price range (all non-zero positive decimals)."""
    price_range = DecimalRangeField(default=(0, None, '()'))  # bounds == '()' as specified

>>> Offer.objects.create(price_range=(0, 150))  # bounds == '[)' as default
>>> Offer.objects.create(price_range=(0, 150, '(]'))  # bounds == '(]' as specified
>>> Offer.objects.create(price_range=(0, 150, '-'))  # bounds == '[)' as fallback

[felixxm]

@gmcrocetti thanks for working on this issue.

I think a perfect addition to this PR would be, let people optionally pass the bounds in the list without importing the psycopg2 range types.

This is a separate ticket (see ticket-31872), which I rejected ....

[pauloxnet]

@gmcrocetti thanks for working on this issue.
I think a perfect addition to this PR would be, let people optionally pass the bounds in the list without importing the psycopg2 range types.

This is a separate ticket (see ticket-31872), which I rejected ....

Thanks for let me know. 👍

[felixxm]

I'm ok with raising a TypeError. Could you point to an existing example in the codebase?

I don't have an example, but something like this:

class RangeField(models.Field):
    ...

    def __init__(self, *args, **kwargs):
        if 'default_bounds' in kwargs:
            raise TypeError(f"Cannot use 'default_bounds' with {self.__class__.__name__}.")

should work.

I'm just brainstorming and definitely want to hear your feedback regarding this API. I can see two options

OK, let's leave it as it is. We can add a note in docs, that default_bounds are ignored for range types from psycopg2.extras.

[gmcrocetti]

@felixxm , pushed documentation and left one question regarding the validation of default_bounds.
Great thanks

[felixxm]

@gmcrocetti Thanks for updates 👍

We should also add release notes (4.1.txt) and update Range Fields description.

[gmcrocetti]

@felixxm , updated.

[felixxm]

@gmcrocetti Thanks for updates 👍 Welcome aboard ⛵

I pushed final edits to docs and tests.

[gmcrocetti]

@felixxm , thanks a lot for all the effort. Documentation is pretty clear and thanks for the tests. Just left one nit.
Thanks again ! :)