E11000 duplicate key error collection

[NikolayMurha]

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

[janko]

Could you provide a self-contained script that reproduces a problem? At the moment I don't quite understand what caused the problem.

[flvrone]

Hey @janko, my old tests for embedded models prove it! :)
Here: https://github.com/shrinerb/shrine-mongoid/pull/3/files#diff-e36773e2ddf368779ff45b39165f0665

[janko]

@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)?

[NikolayMurha]

@janko
This is a new rails application with sample models and form:
https://github.com/NikolayMurha/shrine_mongoid_error

[janko]

@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.

[NikolayMurha]

@janko https://gist.github.com/NikolayMurha/e18042d513809ec201fe331a254c0970

[janko]

Thank you! 🙏 I'll make sure to look into it later today 👍

[janko]

@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 😃

[janko]

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️⃣
  • this firstly triggers Photo.after_save
  • Photo.after_save promotes the attached file and persists the Photo again
  • Galley#save is called, which I think effectively calls Gallery.create again 2️⃣ , as the Gallery wasn't yet persisted
    • this triggers Photo.after_save again, which should now no-op on Shrine side
    • Gallery document is inserted 2️⃣
  • Gallery document is inserted 1️⃣

I 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.

[janko]

Yes, the order in which Mongoid executes cascading callbacks is definitely very problematic and in my opinion incorrect. If we execute the following script:

Script

require "mongoid"

Mongoid.configure do |config|
  config.clients.default = {
    hosts: ["localhost:27017"],
    database: "shrine_mongoid_test",
  }
end

class Photo
  include Mongoid::Document
  before_save { puts "====== before_save Photo" }
  after_save { puts "====== after_save Photo" }
  embedded_in :gallery, polymorphic: true

  field :title, type: String
end


class Gallery
  include Mongoid::Document
  before_save { puts "====== before_save Gallery" }
  after_save { puts "====== after_save Gallery" }

  embeds_one :photo, class_name: Photo.to_s, cascade_callbacks: true
  accepts_nested_attributes_for :photo, allow_destroy: true
end

Gallery.create(photo_attributes: { title: "Image" })

We can see that Mongoid executes actions in the following order:

1. Photo.before_save
2. Photo.after_save
3. Gallery.before_save
4. Gallery persist operation
5. 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 🤔

[janko]

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 😃

[flvrone]

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. 😉

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 😄

[janko]

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.

[flvrone]

@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.

[janko]

@FunkyloverOne I'm fine with separating these two things. Could we perhaps make it a plugin option instead?

[flvrone]

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 #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!

[flvrone]

Meanwhile, I've figured out that _children method returns a flat array which already includes deeply nested ones, yay!

[flvrone]

And we should only perform that fancy "reload" for an embedded record if it is already persisted within its parent.

[flvrone]

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

[flvrone]

@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.