Open NikolayMurha opened 4 years ago
Could you provide a self-contained script that reproduces a problem? At the moment I don't quite understand what caused the problem.
Hey @janko, my old tests for embedded models prove it! :) Here: https://github.com/shrinerb/shrine-mongoid/pull/3/files#diff-e36773e2ddf368779ff45b39165f0665
@FunkyloverOne Great, which ones exactly? Could you extract the relevant code into a self-contained script that I can run (on Shrine 3.x and shrine-mongoid 1.x)?
@janko This is a new rails application with sample models and form: https://github.com/NikolayMurha/shrine_mongoid_error
@NikolayMurha Thanks, that helps. It would be ideal if there wasn't any Rails at all, but instead if it was just a script like this one. That would make it the easiest for me to debug and talk about a solution.
Thank you! 🙏 I'll make sure to look into it later today 👍
@NikolayMurha I minimized your script to the following:
require "mongoid"
require "shrine"
require "shrine/storage/memory"
Mongoid.configure do |config|
config.clients.default = {
hosts: ["localhost:27017"],
database: "shrine_mongoid_test",
}
end
Shrine.storages = {
cache: Shrine::Storage::Memory.new,
store: Shrine::Storage::Memory.new,
}
Shrine.plugin :mongoid
class Photo
include Mongoid::Document
include Shrine::Attachment[:image]
embedded_in :gallery, polymorphic: true
field :image_data, type: String
end
class Gallery
include Mongoid::Document
embeds_one :photo, class_name: Photo.to_s, autobuild: true, cascade_callbacks: true
accepts_nested_attributes_for :photo, allow_destroy: true
end
Gallery.create(photo_attributes: { image: StringIO.new("image") })
I should be able to look into it tomorrow. If you or @FunkyloverOne have any ideas on what should be done, feel free to let me know 😃
It appears that Mongoid calls the #after_save
callback on the embedded document (Photo
in this case) before the actual insert of the parent document happened (Gallery
in this case). That might make it difficult for shrine-mongoid to correctly support features like backgrounding. Thought that might not be directly related to this issue.
What I believe is happening here is the following:
Gallery.create
is called 1️⃣
Photo.after_save
Photo.after_save
promotes the attached file and persists the Photo againGalley#save
is called, which I think effectively calls Gallery.create
again 2️⃣ , as the Gallery wasn't yet persisted
Photo.after_save
again, which should now no-op on Shrine sideI think that shrine-mongoid would somehow need to know whether the embedded document is being saved standalone, or as part of the parent record save.
Yes, the order in which Mongoid executes cascading callbacks is definitely very problematic and in my opinion incorrect. If we execute the following script:
We can see that Mongoid executes actions in the following order:
Photo.before_save
Photo.after_save
Gallery.before_save
Gallery.after_save
So, Photo.after_save
is in fact executed before the photo is actually persisted, which is in my opinion incorrect behaviour. I don't see any callback we can add to Photo
which will get executed after it has been persisted, which is something that Shrine needs.
Perhaps we can add the callback to the top-most parent model, in this case Gallery
. I will investigate that option :thinking:
I'm not managing to get back to this issue. @FunkyloverOne Would you perhaps be willing to investigate this? You seem to be very knowledgeable in this area, and already had other contributions here 😃
Hey @janko, I believe what you said is what we need to do:
Perhaps we can add the callback to the top-most parent model, in this case Gallery. I will investigate that option
And yes, I will actually have time for it soon. :wink:
The only thing that bothers me now, is I kinda used to understand how Shrine 2.x worked, but now it's all so different and seems harder. That's gonna be a challenge :smile:
Yeah, some bigger changes needed to be made, but the core principles largely remained the same, mostly the code got reorganized (e.g. model integration being extracted into plugins). I guess the biggest relevant change was the persistence/backgrounding rewrite.
@janko how about 2 separate plugins in this gem?
What we have right now should be mongoid_root
, which works perfectly with full set of features for attachments in "root" (non-embedded) models.
And we could have a second one like mongoid_embedded
, which is essentially the same, but without mongoid_persist
& mongoid_reload
functionality.
@FunkyloverOne I'm fine with separating these two things. Could we perhaps make it a plugin option instead?
Or we could just add if
statements to those 2 methods, and check whether the record is embedded?
:D
# Saves changes to the model instance, raising an exception on validation
# errors. Used by the _persistence plugin.
def mongoid_persist
return if record.embedded?
record.save(validate: false)
end
# Yields the reloaded record. Used by the _persistence plugin.
def mongoid_reload
return yield record if record.embedded?
record_copy = record.dup
record_copy.id = record.id
yield record_copy.reload
end
This is misleading though:
raising exception on validation errors
Or we could raise an error there because it's more like the user's code shouldn't reach those methods at all.
Another solution is to only skip/fail mongoid_persist
if the root record is not yet persisted. ~However, I'd rather leave it for the user to take care of, as all that embedded record saving is rather "hacky".~ (it's fine) Let's just fail with an error in that specific case and proceed with saving otherwise.
As for mongoid_reload
- we still could do some similar stuff as here https://github.com/shrinerb/shrine-mongoid/pull/6
but it must look up root record recursively, and then search for an original child recursively as well (and I do not really know how it can be done yet). Seems legit to me.
Oh, and we should always skip the reload if the root record is not yet persisted!
Meanwhile, I've figured out that _children
method returns a flat array which already includes deeply nested ones, yay!
And we should only perform that fancy "reload" for an embedded record if it is already persisted within its parent.
BTW, we might use set
method for atomic writes.
Here's more info, just in case: https://docs.mongodb.com/mongoid/current/tutorials/mongoid-persistence/#atomic
@janko why do we persist
after save though?
https://github.com/shrinerb/shrine-mongoid/blob/v1.0.0/lib/shrine/plugins/mongoid.rb#L84
I've just moved the finalize
call to mongoid_before_save
, and removed the persist
call at all, and all the tests successfully passed.
Hello.
I have "E11000 duplicate key error collection: project_db.users index: id dup key..." I investigate this and noticed, that problem in embeded models with cascade_callbacks: true option. This case actual only for new models.
Because this callback called inside insert workflow: https://github.com/shrinerb/shrine-mongoid/blob/master/lib/shrine/plugins/mongoid.rb#L95 and inserted parent record before main insert executed. Thanks!
Rails 6.0.1 Mongoid 7.0.5