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

Raise an exception if invalid options are passed to Translations initializer #457

Merged
merged 7 commits into from
Nov 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
([#454](https://github.com/shioyama/mobility/pull/454))
- Instance exec configure block if it takes no arguments
([#456](https://github.com/shioyama/mobility/pull/456))
- Raise an exception if invalid options are passed to Translations initializer
([#457](https://github.com/shioyama/mobility/pull/457/files))

## 1.0.0.alpha (pre-release)

Expand Down
5 changes: 5 additions & 0 deletions lib/mobility/backend.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ def self.included(base)

# Defines setup hooks for backend to customize model class.
module ClassMethods
# @return [Array] Valid option keys for this backend
def valid_keys
[]
end

# Assign block to be called on model class.
# @yield [attribute_names, options]
# @note When called multiple times, setup blocks will be appended
Expand Down
4 changes: 4 additions & 0 deletions lib/mobility/backends/active_record/container.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ class ActiveRecord::Container
# @return [Symbol] (:translations) Name of translations column
option_reader :column_name

def self.valid_keys
[:column_name]
end

# @!group Backend Accessors
#
# @note Translation may be a string, integer, boolean, hash or array
Expand Down
4 changes: 4 additions & 0 deletions lib/mobility/backends/active_record/serialized.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ class ActiveRecord::Serialized
include ActiveRecord
include HashValued

def self.valid_keys
super + [:format]
end

# @!group Backend Configuration
# @param (see Backends::Serialized.configure)
# @option (see Backends::Serialized.configure)
Expand Down
4 changes: 4 additions & 0 deletions lib/mobility/backends/hash_valued.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ def self.included(backend_class)
end

module ClassMethods
def valid_keys
[:column_prefix, :column_suffix]
end

def configure(options)
options[:column_affix] = "#{options[:column_prefix]}%s#{options[:column_suffix]}"
end
Expand Down
2 changes: 1 addition & 1 deletion lib/mobility/backends/jsonb.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module Backends

==Backend Options

===+prefix+ and +suffix+
===+column_prefix+ and +column_suffix+

Prefix and suffix to add to attribute name to generate jsonb column name.

Expand Down
4 changes: 4 additions & 0 deletions lib/mobility/backends/key_value.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ def self.included(backend_class)
end

module ClassMethods
def valid_keys
[:type, :association_name, :class_name]
end

# @!group Backend Configuration
# @option options [Symbol,String] type Column type to use
# @option options [Symbol] association_name (:<type>_translations) Name
Expand Down
4 changes: 4 additions & 0 deletions lib/mobility/backends/sequel/container.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ class Sequel::Container
# @return [Symbol] (:translations) Name of translations column
option_reader :column_name

def self.valid_keys
[:column_name]
end

# @!group Backend Accessors
#
# @note Translation may be a string, integer, boolean, hash or array
Expand Down
4 changes: 4 additions & 0 deletions lib/mobility/backends/sequel/serialized.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class Sequel::Serialized
include Sequel
include HashValued

def self.valid_keys
super + [:format]
end

# @!group Backend Configuration
# @param (see Backends::Serialized.configure)
# @option (see Backends::Serialized.configure)
Expand Down
4 changes: 4 additions & 0 deletions lib/mobility/backends/table.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ def self.included(backend)
end

module ClassMethods
def valid_keys
[:association_name, :subclass_name, :foreign_key, :table_name]
end

# Apply custom processing for cache plugin
def include_cache
include self::Cache
Expand Down
22 changes: 21 additions & 1 deletion lib/mobility/pluggable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ def plugins(&block)
Plugin.configure(self, defaults, &block)
end

def included_plugins
included_modules.grep(Plugin)
end

def defaults
@defaults ||= {}
end
Expand All @@ -28,9 +32,25 @@ def inherited(klass)
end

def initialize(*, **options)
@options = self.class.defaults.merge(options)
initialize_options(options)
validate_options(@options)
end

attr_reader :options

private

def initialize_options(options)
@options = self.class.defaults.merge(options)
end

# This is overridden by backend plugin to exclude mixed-in backend options.
def validate_options(options)
plugin_keys = self.class.included_plugins.map { |p| Plugins.lookup_name(p) }
extra_keys = options.keys - plugin_keys
raise InvalidOptionKey, "No plugin configured for these keys: #{extra_keys.join(', ')}." unless extra_keys.empty?
end

class InvalidOptionKey < Error; end
end
end
47 changes: 35 additions & 12 deletions lib/mobility/plugins/backend.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,13 @@ module Backend
def initialize(*args, **original_options)
super

case options[:backend]
when String, Symbol, Class
@backend, @backend_options = options[:backend], options
when Array
@backend, @backend_options = options[:backend]
@backend_options = @backend_options.merge(original_options)
when NilClass
@backend = @backend_options = nil
else
raise ArgumentError, "backend must be either a backend name, a backend class, or a two-element array"
# Validate that the default backend from config has valid keys
if (default = self.class.defaults[:backend])
name, backend_options = default
extra_keys = backend_options.keys - backend.valid_keys
raise InvalidOptionKey, "These are not valid #{name} backend keys: #{extra_keys.join(', ')}." unless extra_keys.empty?
end

@backend = load_backend(backend)

include InstanceMethods
end

Expand Down Expand Up @@ -83,6 +76,34 @@ def load_backend(backend)
raise e, "could not find a #{backend} backend. Did you forget to include an ORM plugin like active_record or sequel?"
end

private

# Override to extract backend options from options hash.
def initialize_options(original_options)
super

case options[:backend]
when String, Symbol, Class
@backend, @backend_options = options[:backend], options
when Array
@backend, @backend_options = options[:backend]
@backend_options = @backend_options.merge(original_options)
when NilClass
@backend = @backend_options = nil
else
raise ArgumentError, "backend must be either a backend name, a backend class, or a two-element array"
end

@backend = load_backend(backend)
end

# Override default validation to exclude backend options, which may be
# mixed in with plugin options.
def validate_options(options)
return super unless backend
super(options.slice(*(options.keys - backend.valid_keys)))
end

# Override default argument-handling in DSL to store kwargs passed along
# with plugin name.
def self.configure_default(defaults, key, *args, **kwargs)
Expand Down Expand Up @@ -131,6 +152,8 @@ def mobility_backend_classes
@mobility_backend_classes ||= {}
end
end

class InvalidOptionKey < Error; end
end

register_plugin(:backend, Backend)
Expand Down
4 changes: 2 additions & 2 deletions spec/integration/active_record_compatibility_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@
include Helpers::Plugins
include Helpers::Translates
# Enable all plugins that are enabled by default pre v1.0
plugins :active_record, :reader, :writer, :cache, :dirty, :presence, :query, :attribute_methods
plugins :active_record, :reader, :writer, :cache, :dirty, :presence, :query, :attribute_methods, :fallbacks
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, this PR highlighted that the spec here was not actually enabling the fallbacks plugin, although specs were testing fallbacks stuff. So basically those tests were doing nothing useful 🤦


before { stub_const 'Post', Class.new(ActiveRecord::Base) }

describe "#assign_attributes" do
before { translates Post, :title, backend: :key_value, type: :string }
before { translates Post, :title, backend: :key_value, type: :string, fallbacks: false }
let!(:post) { Post.create(title: "foo title") }

it "assigns translated attributes" do
Expand Down
11 changes: 10 additions & 1 deletion spec/mobility/pluggable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,25 @@
include Helpers::PluginSetup

describe "#initialize" do
define_plugins(:foo)
define_plugins(:foo, :other)

it "merges defaults into @options when initializing" do
klass = Class.new(described_class)

klass.plugin :foo, 'bar'
klass.plugin :other

pluggable = klass.new(other: 'param')
expect(pluggable.options).to eq(foo: 'bar', other: 'param')
end

it "raises InvalidOptionKey error if no plugin is configured for an options key" do
klass = Class.new(described_class)

expect {
klass.new(foo: 'foo', bar: 'bar')
}.to raise_error(Mobility::Pluggable::InvalidOptionKey, "No plugin configured for these keys: foo, bar.")
end
end

describe "subclassing" do
Expand Down
44 changes: 42 additions & 2 deletions spec/mobility/plugins/backend_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@
plugins :backend
plugin_setup

define_plugins :foo

# Override default helper-defined model_class which has attributes module
# pre-included, so we can explicitly include it in specs.
let(:model_class) { Class.new }
before do
# Define 'foo' as a valid option key for this backend
allow(backend_class).to receive(:valid_keys).and_return([:foo])
end

describe "#included" do
it "calls build_subclass on backend class with options merged with default options" do
Expand Down Expand Up @@ -99,7 +105,7 @@
describe "#mobility_backends" do
it "returns instance of backend for attribute" do
mod1 = translations_class.new("title", backend: :null)
mod2 = translations_class.new("content", backend: :null, foo: :bar)
mod2 = translations_class.new("content", backend: :null)
model_class.include mod1
model_class.include mod2
instance1 = model_class.new
Expand Down Expand Up @@ -151,6 +157,26 @@
end
end

describe "#initialize" do
define_plugins :foo

it "excludes backend options from plugin key validation" do
klass = Class.new(Mobility::Pluggable)
backend_class = Class.new
def backend_class.valid_keys
[:bar]
end
klass.plugin :backend, backend_class
klass.plugin :foo

expect { klass.new(foo: 'foo', bar: 'bar') }.not_to raise_error

expect {
klass.new(foo: 'foo', other: 'other')
}.to raise_error(Mobility::Pluggable::InvalidOptionKey)
end
end

describe "defining attribute backend on model" do
before { model_class.include translations_class.new("title", backend: backend_class, foo: "bar") }

Expand Down Expand Up @@ -181,7 +207,8 @@

describe "configuring defaults" do
before do
stub_const("FooBackend", Class.new(Mobility::Backends::Null))
backend_class = stub_const("FooBackend", Class.new(Mobility::Backends::Null))
expect(backend_class).to receive(:valid_keys).twice.and_return([:association_name])
Mobility::Backends.register_backend(:foo, FooBackend)
end
after { Mobility::Backends.instance_variable_get(:@backends).delete(:foo) }
Expand All @@ -201,6 +228,19 @@
end
end

describe "default with invalid backend option" do
plugins do
backend :foo, invalid_option: :bar
end

it "raises exception when instantiated" do
expect {
translations_class.new("title")
}.to raise_error(Mobility::Plugins::Backend::InvalidOptionKey,
"These are not valid foo backend keys: invalid_option.")
end
end

describe "default with backend options" do
plugins do
backend :foo, association_name: :bar
Expand Down
2 changes: 1 addition & 1 deletion spec/mobility/plugins/sequel/dirty_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ def values
ArticleWithFallbacks.class_eval do
dataset = DB[:articles]
end
translates ArticleWithFallbacks, :title, backend: backend_class, dirty: true, cache: false, fallbacks: { en: 'ja' }
translates ArticleWithFallbacks, :title, backend: backend_class, dirty: true, fallbacks: { en: 'ja' }
end

it "does not compare with fallback value" do
Expand Down
Loading