From 6dbb8844e5852e6d119abdfed644111519dc92b2 Mon Sep 17 00:00:00 2001 From: Chris Salzberg Date: Sun, 8 Nov 2020 22:12:06 +0900 Subject: [PATCH] Refactor inheritance in attributes and backend plugins - removes register methods which we don't need - freezes attributes/backends after inheritance, to ensure they are not modified later --- lib/mobility/plugins/attributes.rb | 30 ++++++++------- lib/mobility/plugins/backend.rb | 14 +++---- spec/mobility/plugins/attributes_spec.rb | 47 ++++++++++++++++++++++++ spec/mobility/plugins/backend_spec.rb | 12 ++++++ 4 files changed, 83 insertions(+), 20 deletions(-) diff --git a/lib/mobility/plugins/attributes.rb b/lib/mobility/plugins/attributes.rb index 28479f549..142a6e9ff 100644 --- a/lib/mobility/plugins/attributes.rb +++ b/lib/mobility/plugins/attributes.rb @@ -32,8 +32,15 @@ def inspect end included_hook do |klass| - klass.extend ClassMethods - @names.each { |name| klass.register_mobility_attribute(name) } + names = @names + + klass.class_eval do + extend ClassMethods + names.each { |name| mobility_attributes << name.to_s } + mobility_attributes.uniq! + rescue FrozenError + raise FrozenAttributesError, "Attempting to translate these attributes on #{klass}, which has already been subclassed: #{names.join(', ')}." + end end module ClassMethods @@ -44,23 +51,20 @@ def mobility_attribute?(name) mobility_attributes.include?(name.to_s) end - # Register a new attribute name. Public, but treat as internal. - # @param [String, Symbol] Attribute name - def register_mobility_attribute(name) - (self.mobility_attributes << name.to_s).uniq! - end - - def inherited(klass) - super - mobility_attributes.each { |name| klass.register_mobility_attribute(name) } - end - # Return translated attribute names on this model. # @return [Array] Attribute names def mobility_attributes @mobility_attributes ||= [] end + + def inherited(klass) + super + attrs = mobility_attributes.freeze # ensure attributes are not modified after being inherited + klass.class_eval { @mobility_attributes = attrs.dup } + end end + + class FrozenAttributesError < Error; end end register_plugin(:attributes, Attributes) diff --git a/lib/mobility/plugins/backend.rb b/lib/mobility/plugins/backend.rb index d971ff619..32aec1531 100644 --- a/lib/mobility/plugins/backend.rb +++ b/lib/mobility/plugins/backend.rb @@ -56,8 +56,11 @@ def included(klass) backend_class.setup_model(klass, names) - @names.each do |name| - klass.register_mobility_backend_class(name, @backend_class) + names = @names + backend_class = @backend_class + + klass.class_eval do + names.each { |name| mobility_backend_classes[name.to_sym] = backend_class } end backend_class @@ -137,12 +140,9 @@ def mobility_backend_class(name) raise KeyError, "No backend for: #{name}" end - def register_mobility_backend_class(name, backend_class) - mobility_backend_classes[name.to_sym] = backend_class - end - def inherited(klass) - klass.mobility_backend_classes.merge!(@mobility_backend_classes) + parent_classes = mobility_backend_classes.freeze # ensure backend classes are not modified after being inherited + klass.class_eval { @mobility_backend_classes = parent_classes.dup } super end diff --git a/spec/mobility/plugins/attributes_spec.rb b/spec/mobility/plugins/attributes_spec.rb index a2256575d..92356e87c 100644 --- a/spec/mobility/plugins/attributes_spec.rb +++ b/spec/mobility/plugins/attributes_spec.rb @@ -22,6 +22,18 @@ end end + describe ".mobility_attributes" do + it "returns attributes included in one of multiple pluggable modules" do + translations1 = translations_class.new("title", "content") + translations2 = translations_class.new("author") + klass = Class.new + klass.include translations1 + klass.include translations2 + + expect(klass.mobility_attributes).to eq(%w[title content author]) + end + end + describe ".mobility_attribute?" do it "returns true if name is an attribute" do translations = translations_class.new("title", "content") @@ -46,4 +58,39 @@ expect(klass.mobility_attribute?("foo")).to eq(false) end end + + describe "inheritance" do + it "inherits mobility attributes from parent" do + mod1 = translations_class.new("title", "content") + klass1 = Class.new + klass1.include mod1 + + klass2 = Class.new(klass1) + + expect(klass1.mobility_attributes).to match_array(%w[title content]) + expect(klass2.mobility_attributes).to match_array(%w[title content]) + + mod2 = translations_class.new("author") + klass2.include mod2 + + expect(klass1.mobility_attributes).to match_array(%w[title content]) + expect(klass2.mobility_attributes).to match_array(%w[title content author]) + end + + it "freezes inherited attributes to ensure they are not changed after subclassing" do + mod1 = translations_class.new("title") + stub_const("Foo", Class.new) + klass1 = Foo + klass1.include mod1 + + Class.new(klass1) + expect(klass1.mobility_attributes).to be_frozen + + mod2 = translations_class.new("content") + expect { + klass1.include mod2 + }.to raise_error(Mobility::Plugins::Attributes::FrozenAttributesError, + "Attempting to translate these attributes on Foo, which has already been subclassed: content.") + end + end end diff --git a/spec/mobility/plugins/backend_spec.rb b/spec/mobility/plugins/backend_spec.rb index d42ddb179..08ef37585 100644 --- a/spec/mobility/plugins/backend_spec.rb +++ b/spec/mobility/plugins/backend_spec.rb @@ -100,6 +100,18 @@ content_backend_class = model_subclass.mobility_backend_class("content") expect(content_backend_class).to be <(backend_class_2) end + + it "freezes backends after subclassing" do + backend_class_1 = Class.new + backend_class_1.include Mobility::Backend + + mod1 = translations_class.new("title", backend: backend_class_1) + model_class.include mod1 + + Class.new(model_class) + + expect(model_class.send(:mobility_backend_classes)).to be_frozen + end end describe "#mobility_backends" do