Backgrounding for embedded models

[flvrone]

Hi! In mongoid you have another relation type: embeds_one/embeds_many+embedded_in.

This is an analog for has_one/has_many+belongs_to, but in case of embedding - embedded model will be saved as a nested JSON object inside it's parent object, instead of a standalone object (record) in separate collection ("table").

That's quite common practice to have embedded models in mongoid, and in most cases it totally makes sense to use this approach for some "Attachment" model.

But there is slight problem with that - it's not possible to find such embedded model using code like this: Attachment.find(id). Instead one should find parent record first, and then search for that attachment by its id inside parent's attachments collection, like this: parent_record.attachments.find(id)

So this means that the following code won't work:

class TranscoderUploader < Shrine
  plugin :backgrounding
  Attacher.promote { |data| TranscodeJob.perform_async(data) }
end

Because the data hash contains data like this:

{ ..., "record"=>["Attachment", "5bbb71b79bf2cc634c187a24"], ... }

So it actually tries to find attachment record like this: Attachment.find('5bbb71b79bf2cc634c187a24'), and we already know that won't work.

I got no idea how to make it work on this gem level "out of box", meanwhile I will try to come up with some workaround for this issue, and I will leave it here once I find it.

---

UPDATE I meant that this code inside TranscodeJob won't work actually:

def perform(data)
    attacher = TranscoderUploader::Attacher.load(data)
    ...
end

So maybe that's not an issue at all, because it's my custom sidekiq job code is not working, not shrine's. I just was relaying on shrine's Attacher.load code, and now it's just not enough :)

[janko]

Uh, yeah, the backgrounding plugin wasn't designed for this use case. Thanks for explaining it so clearly, I should be able to figure out how to modify Shrine so that shrine-mongoid can take care of the embedded scenario automatically.

In the meanwhile, I managed to come up with this workaround (you can put it in your shrine.rb initializer):

module EmbeddedMongoidSupport
  module AttacherClassMethods
    def load_record(data)
      if data["parent"]
        parent_class, parent_id, association_name = data["parent"]
        child_class, child_id = data["record"]

        parent_class = Object.const_get(parent_class)
        parent       = find_record(parent_class, parent_id)
        association  = parent.send(association_name)

        association.where(id: child_id).first
      else
        super
      end
    end
  end

  module AttacherMethods
    def dump
      hash = super
      if record.embedded?
        hash["parent"] = [
          record._parent.class.to_s,
          record._parent.id.to_s,
          record.association_name.to_s
        ]
      end
      hash
    end

    def swap(new_file)
      if record.embedded?
        association = record._parent.send(record.association_name)
        reloaded = association.where(id: record.id).first
        return if reloaded.nil? || self.class.new(reloaded, name).read != read
        update(new_file)
      else
        super
      end
    end
  end
end

This is a plugin that you should load after you load the backgrounding plugin:

class TranscoderUploader < Shrine
  plugin :backgrounding
  plugin EmbeddedMongoidSupport
  Attacher.promote { |data| TranscodeJob.perform_async(data) }
end

Here is the script which I used for testing: https://gist.github.com/janko-m/8ba5fca80719101ac91d744890e5075b

[flvrone]

@janko-m you're lightning-fast, you know that? :D Thanks a lot! I was working on it a bit yesterday also, you can take a look on it on this branch: https://github.com/LinkUpStudioUA/shrine-mongoid/tree/embedded-records-support

[janko]

Hehe, thank you 😃

Thanks for working on it also, I see now that I forgot about the embed_one case. For me this looks good, though I'll still need to change Shrine so that we can make it order-independent; at the moment both my plugin and your branch will not work if mongoid plugin is loaded before the backgrounding plugin, as they override methods defined in the backgrounding plugin. That change should hopefully require just minor tweaks to your branch.

[flvrone]

yeah, I also had that thought, that there might certain order of plugins including be needed to make everything work properly... Well, we will see, hopefully we will find some good solution :)

[flvrone]

@janko-m I got an update on my branch, now with tests :)

As for issues with plugin including order - it could be fixed if mongoid plugin was not just included/extended, but rather prepended, read this for more info, if you're not yet familiar with this concept: http://leohetsch.com/include-vs-prepend-vs-extend/

The problem here is that it works more like include and not like extend at all, so that might not work for cases when extend is needed...

P.S. why are you using double quotes everywhere? My rubocop is freaking out, and I often accidentally write single quotes, which are different from your code style... :)

