Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add application settings #1029

Open
wants to merge 35 commits into
base: unstable
from
Open

Conversation

@timriley
Copy link
Member

timriley commented Feb 13, 2020

This PR was built in collaboration with @andrewcroome — thank you Andrew!

Add "application settings" as a first-class concept within Hanami 2.0 applications.

Settings are defined using their own block DSL, which by convention will live in config/settings.rb and will look like this:

Hanami.application.settings do
  setting :base_url
  setting :sessions_secret
end

Setting definitions also support a second argument, which should be a callable "type" object used to determine the validity of the setting value. This means it will work very nicely with dry-types:

lib/test_app/types.rb:

require "dry/types"

module TestApp
  module Types
    include Dry.Types()
  end
end

config/settings.rb:

Hanami.application.settings do
  setting :base_url, TestApp::Types::String
  setting :sessions_secret, TestApp::Types::String
  setting :some_feature_flag, TestApp::Types::Params::Bool.optional.default(true)
end

If any of the setting values do not match their type expectations, the settings loading will fail and return a helpful error message containing the details of all the failed settings, e.g.

Hanami::Application::Settings::Loader::InvalidSettingsError:
  Could not initialize settings. The following settings were invalid:

  numeric_setting: invalid value for Integer(): "hello"
  feature_flag: maybe cannot be coerced to false

Settings are loaded by default when the Hanami application inits.

However, they will also be loaded lazily, on first access, if accessed before the application inits. This makes it possible to use those settings when configuring the Hanami app itself, e.g.

module TestApp
  class Application < Hanami::Application
    config.sessions = :cookie, {secret: settings.sessions_secret}
  end
end

As this also demonstrates, loading the settings creates a struct object with methods for each of the defined settings.

This object is also available as Hanami.application.settings, and also registered in the application container as :settings, which makes it possible to auto-inject the settings as a dependency of other objects in the application.

The settings loading is also flexible, with the following being configurable:

  • settings_path - defaults to "config/settings")
  • settings_loader - the class to use to load the settings, defaults to Hanami::Application::Settings::Loader, which loads dotenv (if available), fetches the settings from ENV, and then provides the type checking behaviour
  • settings_loader_options - arguments to pass to the settings_loader when it is initialized

These configs may not be used much, but they do provide a valuable escape hatch in the case of unusual settings loading requirements.

andrewcroome and others added 30 commits Nov 30, 2019
It should be permissible to have an Hanami application without these
This is necessary for its auto-resolution of components to work
This is helpful for using types with settings, e.g.

setting :precompiled_assets, Types::Params::Bool.optional.default(false)

Without `Undefined` being passed, `nil` would be accepted as the value for this setting, which is not the intended outcome.
This behavior was introduced in dry-rb/dry-system#134 and released in 0.14.0.

Longer-term, we'll probably want to do both of the following:

- Further adjust the dry-system behaviour: revert the raising behavior? Or disable the raising behavior for auto_register dirs specified by config? Or convert the raise into a warning instead?
- Make it possible to tweak container configs early on in the hanami boot cycle, so custom configs can be applied (in this case, to set the auto_register config to []
@solnic

This comment has been minimized.

Copy link
Member

solnic commented on e75bd11 Feb 12, 2020

I don't think it's a problem to be honest. It is a good thing to have these directories there anyway.

timriley added 3 commits Feb 13, 2020
This ensures it continues to be possible to use the settings to configure the Hanami::Application subclass.

What it also enables, however, is the ability to _first_ configure the settings_loader before accessing the settings, in the case of a custom settings loader being required.
dry-system now raises an error when auto-register paths are missing
def settings(&block) # rubocop:disable Metrics/MethodLength
if block
@_settings = Application::Settings.build(
configuration.settings_loader,
configuration.settings_loader_options,
&block
)
elsif instance_variable_defined?(:@_settings)
@_settings
else
# Load settings lazily so they can be used to configure the
# Hanami::Application subclass (before the application has inited)
load_settings
@_settings ||= nil
end
end
Comment on lines +143 to +158

This comment has been minimized.

Copy link
@timriley

timriley Feb 13, 2020

Author Member

FWIW I couldn't follow the @_mutex.synchronize do ... end dance that is followed in the .routes method in this same class, because in the else case above, running load_settings will most likely result in the .settings method being called again when config/settings.rb is loaded. This results in a deadlock if the code in this method is wrapped in the synchronize block.

The particular behaviour in that else case is important for the lazy loading of settings upon first access.

And look, while I'm here, I'll be honest: I don't really understand why we're doing the synchronizing of things like the @_routes assignment inside this class... what is it actually in aid of? And if it's really necessary, how can we establish similar protections here?

This could be useful in the end (see usage in soundeck)
@timriley

This comment has been minimized.

Copy link
Member Author

timriley commented Feb 17, 2020

FYI: I plan to take another pass over this. After discussing with @solnic, there's a good chance we could use dry-configurable to provide a lot of what we've implemented here manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.