Opened 6 years ago

Closed 6 years ago

#29443 closed Cleanup/optimization (wontfix)

Unify registries in Django

Reported by: Johannes Maron Owned by:
Component: Utilities Version: dev
Severity: Normal Keywords: plugin, registry, pattern, utils
Cc: info@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Johannes Maron)

Django utilizes registries in various places thought the code, such as the admin, models, template-tags and the check framework.

At the moment all those places provide their own implementation. Considering that this is a recurring pattern I would suggest to unify the implementation. More specifically to provide a common super class and override or extend behavior if need be.

I would also suggest to add this to django.utils including public documentation, because this pattern could be useful for Django projects and 3rd party libraries as it is to the Django project itself.

Luckily I have written one before. I don't know if it is any good or even compatible implementations in Django but I added a lot of documentation and research. I am happy to share this here:

class Registry:
    """
    A registry to store and retrieve elements.

    This is an implementation of the "registry pattern" by Martin Fowler (ISBN: 0321127420).

    A registry can be used interact with a list of objects, without knowing them.
    It allows a package to define a common interface and to interact with its implementations
    without holding a reference to them.

    A typical use case are plugins where a package provides a feature and allows
    other packages to extend it. In this case the super package might need to interact
    with its extensions, without knowing them.

    This concept is used in places like Django's admin or template tags.

    Typing a registry can help to define a common interface, that needs to be implemented by
    all extensions.

    Usage example:

    ``mainapp/feature.py``::

        from common.utils import Registry

        class BasePlugin:

            def get_url(self):
                raise NotImplementedError()

        plugins = Registry(entry_type=BasePlugin)

        class MyFeature:

            def get_plugin_urls(self):
                return (plugin.get_url() for plugin in plugins)


    ``featureapp/feature.py``::

        from mainapp.feature import BasePlugin

        class MyPlugin(BasePlugin):

            def get_url(self):
                return "https://example.com"

    ``featureapp/app.py``::

        from django.apps import AppConfig

        from mainapp.feature import plugins

        class FeatureApp(AppConfig):
            name = 'featureapp'

            def ready(self):
                plugins.register(MyPlugin)

    In order for a plugin to be registered, the ``register`` call needs to
    be executed. A good place to do that in Django apps is in the app's
    `AppConfig.ready` method.

    Args:
        entry_type (``type``):
            The registry will only allow subclasses or instances of this
            type to be registered.
        unique (bool):
            Whether or not elements can be registered multiple times.
            Default: ``True``

    """

    def __init__(self, entry_type=None, unique=True):
        if entry_type is not None and not isinstance(entry_type, type):
            raise TypeError('"entry_type" expects a class, but got an instance.')
        self.entry_type = entry_type
        self.unique = unique
        self._register = []

    def __iter__(self):
        # The list is copied, to avoid mutation of the registry while iterating.
        return iter(list(self._register))

    def __len__(self):
        return len(self._register)

    def clear(self):
        """Clear all entries from the registry."""
        self._register.clear()

    def register(self, entry):
        """
        Store entry in the registry.

        Args:
            entry: Entry that is to be added to the registry.

        Raises:
            TypeError:
                If the entry type does not match the type of the registry instance.

            ValueError:
                If the entry is already registered and the registry instance is set to be unique.

        """
        entry_type = entry if isinstance(entry, type) else type(entry)
        if self.entry_type and not (
                isinstance(entry, self.entry_type) or issubclass(entry_type, self.entry_type)
        ):
            raise TypeError('"entry" expects %s, but got %s' % (self.entry_type, entry))
        if self.unique and entry in self._register:
            raise ValueError('"%s" is already registered.' % entry)
        self._register.append(entry)

    def unregister(self, entry):
        """
        Remove all occurrences of ``entry`` from registry.

        Args:
            entry: Entry that is to be removed from the registry.

        Raises:
            ValueError: If entry is not registered.

        """
        if entry not in self._register:
            raise ValueError('"%s" is not registered.' % entry)

        # If the registry is not unique, an element could be in the list more than once.
        while entry in self._register:
            self._register.remove(entry)

Change History (12)

comment:1 by Johannes Maron, 6 years ago

Description: modified (diff)

comment:2 by Johannes Maron, 6 years ago

Description: modified (diff)

comment:3 by Johannes Maron, 6 years ago

I also have tests for the example, but it's written in PyTest but it's a start:

class Plugin:
    pass