[janko]

As for issues with plugin including order - it could be fixed if mongoid plugin was not just included/extended, but rather prepended, read this for more info, if you're not yet familiar with this concept: http://leohetsch.com/include-vs-prepend-vs-extend/

I think it wouldn't work because it would be prepended even before the base functionality, so it couldn't call super in that case. Even if that would work, (a) it would be surprising that :mongoid is the only plugin that's prepended, and (b) Shrine.plugin methods performs the module inclusion, plugins don't have a say over it.

But we can still make it order-independent in another way, which would be to have :mongoid plugin override only base methods, instead of backgrounding methods. So these methods would first need to be added to base, and then backgrounding plugin needs to be modified to call those base methods, and then when mongoid overrides them it will call the overriden methods.

P.S. why are you using double quotes everywhere? My rubocop is freaking out, and I often accidentally write single quotes, which are different from your code style... :)

Because it removes the cognitive overhead of having to choose double quotes or single quotes depending on whether I'm interpolating or not. If I always use double quotes, then things will work whether I interpolate or not 😃. Note that "double quotes by default" may not be the Rubocop default, but they're allowed in the Ruby Style guide 😉

[flvrone]

Oh, I see, cool! :) As for style guide - you could have your .rubocop.yml file in this repo (or all of them), I guess I will add it.

[janko]

I’m not a fan of Rubocop and I don’t use it, so I wouldn’t like to have a rubocop.yml.

Ok. I’ll try to find time soon to come up with a generic solution for this issue, and incorporate it into your branch.

[flvrone]

OK, no problem. BTW, I need to have tests for different versions of Mongoid, as some interfaces have changed in 7 major release. But I don't have any experience with that (testing against different versions of something). Do you have any solutions in mind? Something you already familiar with?

[janko]

I was using appraisal on sinatra-activerecord, it worked quite well.

[flvrone]

Cool, thanks! It's done now :)

[flvrone]

Hey @janko-m , could you please explain me your swap method? I believe it's to get the most recent version of embedded object from DB... And as far as I know it's not being reloaded with code like this: association.where(id: record.id).first, as it's actually already loaded from DB, it's sort of already in memory.

I was trying to find some evidence, I didn't find one, but instead, I've found this: https://jira.mongodb.org/browse/MONGOID-4191

And as it's faster to work ruby's Array, then perhaps it's better to use this approach in this gem, and also looks like I'm right - there's no reloading.

[flvrone]

Here, now I've checked it out manually: https://gist.github.com/FunkyloverOne/fcda2c3d725590743cb13ad069445375

[flvrone]

Ok, I see that it somehow updates ImageUploader::UploadedFile instance, don't understand how exactly though...

[janko]

When the backgrounding plugin is loaded, the Attacher#swap method does a "safe update", in the sense that it first reloads the record to check whether the attachment has changed, and only if it hasn't it will proceed with the update. Since processing and upload can potentially take some time, it's possible the attachment changed in the meanwhile and we wouldn't want to overwrite it, so Attacher#swap has this mechanism to abort.

And as far as I know it's not being reloaded with code like this: association.where(id: record.id).first, as it's actually already loaded from DB, it's sort of already in memory.

Good catch! You're right, this won't reload the embedded document. What we need to do is reload the parent first, so I'm thinking that changing:

association = record._parent.send(record.association_name)

to

association = record._parent.reload.send(record.association_name)

should correctly reload the embedded document.

[janko]

I've just updated shrine-mongoid for the upcoming Shrine 3.0, which takes a different approach to backgrounding:

1️⃣The user now needs to retrieve the record from the database themselves. That removes undesired responsibility from Shrine and puts it into the hands of the user, where it belongs. 2️⃣Reloading the record when doing "safe-update" is now done using Mongoid::Document#reload, which appears to support embedded document.

I believe that resolves this issue. Thanks for all your hard work on https://github.com/shrinerb/shrine-mongoid/pull/3, but I think these changes won't be necessary anymore.

Shrine 3.0 is currently in beta, we're still working on updating the docs and improving backwards compatibility. I've released 1.0.0.beta of shrine-mongoid if you want to try out Shrine 3.0 early, though some docs such as backgrounding plugin are still outdated (we still plan to write an upgrading guide for 2.x to 3.x).

[flvrone]

Hey @janko, sounds good! Yeah, and don't have to finish that anymore :grin: Perhaps I will try it out soon.