Registry (anti-)pattern

Foreman's codebase has a number of "registries", collected in app/registries/ since #19317, but (in the author's opinion) this is an anti-pattern that should be avoided.

They tend to look like this:

module FooManager
  @widgets = {}

  def self.register(key, value)
    @widgets[key] = value
  end

  def self.get(key)
    @widgets[key]
  end
end

and then:

  1. Foreman::Plugin has an API to call FooManager.register for plugin authors to extend
  2. and/or config/initializers/foreman.rb calls a method somewhere to populate initial data

Problems with registry classes

Registry classes have the potential to cause test failures as they hold state that's not reset between individual tests or test cases. As tests are run in a random order, unrelated tests will sometimes fail as bits of state are left from earlier unit tests of the registry - e.g. modification tests for a registry storing menu items might cause integration tests to fail if the default menu system has been changed.

Resetting state between tests becomes expensive, error-prone and may require additional APIs to store and reset state (e.g. Pagelets::Manager isolation in tests).

Registry classes usually can't be reloaded in development environments unless they've been designed with a lot of care. If the class is excluded from reloading (e.g. required early, or in app/registries/) then modifications require developers to restart the entire Rails application. If it's misplaced, then code reloads will cause the class to lose state and de-register items from plugins etc.

Rewriting registries

By removing existing registries and blocking the introduction of new ones, new types of test failures and interactions can be prevented, while ensuring that new features can have good unit test coverage and support reloading for development.

Some existing registries separate the storage of items from the list of default items, which are then loaded during app initialisation. These can be rewritten quite simply with a lazily populated instance or class variable, e.g.

module FooManager
  class << self
    def get(key)
      widgets[key]
    end

    private

    def widgets
      @widgets ||= begin
        {default: 'foo'}
      end
    end
  end
end

The default value of the variable can be in another class, inverting the relationship, or another class can be loaded through require_dependency and then loads data in. There are many ways of writing this that are autoloader-safe without needing to move setup into the app's initializers.

Registries that can be extended by plugins should store data in Foreman::Plugin instances and retrieve it at runtime, instead of Foreman::Plugin API methods storing data into the registry object. Foreman::Plugin is a registry itself, but keeping one registry is preferable to having state in two, and may allow for Foreman::Plugin to be refactored independently later.

module Foreman
  class Plugin
    attr_reader :widgets

    def initialize
      @widgets = {}
    end

    def register_widget(key, value)
      @widgets[key] = value
    end
  end
end

module FooManager
  class << self
    def get(key)
      widgets[key]
    end

    private

    def builtin_widgets
      @widgets ||= begin
        {default: 'foo'}
      end
    end

    def plugin_widgets
      Foreman::Plugin.all.inject({}) { |all, plugin| all.update(plugin.widgets) }
    end

    def widgets
      builtin_widgets + plugin_widgets
    end
  end
end