From 87a17640689977629a6edef9e224a3823c4c6e59 Mon Sep 17 00:00:00 2001 From: art3606 Date: Thu, 14 Nov 2019 19:46:21 +0200 Subject: [PATCH 1/5] Add tests for child relations support Fix Attacher#atomic_persist for embeded models. TODO: Fix nested atributes with cascade_callbacks support. --- lib/shrine/plugins/mongoid.rb | 4 +- test/mongoid_test.rb | 95 +++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/lib/shrine/plugins/mongoid.rb b/lib/shrine/plugins/mongoid.rb index 2b7e228..d140f1c 100644 --- a/lib/shrine/plugins/mongoid.rb +++ b/lib/shrine/plugins/mongoid.rb @@ -100,7 +100,9 @@ def mongoid_reload record_copy = record.dup record_copy.id = record.id - yield record_copy.reload + record_copy.reload unless record_copy.embedded? + + yield record_copy end # Returns true if the data attribute represents a Hash field. Used by diff --git a/test/mongoid_test.rb b/test/mongoid_test.rb index a1330ba..a304be4 100644 --- a/test/mongoid_test.rb +++ b/test/mongoid_test.rb @@ -547,4 +547,99 @@ def validate end end end + + describe "child relations support" do + before do + User = @user.class + Photo = Class.new { + include Mongoid::Document + field :title, type: String + field :image_data, type: Hash + } + Photo.include @shrine::Attachment.new(:image) + end + + after do + Object.send(:remove_const, "Photo") + Object.send(:remove_const, "User") + end + + describe "nested attributes support" do + describe "for referenced models" do + before do + Photo.store_in collection: "photos" + Photo.belongs_to :user + User.has_many :photos, dependent: :destroy + User.accepts_nested_attributes_for :photos, allow_destroy: true + end + + it "stores files for nested models" do + user = User.create!(name: "Moe") + user.update!(photos_attributes: [{ image: fakeio }]) + photo = user.photos.first + assert photo.image_data["storage"] == "store" + end + + describe "and not yet existing parent" do + it "stores files for nested models" do + user = + User.create!(name: "Moe", photos_attributes: [{ image: fakeio }]) + photo = user.photos.first + assert photo.image_data["storage"] == "store" + end + end + end + + describe "for embedded models" do + before do + Photo.embedded_in :user + User.embeds_many :photos, cascade_callbacks: true + User.accepts_nested_attributes_for :photos, allow_destroy: true + end + + it "stores files for nested models" do + user = User.create!(name: "Jacob") + user.update!(photos_attributes: [{ image: fakeio }]) + photo = user.photos.first + assert photo.image_data["storage"] == "store" + end + + describe "and not yet existing parent" do + it "stores files for nested models" do + user = User.create!(name: "Moe", + photos_attributes: [{ image: fakeio }]) + photo = user.photos.first + assert photo.image_data["storage"] == :store + end + end + end + end + + + describe "(embedded)" do + before do + Photo.embedded_in :user + User.embeds_one :photo, cascade_callbacks: true + end + + describe "Attacher" do + describe "#atomic_persist" do + it "persists the record" do + photo = Photo.new(user: @user) + attacher = @shrine::Attacher.from_model(photo, :image) + + file = attacher.attach(fakeio) + photo.save + + photo.title = "me" + attacher.atomic_persist + + assert_equal "me", photo.title + assert_equal "me", @user.reload.photo.title + assert_equal file, attacher.file + end + end + end + end + end end From 27bec0c3017eeff7dbf9871d460dc83168efa9f6 Mon Sep 17 00:00:00 2001 From: art3606 Date: Thu, 2 Jan 2020 18:16:31 +0200 Subject: [PATCH 2/5] Atomic_promote fix --- lib/shrine/plugins/mongoid.rb | 16 ++++++++++++---- test/mongoid_test.rb | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/lib/shrine/plugins/mongoid.rb b/lib/shrine/plugins/mongoid.rb index d140f1c..a361d4e 100644 --- a/lib/shrine/plugins/mongoid.rb +++ b/lib/shrine/plugins/mongoid.rb @@ -97,10 +97,18 @@ def mongoid_persist # Yields the reloaded record. Used by the _persistence plugin. def mongoid_reload - record_copy = record.dup - record_copy.id = record.id - - record_copy.reload unless record_copy.embedded? + if record.embedded? + parent_copy = record._parent.dup + parent_copy.id = record._parent.id + parent_copy.reload + record_copy = parent_copy._children.find do |child| + child.id == record.id + end + else + record_copy = record.dup + record_copy.id = record.id + record_copy.reload + end yield record_copy end diff --git a/test/mongoid_test.rb b/test/mongoid_test.rb index a304be4..fa0208c 100644 --- a/test/mongoid_test.rb +++ b/test/mongoid_test.rb @@ -639,6 +639,22 @@ def validate assert_equal file, attacher.file end end + + describe "#atomic_promote" do + it "promotes cached file to permanent storage" do + photo = Photo.new(user: @user) + attacher = @shrine::Attacher.from_model(photo, :image) + + attacher.attach_cached(fakeio) + photo.save + + attacher.atomic_promote + + assert attacher.stored? + attacher.reload + assert attacher.stored? + end + end end end end From 22b836bc2eb72c3d9c7d1dff7a16fdc46b586f29 Mon Sep 17 00:00:00 2001 From: art3606 Date: Wed, 8 Jan 2020 14:47:00 +0200 Subject: [PATCH 3/5] Fix possible bug --- lib/shrine/plugins/mongoid.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/shrine/plugins/mongoid.rb b/lib/shrine/plugins/mongoid.rb index a361d4e..1413691 100644 --- a/lib/shrine/plugins/mongoid.rb +++ b/lib/shrine/plugins/mongoid.rb @@ -102,7 +102,7 @@ def mongoid_reload parent_copy.id = record._parent.id parent_copy.reload record_copy = parent_copy._children.find do |child| - child.id == record.id + child.id == record.id && child.class == record.class end else record_copy = record.dup From b08e1dda8f520457a7178166c9982e36cb42392d Mon Sep 17 00:00:00 2001 From: Nazar Matus Date: Wed, 7 Oct 2020 20:02:42 +0300 Subject: [PATCH 4/5] Multiple fixes - Do not perform extra `persist` in after_save callback, so there's no more bugs with saving records with embedded models with shrine attachments. Call `finalize` in before_save callback now. - Support deeply nested records. - Skip reload for non-persisted records. - Add more tests. --- lib/shrine/plugins/mongoid.rb | 55 ++++++++++++++++++--------------- test/mongoid_test.rb | 57 +++++++++++++++++++++++++---------- 2 files changed, 71 insertions(+), 41 deletions(-) diff --git a/lib/shrine/plugins/mongoid.rb b/lib/shrine/plugins/mongoid.rb index 1413691..9904f22 100644 --- a/lib/shrine/plugins/mongoid.rb +++ b/lib/shrine/plugins/mongoid.rb @@ -35,10 +35,6 @@ def included(model) send(:"#{name}_attacher").send(:mongoid_before_save) end - model.after_save do - send(:"#{name}_attacher").send(:mongoid_after_save) - end - model.after_destroy do send(:"#{name}_attacher").send(:mongoid_after_destroy) end @@ -69,19 +65,13 @@ def mongoid_validate end end - # Calls Attacher#save. Called before model save. + # Calls Attacher#save and finalizes attachment. + # Called before model save. def mongoid_before_save return unless changed? save - end - - # Finalizes attachment and persists changes. Called after model save. - def mongoid_after_save - return unless changed? - finalize - persist end # Deletes attached files. Called after model destroy. @@ -89,25 +79,40 @@ def mongoid_after_destroy destroy_attached end - # Saves changes to the model instance, raising exception on validation - # errors. Used by the _persistence plugin. + # Saves changes to the model instance, skipping validation. + # Used by the _persistence plugin. def mongoid_persist record.save(validate: false) end + # Internal only + def _find_root_parent(record) + parent = record._parent + return parent unless parent.embedded? + + _find_root_parent(parent) + end + + # Internal only + def _copy_record_instance(record) + copy = record.dup + copy.id = record.id + copy + end + # Yields the reloaded record. Used by the _persistence plugin. def mongoid_reload - if record.embedded? - parent_copy = record._parent.dup - parent_copy.id = record._parent.id - parent_copy.reload - record_copy = parent_copy._children.find do |child| - child.id == record.id && child.class == record.class - end - else - record_copy = record.dup - record_copy.id = record.id - record_copy.reload + unless record.persisted? + return yield record + end + + unless record.embedded? + return yield _copy_record_instance(record).reload + end + + parent_copy = _copy_record_instance(_find_root_parent(record)).reload + record_copy = parent_copy._children.find do |child| + child.class == record.class && child.id == record.id end yield record_copy diff --git a/test/mongoid_test.rb b/test/mongoid_test.rb index fa0208c..1bbd593 100644 --- a/test/mongoid_test.rb +++ b/test/mongoid_test.rb @@ -551,11 +551,11 @@ def validate describe "child relations support" do before do User = @user.class - Photo = Class.new { + Photo = Class.new do include Mongoid::Document field :title, type: String field :image_data, type: Hash - } + end Photo.include @shrine::Attachment.new(:image) end @@ -576,21 +576,21 @@ def validate it "stores files for nested models" do user = User.create!(name: "Moe") user.update!(photos_attributes: [{ image: fakeio }]) - photo = user.photos.first + photo = user.reload.photos.first assert photo.image_data["storage"] == "store" end - describe "and not yet existing parent" do + describe "with not yet existing parent" do it "stores files for nested models" do user = User.create!(name: "Moe", photos_attributes: [{ image: fakeio }]) - photo = user.photos.first + photo = user.reload.photos.first assert photo.image_data["storage"] == "store" end end end - describe "for embedded models" do + describe "for embedded (many) models" do before do Photo.embedded_in :user User.embeds_many :photos, cascade_callbacks: true @@ -600,21 +600,44 @@ def validate it "stores files for nested models" do user = User.create!(name: "Jacob") user.update!(photos_attributes: [{ image: fakeio }]) - photo = user.photos.first + photo = user.reload.photos.first assert photo.image_data["storage"] == "store" end - describe "and not yet existing parent" do + describe "with not yet existing parent" do it "stores files for nested models" do - user = User.create!(name: "Moe", - photos_attributes: [{ image: fakeio }]) - photo = user.photos.first - assert photo.image_data["storage"] == :store + user = + User.create!(name: "Moe", photos_attributes: [{ image: fakeio }]) + photo = user.reload.photos.first + assert photo.image_data["storage"] == "store" end end end - end + describe "for embedded (one) model" do + before do + Photo.embedded_in :user + User.embeds_one :photo, cascade_callbacks: true + User.accepts_nested_attributes_for :photo, allow_destroy: true + end + + it "stores files for nested model" do + user = User.create!(name: "Jacob") + user.update!(photo_attributes: { image: fakeio }) + photo = user.reload.photo + assert photo.image_data["storage"] == "store" + end + + describe "with not yet existing parent" do + it "stores files for nested model" do + user = + User.create!(name: "Moe", photo_attributes: { image: fakeio }) + photo = user.reload.photo + assert photo.image_data["storage"] == "store" + end + end + end + end describe "(embedded)" do before do @@ -629,13 +652,15 @@ def validate attacher = @shrine::Attacher.from_model(photo, :image) file = attacher.attach(fakeio) - photo.save + photo.save! photo.title = "me" attacher.atomic_persist + photo = @user.reload.photo + attacher = @shrine::Attacher.from_model(photo, :image) + assert_equal "me", photo.title - assert_equal "me", @user.reload.photo.title assert_equal file, attacher.file end end @@ -646,7 +671,7 @@ def validate attacher = @shrine::Attacher.from_model(photo, :image) attacher.attach_cached(fakeio) - photo.save + photo.save! attacher.atomic_promote From 367c8d336c4d8c49ce9844026c0f34693cd7b942 Mon Sep 17 00:00:00 2001 From: Nazar Matus Date: Fri, 9 Oct 2020 22:50:42 +0300 Subject: [PATCH 5/5] Make finalization configurable TODO: add tests, especially for backgrounding. --- lib/shrine/plugins/mongoid.rb | 27 +++++++++++++++++++++++++-- test/mongoid_test.rb | 2 +- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/lib/shrine/plugins/mongoid.rb b/lib/shrine/plugins/mongoid.rb index 9904f22..614b187 100644 --- a/lib/shrine/plugins/mongoid.rb +++ b/lib/shrine/plugins/mongoid.rb @@ -5,12 +5,18 @@ class Shrine module Plugins module Mongoid + VALID_FINALIZE_OPTS = [nil, :before_save, :after_save].freeze + def self.load_dependencies(uploader, *) uploader.plugin :model uploader.plugin :_persistence, plugin: self end def self.configure(uploader, **opts) + unless VALID_FINALIZE_OPTS.include?(opts[:finalize]) + fail ArgumentError, "valid finalize options: #{VALID_FINALIZE_OPTS}" + end + uploader.opts[:mongoid] ||= { validations: true, callbacks: true } uploader.opts[:mongoid].merge!(opts) end @@ -35,11 +41,19 @@ def included(model) send(:"#{name}_attacher").send(:mongoid_before_save) end + model.after_save do + send(:"#{name}_attacher").send(:mongoid_after_save) + end + model.after_destroy do send(:"#{name}_attacher").send(:mongoid_after_destroy) end end + define_method :"#{name}_finalize" do + send(:"#{name}_attacher").finalize + end + define_method :reload do |*args| result = super(*args) instance_variable_set(:"@#{name}_attacher", nil) @@ -65,13 +79,22 @@ def mongoid_validate end end - # Calls Attacher#save and finalizes attachment. + # Calls Attacher#save and finalizes attachment if so configured. # Called before model save. def mongoid_before_save return unless changed? save - finalize + finalize if shrine_class.opts[:mongoid][:finalize] == :before_save + end + + # Finalizes attachment if so configured. + # Makes sense when used with the backgrounding plugin. + # Called after model save. + def mongoid_after_save + return unless changed? + + finalize if shrine_class.opts[:mongoid][:finalize] == :after_save end # Deletes attached files. Called after model destroy. diff --git a/test/mongoid_test.rb b/test/mongoid_test.rb index 1bbd593..353f79e 100644 --- a/test/mongoid_test.rb +++ b/test/mongoid_test.rb @@ -7,7 +7,7 @@ @shrine.storages[:cache] = Shrine::Storage::Memory.new @shrine.storages[:store] = Shrine::Storage::Memory.new - @shrine.plugin :mongoid + @shrine.plugin :mongoid, finalize: :before_save user_class = Class.new user_class.include Mongoid::Document