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.

Sign up for GitHub

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

Fix duping for AR KeyValue backend #126

Merged
merged 1 commit into from
Dec 3, 2017
Merged

Fix duping for AR KeyValue backend #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions lib/mobility/backends/active_record/key_value.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ def self.configure(options)
end
after_destroy :mobility_destroy_key_value_translations

module_name = "MobilityArKeyValue#{association_name.to_s.camelcase}"
unless const_defined?(module_name)
callback_methods = Module.new do
define_method :initialize_dup do |source|
super(source)
self.send("#{association_name}=", source.send(association_name).map(&:dup))
# Set inverse on associations
send(association_name).each { |translation| translation.translatable = self }
Copy link
Owner Author

@shioyama shioyama Dec 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If I leave out the spec checking that changing the attribute on one instance does not affect the value on the other instance, then this can be simplified to just:

define_method :initialize_dup do |source|
  super(source)
  self.send("#{association_name}=", source.send(association_name))
end

i.e. you don't need to dup the association objects, nor set the inverse.

However, I personally think that it's better that the dup'ed instance does not affect the original and vice versa. However, Sequel (for example) does not "deep dup" for any pattern here used in the backends... I suppose because there's a performance cost, and Sequel always defaults to not incurring any performance costs.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Never mind, we definitely want duplicate models to have their own translations with AR at least. I think this is the correct way to fix this.

end
end
include const_set(module_name, callback_methods)
end

private

# Clean up *all* leftover translations of this model, only once.
Expand Down
13 changes: 12 additions & 1 deletion spec/support/shared_examples/dup_examples.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@
save_or_raise(dupped_instance)
expect(dupped_instance.send(attribute)).to eq(instance.send(attribute))

if ENV['ORM'] == 'active_record'
# Ensure duped instances are pointing to different objects
instance_backend.write(:en, "bar")
expect(dupped_backend.read(:en)).to eq("foo")
end

# Ensure we haven't mucked with the original instance
instance.reload

Expand All @@ -35,6 +41,11 @@
save_or_raise(instance)
save_or_raise(dupped_instance)

if ENV['ORM'] == 'active_record'
instance.send("#{attribute}=", "bar")
expect(dupped_instance.send(attribute)).to eq("foo")
end

# Ensure we haven't mucked with the original instance
instance.reload
dupped_instance.reload
Expand All @@ -52,7 +63,7 @@ def save_or_raise(instance)
end

def skip_if_duping_not_implemented
if described_class < Mobility::Backends::KeyValue
if ENV['ORM'] == 'sequel' && described_class < Mobility::Backends::KeyValue
skip "Duping has not been properly implemented"
end
end
Expand Down