Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#14823 closed (fixed)

Unexpected behavior with core.serializers.register_serializer and unregister_serializer

Reported by: miker985@… Owned by: nobody
Component: Core (Serialization) Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


Almost all the functions in serializers/ begin with the lines

if not _serializers:

but register_serializer and unregister_serializer do not.

When only using the built-in serializers you didn't have to manually register them but if you used an additional serializer you either have to

  1. Include it in settings.SERIALIZATION_MODULES (probably the "right way" to do things, but then why make register_serializer public?)
  2. call _load_serializers (non-public API)
  3. manually register both the built-ins and those listed in settings.SERIALIZATION_MODULES (duplicating _load_serializers)
  4. call any public function in except register_serializer or unregister_serializer before registering (strange order of operations)

To reproduce (from a fresh python shell)

from django.core import serializers
serializers.register_serializer('json2', 'django.core.serializers.json')
serializers.get_public_serializer_formats() # ['json2']
serializers.get_serializer('xml') # KeyError: 'xml'

I've attached a patch that adds the same if not _serializers: check for register_serializer and unregister_serializer. It keeps with the general mentality of the existing code (e.g., not using a separate boolean value to check for registration) but introduces an edge case where code that attempts to unregister all built-in serializers before registering it's own will effectively do nothing. e.g.

from django.core import serializers
serializers.unregister_serializer('json') # loads json/python/xml (assume no yaml), then unregisters json
serializers.unregister_serializer('python') # _serializers is now an empty dict
serializers.register_serializer('foo', '') # re-registers json/python/xml, then registers foo.

Under the existing code you'd get a KeyError when attempting to unregister json (since it hasn't been loaded yet).

Alternatively, it could be determined that register_serializer and unregister_serializer aren't stable API (they're not documented on the Serialization topic page). If that's the case it may make more sense to just prefix them with an _ and/or leave a note similar to the thread-safe notes.

Attachments (1)

serializers__init__nowarning.diff (797 bytes) - added by miker985@… 6 years ago.
Simple patch for serializers/

Download all attachments as: .zip

Change History (5)

Changed 6 years ago by miker985@…

Simple patch for serializers/

comment:1 Changed 6 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedReady for checkin

comment:2 Changed 6 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

(In [15336]) Fixed #14823 -- Corrected bootstrapping problems with register_serializers. Thanks to miker985@… for the report and draft patch.

comment:3 Changed 6 years ago by Russell Keith-Magee

A slight correctly was necessary for this patch -- as provided, there is an infinite loop, because register_serializer called _load_serializers, which calls register_serializer, and so on.

I've also taken the liberty of adding some tests.

comment:4 Changed 6 years ago by Russell Keith-Magee

(In [15339]) [1.2.X] Fixed #14823 -- Corrected bootstrapping problems with register_serializers. Thanks to miker985@… for the report and draft patch.

Backport of r15336 from trunk.

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