class TestRegistry:

    def test_init(self):
        reg = Registry()
        assert reg.unique is True
        assert reg.entry_type is None

        reg = Registry(entry_type=Plugin, unique=False)
        assert reg.unique is False
        assert reg.entry_type is Plugin

        with pytest.raises(TypeError) as e:
            Registry(entry_type=Plugin())
        assert str(e.value) == '"entry_type" expects a class, but got an instance.'

    def test_iter(self):
        reg = Registry()
        assert reg._register is not reg.__iter__()

        for i in range(5):
            reg.register(i)

        for i, entry in enumerate(reg):
            reg.register(i + 5)
            assert entry < 5

    def test_len(self):
        reg = Registry()
        assert not reg
        reg.register(1)
        assert len(reg) == 1

    def test_clear(self):
        reg = Registry()
        reg.register(1)
        assert len(reg) == 1
        reg.clear()
        assert not reg
        assert list(reg) == []

    def test_register(self):
        reg = Registry()
        reg.register(1)
        assert reg._register == [1], "1 should be in registry"

        with pytest.raises(ValueError) as e:
            reg.register(1)
        assert str(e.value) == '"1" is already registered.'
        assert reg._register.count(1) == 1, "1 is only once in the registry"

        reg = Registry(entry_type=list)
        with pytest.raises(TypeError) as e:
            reg.register(entry={})

        assert str(e.value) == '"entry" expects <class \'list\'>, but got {}'

        with pytest.raises(TypeError) as e:
            reg.register(entry=dict)

        assert str(e.value) == '"entry" expects <class \'list\'>, but got <class \'dict\'>'

    def test_unregister(self):
        reg = Registry()
        with pytest.raises(Exception) as e:
            reg.unregister(1)
        assert str(e.value) == '"1" is not registered.'
        assert not reg
        reg.register(1)
        assert len(reg) == 1
        assert 1 in reg._register
        reg.unregister(1)
        assert not reg
        assert 1 not in reg._register

        reg = Registry(unique=False)
        reg.register(1)
        reg.register(2)
        reg.register(1)

        assert list(reg) == [1, 2, 1]
        reg.unregister(1)
        assert reg._register == [2]

comment:4 by Tim Graham, 6 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Claude Paroz, 6 years ago

Interesting. Having a @register decorator would also be nice.

comment:6 by Florian Apolloner, 6 years ago

Rather than reinventing the wheel, can we use stuff like https://github.com/pytest-dev/pluggy ?

comment:7 by Johannes Maron, 6 years ago

Hi Florian,

thanks for bringing this up. I didn't check 3rd party.

IMHO this particular implementation does too much. I goes way beyond a simple registry. Replacing the existing code will be more difficult and could introduce unwanted bug.

What concerns me more is adding a dependency.

If you prefer using this package, I am happy to run it by the mailing list. I personally would avoid to use it.

Best
-Joe

comment:8 by Ana Paula Gomes, 6 years ago

Owner: changed from nobody to Ana Paula Gomes
Status: newassigned

comment:9 by Ana Paula Gomes, 6 years ago

Owner: Ana Paula Gomes removed
Status: assignednew

This ticket requires more knowledge about the codebase (which I don't have yet). But I would suggest using exceptions like AlreadyRegistered and NotRegistered in AdminSite.
For those interested in taking a look at places implementing registers, here a list of some classes:

  • Library: template/library
  • Apps: apps/registry.py
  • CheckRegistry: core/checks/registry.py
  • AdminSite: contrib/admin/sites.py

comment:10 by Claude Paroz, 6 years ago

I took a look to the Django own registry implementations, and struggled to find much code that could be factorized in a common class. The structure of the registries themselves are various: list, set, mapping, sometimes several of them. Then the implementations differ much depending on the internal registry structure.

Johannes, could you explore that a bit and show us some example of current code that would benefit from a utility class?

comment:11 by Johannes Maron, 6 years ago

Hi Claude,

yes you are absolutely right. Ana and I discovered the same problem during the springs at DjangoConEu'18.
The implementations are extremely different. Maybe a good reason to refactor it, maybe a good reason to just leave it be.
It's definitely a very difficult tasks. Kudos to whomever solves this ;)

Best
-Joe

comment:12 by Claude Paroz, 6 years ago

Resolution: wontfix
Status: newclosed

At first sight, I don't see a compelling reason to refactor the various registry implementations, as they serve different purposes.

I'll close the ticket, but if someone comes later with some POC code that demonstrates advantages to unify registry implementations, we'll happily reconsider that decision.

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