#29738 closed Bug (fixed)
Django can't serialize DateTimeTZRange(lower=None, upper=None, bounds='[)')
Reported by: | Graham Mayer | Owned by: | Can Sarıgöl |
---|---|---|---|
Component: | Migrations | Version: | 2.0 |
Severity: | Normal | Keywords: | rangefield postgresql psycopg2 migrations removed |
Cc: | jon.dufresne@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Tried to use DateTimeTZRange(lower=None, upper=None, bounds='[)')
as a default for a model field and get the following error when running python manage.py makemigrations
:
Traceback (most recent call last): File "manage.py", line 12, in <module> execute_from_command_line(sys.argv) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/core/management/__init__.py", line 371, in execute_from_command_line utility.execute() File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/core/management/__init__.py", line 365, in execute self.fetch_command(subcommand).run_from_argv(self.argv) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/core/management/base.py", line 288, in run_from_argv self.execute(*args, **cmd_options) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/core/management/base.py", line 335, in execute output = self.handle(*args, **options) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/core/management/commands/makemigrations.py", line 172, in handle self.write_migration_files(changes) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/core/management/commands/makemigrations.py", line 210, in write_migration_files migration_string = writer.as_string() File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/db/migrations/writer.py", line 151, in as_string operation_string, operation_imports = OperationWriter(operation).serialize() File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/db/migrations/writer.py", line 110, in serialize _write(arg_name, arg_value) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/db/migrations/writer.py", line 74, in _write arg_string, arg_imports = MigrationWriter.serialize(_arg_value) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/db/migrations/writer.py", line 279, in serialize return serializer_factory(value).serialize() File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/db/migrations/serializer.py", line 203, in serialize return self.serialize_deconstructed(path, args, kwargs) File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/db/migrations/serializer.py", line 90, in serialize_deconstructed arg_string, arg_imports = serializer_factory(arg).serialize() File "/home/grahammayer/logimeter/logimeter/lib/python3.6/site-packages/django/db/migrations/serializer.py", line 370, in serializer_factory "topics/migrations/#migration-serializing" % (value, get_docs_version()) ValueError: Cannot serialize: DateTimeTZRange(None, None, '[)')
Change History (17)
comment:1 by , 6 years ago
Component: | Migrations → contrib.postgres |
---|---|
Description: | modified (diff) |
Easy pickings: | set |
Keywords: | rangefield postgresql psycopg2 migrations added |
Needs documentation: | set |
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 6 years ago
Description: | modified (diff) |
---|---|
Keywords: | rangefield postgresql psycopg2 migrations removed |
A solution could to make django.db.migration.serializer
offer a way to register serializers for types and have django.contrib.postgres.App.ready()
be in charge of registering the ones to handle psycopg2
range types.
If the registry uses an OrderedDict
we should be able to replace that long series of if by iterating over the registry items.
registry = OrderedDict([ (models.Field, ModelFieldSerializer), ... ]) def serializer_factory(value): if isinstance(value, Promise): value = str(value) elif isinstance(value, LazyObject): # The unwrapped value is returned as the first item of the arguments # tuple. value = value.__reduce__()[1][0] for type_, serializer_cls in registry.items(): if isinstance(value, type_): return serializer_cls(value) raise ValueError(...)
The hasattr
and checks could be performed using abc
s.
from abc import ABC class Deconstructable(ABC): @classmethod def __subclasshook__(cls, C): return hasattr(C, 'deconstruct')
comment:3 by , 6 years ago
Keywords: | rangefield postgresql psycopg2 migrations removed added |
---|
comment:4 by , 6 years ago
Description: | modified (diff) |
---|
comment:5 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 6 years ago
Can I change serializer_factory like this? so that all unserialized values can be tried to be available.
class UnserializablesSerializer(BaseSerializer): def serialize(self): from django.utils.deconstruct import deconstructible deconstructible_klass = deconstructible( self.value.__class__, path=self.value.__module__ + "." + self.value.__class__.__name__ ) init_values = self.value.__reduce__() new_value = deconstructible_klass() if len(init_values) > 2: new_value = deconstructible_klass(*[v for v in init_values[2].values()]) return DeconstructableSerializer(new_value).serialize() def serializer_factory(value): ... if hasattr(value, "__module__") and hasattr(value, "__class__"): return UnserializablesSerializer(value) raise ValueError(...)
comment:7 by , 6 years ago
comment:8 by , 6 years ago
Component: | contrib.postgres → Migrations |
---|---|
Easy pickings: | unset |
Has patch: | set |
Needs documentation: | unset |
Needs tests: | unset |
Patch needs improvement: | set |
I tested the patch and noticed that while it worked for the use case in the ticket, it put import psycopg2._range
in the migration rather than from psycopg2.extras import DateTimeTZRange
that's in the models file. Maybe there's no way around that.
comment:9 by , 6 years ago
When I check DateTimeTZRange, the result is like below. Is there any problem or does this usage make a problem?
>>> from psycopg2.extras import DateTimeTZRange >>> print(DateTimeTZRange().__class__) <class 'psycopg2._range.DateTimeTZRange'>
comment:10 by , 6 years ago
New PR. Still needs improvement. I'm not sure the general approach is suitable.
comment:11 by , 6 years ago
Patch needs improvement: | unset |
---|
New PR Could you review this last pr? i don't know why but there is a problem in "pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3" test. I checked in my local django-box and it was good.
comment:14 by , 6 years ago
After running a bisect, I've found that this change caused a regression for my project. This occurs when running management commands. It looks like from django.db.migrations.writer import MigrationWriter
is not safe at the module level when django.setup()
is called. Here is the stack trace:
Traceback (most recent call last): File ".../myproject/manage.py", line 12, in <module> execute_from_command_line(sys.argv) File ".../django/django/core/management/__init__.py", line 381, in execute_from_command_line utility.execute() File ".../django/django/core/management/__init__.py", line 357, in execute django.setup() File ".../django/django/__init__.py", line 24, in setup apps.populate(settings.INSTALLED_APPS) File ".../django/django/apps/registry.py", line 91, in populate app_config = AppConfig.create(entry) File ".../django/django/apps/config.py", line 116, in create mod = import_module(mod_path) File "/usr/lib64/python3.7/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "<frozen importlib._bootstrap>", line 1006, in _gcd_import File "<frozen importlib._bootstrap>", line 983, in _find_and_load File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 677, in _load_unlocked File "<frozen importlib._bootstrap_external>", line 728, in exec_module File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed File ".../django/django/contrib/postgres/apps.py", line 8, in <module> from django.db.migrations.writer import MigrationWriter File ".../django/django/db/migrations/writer.py", line 10, in <module> from django.db.migrations.loader import MigrationLoader File ".../django/django/db/migrations/loader.py", line 8, in <module> from django.db.migrations.recorder import MigrationRecorder File ".../django/django/db/migrations/recorder.py", line 9, in <module> class MigrationRecorder: File ".../django/django/db/migrations/recorder.py", line 22, in MigrationRecorder class Migration(models.Model): File ".../django/django/db/models/base.py", line 99, in __new__ app_config = apps.get_containing_app_config(module) File ".../django/django/apps/registry.py", line 252, in get_containing_app_config self.check_apps_ready() File ".../django/django/apps/registry.py", line 135, in check_apps_ready raise AppRegistryNotReady("Apps aren't loaded yet.") django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.
This change appears to fix it for me, but I haven't had a chance to fully verify its correctness or write a test.
diff --git a/django/contrib/postgres/apps.py b/django/contrib/postgres/apps.py index 97475de6f7..151450f4e4 100644 --- a/django/contrib/postgres/apps.py +++ b/django/contrib/postgres/apps.py @@ -5,7 +5,6 @@ from psycopg2.extras import ( from django.apps import AppConfig from django.db import connections from django.db.backends.signals import connection_created -from django.db.migrations.writer import MigrationWriter from django.db.models import CharField, TextField from django.test.signals import setting_changed from django.utils.translation import gettext_lazy as _ @@ -22,6 +21,7 @@ def uninstall_if_needed(setting, value, enter, **kwargs): Undo the effects of PostgresConfig.ready() when django.contrib.postgres is "uninstalled" by override_settings(). """ + from django.db.migrations.writer import MigrationWriter if not enter and setting == 'INSTALLED_APPS' and 'django.contrib.postgres' not in set(value): connection_created.disconnect(register_type_handlers) CharField._unregister_lookup(Unaccent) @@ -42,6 +42,7 @@ class PostgresConfig(AppConfig): verbose_name = _('PostgreSQL extensions') def ready(self): + from django.db.migrations.writer import MigrationWriter setting_changed.connect(uninstall_if_needed) # Connections may already exist before we are called. for conn in connections.all():
comment:15 by , 6 years ago
Cc: | added |
---|
Presumably you are using
default=DateTimeTZRange(lower=None, upper=None, bounds='[)')
? Could you provide a simple example project?The problem is that
psycopg2.extras.DateTimeTZRange
cannot be serialized into a migrations, see documentation.So there are a few options:
default=None, null=True
?default=(None, None)
work instead? (I believe this will be limited to the default bounds:[)
)DateTimeTZRange
deconstructible which will allow for other bounds to be defined.We should be able to get away with making these range objects deconstructible as we only expect them to take simple arguments that are already supported by the serializer.
Here is an example:
I managed to produce the following migration:
(Disclaimer: I have only attempted to produce migrations and not actually called
RangeTestModel.objects.create()
...)So in summary, I think that we only need to add some documentation to explain how to handle defaults for range fields and tests to ensure that it continues to work.