From 3b2356797e4f07c88c29868364fd1a606fb0c848 Mon Sep 17 00:00:00 2001 From: Chris Salzberg Date: Sat, 1 Apr 2017 22:35:36 +0900 Subject: [PATCH 1/4] Destroy all translations associated with model when it is destroyed The KeyValue backend uses a polymorphic association where key columns store attributes, and we limit the association to only the keys defined as translated attributes for the model (for performance, etc.) This has the consequence that although we may have a dependent: destroy on the model, translations which were previously stored with different keys (for example, if we changed attribute names in the model) will be left behind, because they are not seen by the association (anymore). Having them left behind is unavoidable, but at minimum we should destroy them all when the model is destroyed to avoid them begin associated with other, new models. To do this we add an after destroy callback which destroys *all* string and text translations associated with the model, regardless of their attribute name (key). This ensures that when we destroy a translated record, we are guaranteed that the entire record of the record is wiped from the database. --- .../backend/active_record/key_value.rb | 11 +++++++ lib/mobility/backend/sequel/key_value.rb | 16 ++++++++++ .../backend/active_record/key_value_spec.rb | 26 ++++++++++++++++ .../mobility/backend/sequel/key_value_spec.rb | 30 +++++++++++++++++++ 4 files changed, 83 insertions(+) diff --git a/lib/mobility/backend/active_record/key_value.rb b/lib/mobility/backend/active_record/key_value.rb index 5177b5699..5cfd1d321 100644 --- a/lib/mobility/backend/active_record/key_value.rb +++ b/lib/mobility/backend/active_record/key_value.rb @@ -85,6 +85,7 @@ def self.configure!(options) send(association_name).destroy(translation) end end + after_destroy :mobility_destroy_key_value_translations mod = Module.new do define_method :i18n do @@ -92,6 +93,16 @@ def self.configure!(options) end end extend mod + + private + + # Clean up *all* leftover translations of this model, only once. + def mobility_destroy_key_value_translations + [:string, :text].freeze.each do |type| + Mobility::ActiveRecord.const_get("#{type.capitalize}Translation".freeze). + where(translatable: self).destroy_all + end + end unless private_instance_methods(false).include?(:mobility_destroy_key_value_translations) end # @!group Cache Methods diff --git a/lib/mobility/backend/sequel/key_value.rb b/lib/mobility/backend/sequel/key_value.rb index 3d720e5f6..99275019e 100644 --- a/lib/mobility/backend/sequel/key_value.rb +++ b/lib/mobility/backend/sequel/key_value.rb @@ -99,6 +99,22 @@ def self.configure!(options) extend extension include Mobility::Sequel::ColumnChanges.new(attributes) + + private + + # Clean up *all* leftover translations of this model, only once. + def self.mobility_key_value_callbacks_module + @mobility_key_value_destroy_callbacks_module ||= Module.new do + def after_destroy + super + [:string, :text].freeze.each do |type| + Mobility::Sequel.const_get("#{type.capitalize}Translation".freeze). + where(translatable_id: id, translatable_type: self.class.name).destroy + end + end + end + end unless respond_to?(:mobility_key_value_callbacks_module, true) + include mobility_key_value_callbacks_module end # @!group Cache Methods diff --git a/spec/mobility/backend/active_record/key_value_spec.rb b/spec/mobility/backend/active_record/key_value_spec.rb index 0bf96bf35..079af246b 100644 --- a/spec/mobility/backend/active_record/key_value_spec.rb +++ b/spec/mobility/backend/active_record/key_value_spec.rb @@ -236,6 +236,32 @@ end end + describe "after destroy" do + # In case we change the translated attributes on a model, we need to make + # sure we clean them up when the model is destroyed. + it "cleans up all associated translations, regardless of key" do + article = Article.create(title: "foo title", content: "foo content") + Mobility.with_locale(:ja) { article.update_attributes(title: "あああ", content: "ばばば") } + article.save + expect(Mobility::ActiveRecord::TextTranslation.count).to eq(4) + + Mobility::ActiveRecord::TextTranslation.create!(translatable: article, key: "key1", value: "value1", locale: "de") + Mobility::ActiveRecord::StringTranslation.create!(translatable: article, key: "key2", value: "value2", locale: "fr") + expect(Mobility::ActiveRecord::TextTranslation.count).to eq(5) + expect(Mobility::ActiveRecord::StringTranslation.count).to eq(1) + + article.destroy! + expect(Mobility::ActiveRecord::TextTranslation.count).to eq(0) + expect(Mobility::ActiveRecord::StringTranslation.count).to eq(0) + end + + it "only destroys translations once when cleaning up" do + article = Article.create(title: "foo title", content: "foo content") + expect(article).to receive(:mobility_destroy_key_value_translations).once.and_call_original + article.destroy! + end + end + describe ".configure!" do it "sets association_name and class_name from string type" do options = { type: :string } diff --git a/spec/mobility/backend/sequel/key_value_spec.rb b/spec/mobility/backend/sequel/key_value_spec.rb index e357d2bf8..e6ea0e8ea 100644 --- a/spec/mobility/backend/sequel/key_value_spec.rb +++ b/spec/mobility/backend/sequel/key_value_spec.rb @@ -272,6 +272,36 @@ def after_save end end + describe "after destroy" do + # In case we change the translated attributes on a model, we need to make + # sure we clean them up when the model is destroyed. + it "cleans up all associated translations, regardless of key" do + article = Article.create(title: "foo title", content: "foo content") + Mobility.with_locale(:ja) { article.update(title: "あああ", content: "ばばば") } + article.save + expect(Mobility::Sequel::TextTranslation.count).to eq(4) + + Mobility::Sequel::TextTranslation.create(translatable: article, key: "key1", value: "value1", locale: "de") + Mobility::Sequel::StringTranslation.create(translatable: article, key: "key2", value: "value2", locale: "fr") + expect(Mobility::Sequel::TextTranslation.count).to eq(5) + expect(Mobility::Sequel::StringTranslation.count).to eq(1) + + article.destroy + expect(Mobility::Sequel::TextTranslation.count).to eq(0) + expect(Mobility::Sequel::StringTranslation.count).to eq(0) + end + + it "only destroys translations once when cleaning up" do + article = Article.create(title: "foo title", content: "foo content") + # This is an ugly way to check that we are not destroying all + # translations twice. Since the actual callback is included in a module, + # it's hard to get at this directly. + expect(Mobility::Sequel::TextTranslation).to receive(:where).once.and_call_original + expect(Mobility::Sequel::StringTranslation).to receive(:where).once.and_call_original + article.destroy + end + end + describe ".configure!" do it "sets association_name and class_name from string type" do options = { type: :string } From 3caaac22bef49e6455e1040baeb93f373a6283f4 Mon Sep 17 00:00:00 2001 From: Chris Salzberg Date: Sun, 2 Apr 2017 11:12:05 +0900 Subject: [PATCH 2/4] Freeze some more literals --- lib/mobility/backend/active_record/key_value.rb | 4 +++- lib/mobility/backend/sequel/key_value.rb | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/mobility/backend/active_record/key_value.rb b/lib/mobility/backend/active_record/key_value.rb index 5cfd1d321..3a6631784 100644 --- a/lib/mobility/backend/active_record/key_value.rb +++ b/lib/mobility/backend/active_record/key_value.rb @@ -1,3 +1,5 @@ +# frozen-string-literal: true + module Mobility module Backend =begin @@ -54,7 +56,7 @@ def write(locale, value, **_) def self.configure!(options) super type = options[:type] - options[:class_name] ||= Mobility::ActiveRecord.const_get("#{type.capitalize}Translation") + options[:class_name] ||= Mobility::ActiveRecord.const_get("#{type.capitalize}Translation".freeze) options[:class_name] = options[:class_name].constantize if options[:class_name].is_a?(String) options[:association_name] ||= options[:class_name].table_name.to_sym %i[type association_name].each { |key| options[key] = options[key].to_sym } diff --git a/lib/mobility/backend/sequel/key_value.rb b/lib/mobility/backend/sequel/key_value.rb index 99275019e..920bb916f 100644 --- a/lib/mobility/backend/sequel/key_value.rb +++ b/lib/mobility/backend/sequel/key_value.rb @@ -1,3 +1,5 @@ +# frozen-string-literal: true + module Mobility module Backend =begin @@ -52,7 +54,7 @@ def self.configure!(options) super raise CacheRequired, "Cache required for Sequel::KeyValue backend" if options[:cache] == false type = options[:type] -options[:class_name] ||= Mobility::Sequel.const_get("#{type.capitalize}Translation") + options[:class_name] ||= Mobility::Sequel.const_get("#{type.capitalize}Translation".freeze) options[:class_name] = options[:class_name].constantize if options[:class_name].is_a?(String) options[:association_name] ||= options[:class_name].table_name.to_sym %i[type association_name].each { |key| options[key] = options[key].to_sym } From 57482d5859ab61b91e7f00351f08e63b7a9556d4 Mon Sep 17 00:00:00 2001 From: Chris Salzberg Date: Sun, 2 Apr 2017 11:32:12 +0900 Subject: [PATCH 3/4] Remove dependent: :destroy since it is now redundant --- lib/mobility/backend/active_record/key_value.rb | 1 - lib/mobility/backend/sequel/key_value.rb | 2 -- 2 files changed, 3 deletions(-) diff --git a/lib/mobility/backend/active_record/key_value.rb b/lib/mobility/backend/active_record/key_value.rb index 3a6631784..b8485d69e 100644 --- a/lib/mobility/backend/active_record/key_value.rb +++ b/lib/mobility/backend/active_record/key_value.rb @@ -79,7 +79,6 @@ def self.configure!(options) has_many association_name, ->{ where key: association_attributes }, as: :translatable, class_name: translations_class, - dependent: :destroy, inverse_of: :translatable, autosave: true before_save do diff --git a/lib/mobility/backend/sequel/key_value.rb b/lib/mobility/backend/sequel/key_value.rb index 920bb916f..60dd9d2e7 100644 --- a/lib/mobility/backend/sequel/key_value.rb +++ b/lib/mobility/backend/sequel/key_value.rb @@ -79,8 +79,6 @@ def self.configure!(options) clearer: proc { send(:"#{association_name}_dataset").update(translatable_id: nil, translatable_type: nil) }, class: translations_class - plugin :association_dependencies, association_name => :destroy - callback_methods = Module.new do define_method :before_save do super() From d0c0c4104be11d4635b9f51ac6b7a0e9b8a1ce63 Mon Sep 17 00:00:00 2001 From: Chris Salzberg Date: Sun, 2 Apr 2017 21:59:22 +0900 Subject: [PATCH 4/4] Ensure that translations of other models are not destroyed in spec --- .../backend/active_record/key_value_spec.rb | 15 ++++++++++----- spec/mobility/backend/sequel/key_value_spec.rb | 15 ++++++++++----- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/spec/mobility/backend/active_record/key_value_spec.rb b/spec/mobility/backend/active_record/key_value_spec.rb index 079af246b..08171eb5c 100644 --- a/spec/mobility/backend/active_record/key_value_spec.rb +++ b/spec/mobility/backend/active_record/key_value_spec.rb @@ -243,16 +243,21 @@ article = Article.create(title: "foo title", content: "foo content") Mobility.with_locale(:ja) { article.update_attributes(title: "あああ", content: "ばばば") } article.save - expect(Mobility::ActiveRecord::TextTranslation.count).to eq(4) + + # Create translations on another model, to check they do not get destroyed + Post.create(title: "post title", content: "post content") + + expect(Mobility::ActiveRecord::StringTranslation.count).to eq(1) + expect(Mobility::ActiveRecord::TextTranslation.count).to eq(5) Mobility::ActiveRecord::TextTranslation.create!(translatable: article, key: "key1", value: "value1", locale: "de") Mobility::ActiveRecord::StringTranslation.create!(translatable: article, key: "key2", value: "value2", locale: "fr") - expect(Mobility::ActiveRecord::TextTranslation.count).to eq(5) - expect(Mobility::ActiveRecord::StringTranslation.count).to eq(1) + expect(Mobility::ActiveRecord::TextTranslation.count).to eq(6) + expect(Mobility::ActiveRecord::StringTranslation.count).to eq(2) article.destroy! - expect(Mobility::ActiveRecord::TextTranslation.count).to eq(0) - expect(Mobility::ActiveRecord::StringTranslation.count).to eq(0) + expect(Mobility::ActiveRecord::TextTranslation.count).to eq(1) + expect(Mobility::ActiveRecord::StringTranslation.count).to eq(1) end it "only destroys translations once when cleaning up" do diff --git a/spec/mobility/backend/sequel/key_value_spec.rb b/spec/mobility/backend/sequel/key_value_spec.rb index e6ea0e8ea..db1586ce1 100644 --- a/spec/mobility/backend/sequel/key_value_spec.rb +++ b/spec/mobility/backend/sequel/key_value_spec.rb @@ -279,16 +279,21 @@ def after_save article = Article.create(title: "foo title", content: "foo content") Mobility.with_locale(:ja) { article.update(title: "あああ", content: "ばばば") } article.save - expect(Mobility::Sequel::TextTranslation.count).to eq(4) + + # Create translations on another model, to check they do not get destroyed + Post.create(title: "post title", content: "post content") + + expect(Mobility::Sequel::StringTranslation.count).to eq(1) + expect(Mobility::Sequel::TextTranslation.count).to eq(5) Mobility::Sequel::TextTranslation.create(translatable: article, key: "key1", value: "value1", locale: "de") Mobility::Sequel::StringTranslation.create(translatable: article, key: "key2", value: "value2", locale: "fr") - expect(Mobility::Sequel::TextTranslation.count).to eq(5) - expect(Mobility::Sequel::StringTranslation.count).to eq(1) + expect(Mobility::Sequel::TextTranslation.count).to eq(6) + expect(Mobility::Sequel::StringTranslation.count).to eq(2) article.destroy - expect(Mobility::Sequel::TextTranslation.count).to eq(0) - expect(Mobility::Sequel::StringTranslation.count).to eq(0) + expect(Mobility::Sequel::TextTranslation.count).to eq(1) + expect(Mobility::Sequel::StringTranslation.count).to eq(1) end it "only destroys translations once when cleaning up" do