Shrine: Shrine not regenerating location when using the copy plugin

[aried3r]

Hey! Just had the time to look into this again and I definitely think it's a bug. There's already a discussion here with a repro-gist.

I _think_ it's because of the activerecord plugin not correctly triggering the after_commit callback which would trigger the migrations, but I might be wrong.

Little bit of background:
It doesn't work in the naive usecase with the pretty_location plugin as seen in the gist, but in my special case it's even more complicated for me to write a workaround since I implement a .deep_copy method on a Gallery (with many Albums which have many Images). So I call .deep_copy on it, which works fine, except for generating the location for which I'd have to implement another method .fix_deep_copy, since records are only saved once all records are copied.

[janko]

Just had the time to look into this again and I definitely think it's a bug. There's already a discussion here with a repro-gist.

I had a look at the gist, and to me the behaviour looks correct. The copy plugin won't promote the copied file once the destination record is saved, because the original files were already copied to permanent storage, there is no need for another re-upload.

If you want the record ID to be there, one way is to call Shrine::Attacher#promote like you did in the gist. But if you want to avoid another re-upload, I came up with the following trick that seems to work:

Post.transaction do # make this operation appear atomic
  copy = Post.create(post.attributes.except("id"))
  copy.image_attacher.copy(post.image_attacher)
  copy.image.id # => "post/2/image/38007749b97452fdf38b9404eede391f"
  copy.save
end

[aried3r]

I had a look at the gist, and to me the behaviour looks correct.

I agree that it works as intended and not in an erroneous way, but I think it's not what people would expect, given that pretty_location is an officially supported plugin. It would lead to "stray" files, where they are not expected.

Not that I don't think people can and maybe should read the design documents and the source code, but coming from carrierwave (where clone = post.dup; clone.image = post.image.dup would suffice) to shrine I was really happy to discover that with the copy plugin I can save the image.dup call, only to discover I have to work a much larger workaround.

It seems like a leaky abstraction to me. I go from post.dup to having to know how shrine works internally, how to manually copy files using the attacher, excluding the ID when copying the record beforehand.

I hope I didn't come across too harsh, I really love shrine and love to work with it! Even just reading the source is a joy. This is my only grief with it so far and felt "wrong", given how everything fit like a glove so far. :)

But if you want to avoid another re-upload, I came up with the following trick that seems to work:

I'll try that, thank you!

[janko]

@aried3r I think that problem is not that #dup isn't working correctly, but that it never could, I see that now from your example. Calling photo1.image_attacher.copy(photo2.image_attacher) should be the primary way to copy attachments (though I'm open to some syntactic sugar around that which would preferably not add pollution).

I won't be able to work on this myself, but I'll gladly accept PRs.

[aried3r]

Ok, I'll first try your workaround, but probably won't get to it this week.

Hmm, as for syntactic sugar, maybe post.image.dup? But that would pretty much defeat the point of having the copy plugin if that doesn't actually copy the attachments as needed. But probably still better to have a note saying _"In addition to calling .dup on the record, you have to call .dup on the attachment as well."_ than _"and here are the 4 lines of workaround code"_.

I think that problem is not that #dup isn't working correctly, but that it never could

Why couldn't it, tho?

[hmistry]

@aried3r Mind if I ask what is the special use case in duplicating the entire record and image file?

[janko]

I think that problem is not that #dup isn't working correctly, but that it never could

Why couldn't it, tho?

Because if #dup is supposed to make a copy of the attachment, but the record is not yet saved at the moment, then the copied attachment cannot possibly have the record ID. #dup is then simply the wrong time to make the copy of the attachment, it has to be after the record is saved.

[aried3r]

Mind if I ask what is the special use case in duplicating the entire record and image file?

We require an exact copy of a record, including updated_at and created_at. With carrierwave it looked like this:

# post.rb

def deep_copy
  copy = dup
  copy.image = image.dup

  copy.created_at = created_at
  copy.updated_at = updated_at

  copy
end

After I had all the copies, I'd only then call .save on them, the image would have been saved into a directory structure, similar to what pretty_location does, including ID.

With shrine I had two problems, first was that .dup didn't work as expected (this issue, see gist in OP) using the copy plugin. Then I tried with copy.image_attacher.promote(action: :migrate) which re-runs generate_location, but also updates updated_at, so I'd end up with a "wrong" (for my use case) timestamp.

So what used to be post.deep_copy ended up being 1) duping the record 2) running promote 3) setting the timestamps which ends up being more work and more DB calls.

[aried3r]

then the copied attachment cannot possibly have the record ID

But then how does it work when initially saving a record? I thought that's pretty much what the post-commit hooks were for.

[hmistry]

We require an exact copy of a record, including updated_at and created_at

@aried3r Sorry one more question, why do you need exact copy of records? Is it for backups, versioning, auditing, something else??? I'm curious about application use case for learning purposes as I was trying to think of scenarios when I'd want to do that in an application.

[aried3r]

But if you want to avoid another re-upload, I came up with the following trick that seems to work:

Hey! Sorry for the late reply. So I've gotten around to trying your workaround, but that yields another problem, it deletes the original. I'm not sure if this is the right use case for the keep_files plugin (from the docs I gathered this is usually just for when you destroy a record, not _copy_ from it).

I extended the original gist, here's the output:

…
Using the workaround:
Original exists: true
post/3/image/dda93e0bfdda5c848b75cb325e0612bd
Original exists: false

Sorry one more question, why do you need exact copy of records? Is it for backups, versioning, auditing, something else???

Phew, hard to explain without going deeply into what we're doing. We do some sort of copy-on-write, but not really, hah. How do I explain this... We make copies of N records where k (k≤N) records will receive updates ("writes"), but we don't know beforehand which will receive updates, so we greedily copy everything, keeping the verbatim copy around, including the timestamps. _Some_ will receive writes and thus their timestamp updated.
It would be possible to otherwise mark the records, but we decided on using the existing timestamps.

I don't think I did a good job of explaining this without this feature sounding stupid, haha. But oh well, client requirements!

[aried3r]

I added the logging pluging to the gist, the output is now:

…
Using the workaround:
Original exists: true
2018-03-14T11:45:07.326Z 26198: STORE[copy] MyUploader[:image] Post[3] 1 file (0.0s)
post/3/image/f22e14b59b0210149ff7c879b2b230c3
2018-03-14T11:45:07.328Z 26198: DELETE[replace] MyUploader[:image] Post[3] 1 file (0.0s)
Original exists: false

I'm not sure where the DELETE[replace] comes from. I don't see how copy.image_attacher.copy(post.image_attacher) should do that.

If I use the keep_files plugin, it works, but I don't think that should be necessary.
https://gist.github.com/aried3r/629f91485692eb3d8b89197c3f92a6b5#file-shrine_copy-rb-L9-L10

[janko]

@aried3r The DELETE[replace] comes from deleting the previous attachment, which was "replaced" by the new attachment. The #initialize_copy in the copy plugin clears out the current attachment on the dup-ed record, but if you're using my snippet it won't be called. The snippet should then be modified to exclude the current attachment:

Post.transaction do # make this operation appear atomic
  copy = Post.create(post.attributes.except("id", "image_data")) # exclude the current attachment data
  copy.image_attacher.copy(post.image_attacher)
  copy.image.id # => "post/2/image/38007749b97452fdf38b9404eede391f"
  copy.save
end

[aried3r]

I see, thank you. It indeed works when also excluding the image_data. I'll try to implement that in my deep_copy code. Makes it a bit more "brittle" than I'd like it to be (compared to post.dup, the workaround requires far more insight into shrine, not that this is a bad thing. I try to understand how everything works in shrine :)).

comes from deleting the previous attachment, which was "replaced" by the new attachment.

I could not find that in the code, could you point me to the part where it deletes the previous attachment if it's not cleared out? I can't really follow how shrine does it internally.

I see where it's set to nil in the copy plugin, but not why this then results in not deleting the previous attachment.

[aried3r]

For anyone stumbling across this, if you're using validates :image, presence: true the workaround above won't work because you can't save a Post without image (image_data). So either you have to make the validation conditional (validates :image, presence: true unless :deep_copy?) or disable validations altogether:

Post.transaction do
  copy = post.dup
  copy = Post.new(post.attributes.except("id", "image_data"))
  copy.save(validate: false)
  copy.image_attacher.copy(post.image_attacher)
  copy.save
end

@janko-m, do you agree with that solution?
Also, do you think this issue can be closed since it doesn't happen in the default config and only with deterministic locations using the ID as the only moving part?
My use case might be special, but still something feels off using the workaround.

[janko]

@aried3r Just curious, wouldn't just this work as well?

Post.transaction do
  copy = Post.new(post.attributes.except("id", "image_data"))
  copy.image_attacher.copy(post.image_attacher)
  copy.save
end

In other words, do you need the post.dup or the copy.save(validate: false) before copying the attachment?

My use case might be special, but still something feels off using the workaround.

Yeah, there is definitely something wrong here, and I would like to keep the issue open. For one, the #intialize_copy override that the copy plugin provides needs to go, as at the moment it's there whether you want it or not.

[aried3r]

In other words, do you need the post.dup or the copy.save(validate: false) before copying the attachment?

From what I've tried, yes, because if I don't .save before, I run into the initial problem that there is no id in the path pretty_location generates.

Post.transaction do # make this operation appear atomic
  copy = Post.new(post.attributes.except("id", "image_data"))
  copy.image_attacher.copy(post.image_attacher)
  copy.save
end

results in post/image/79743ef3970d5e6051a9ae4a18b606b8.

Post.transaction do # make this operation appear atomic
  copy = Post.new(post.attributes.except("id", "image_data"))
  copy.save(validate: false)
  copy.image_attacher.copy(post.image_attacher)
  copy.save
end

results in: post/3/image/52f036416c12b124e87d593603e94600.

[janko]

The copy plugin will be removed in Shrine 3.0. On master, Attacher#set method is modified not to run validations, so the following should be safe even with versions:

copied_file = attacher.upload(other_atacher.file, :store)
attacher.set(copied_file)

All in all, in 3.0 it will be really easy to achieve the behaviour the copy plugin offered, without the downsides discussed here.

[Menelle]

Hello janko.
I'm on shrine 2.1.9.
I tried to replace the copy plugin.
Just noticed that the set method with versions generate a
NoMethodError (undefined methodextension' for #)`

      uploader = ProfileUploader.new(:store)
      copied_file = uploader.upload(user.profile_file) #called on uploader not attacher
      # => { :original => {}, :square => {}, :thumb => {} } 
      partner.profile_file_attacher.set(copied_file)
      # =>NoMethodError (undefined method `extension' for #<Hash:0x000055d2a7490b18>)
      partner.profile_file_attacher.set(copied_file[:original)
      # => true

Is there a way to get all versions with the set method ?
Thank you

[janko]

@Menelle What is the full backtrace of the exception? Could you post a self-contained example that reproduces the problem?

[Menelle]

@janko Thanks for your answer. Here is the backtrace.
`irb(main):002:0> user, partner = User.first, Partner.last #user and partner are identical in structure
irb(main):003:0> uploader = ProfileUploader.new(:store)
=> #eUploader:0x00007fda08a30310 @storage=#, @bucket=#Bucket:0x00007fda04cdda30 @name="app-dev", @data=nil, @client=#>, @prefix="store", @host=nil, @upload_options={:acl=>"public-read", :cache_control=>"public, max-age=315569260", :expires=>604800}, @multipart_threshold={:upload=>15728640, :copy=>104857600}, @signer=nil, @public=nil>, @storage_key=:store>
irb(main):004:0> copied_file = uploader.upload(user.profile_file)
[Aws::S3::Client 200 0.639851 0 retries] copy_object(content_type:"image/jpeg",content_disposition:"inline; filename=\"image_processing-mini_magick20180907-56792-17v1r2h.jpg\"; filename*=UTF-8''image_processing-mini_magick20180907-56792-17v1r2h.jpg",acl:"public-read",cache_control:"public, max-age=315569260",expires:1970-01-08 00:00:00 UTC,bucket:"app-dev",key:"store/medium-58c8508b703b3d04a5b7fb7b258507f5.jpg",copy_source:"app-dev/store/user/1/profile_file/medium-1ca26848b4bf34685d2279b073468c14.jpg")

[Aws::S3::Client 200 0.146109 0 retries] copy_object(content_type:"image/jpeg",content_disposition:"inline; filename=\"image_processing-mini_magick20180907-56792-f03bzn.jpg\"; filename*=UTF-8''image_processing-mini_magick20180907-56792-f03bzn.jpg",acl:"public-read",cache_control:"public, max-age=315569260",expires:1970-01-08 00:00:00 UTC,bucket:"app-dev",key:"store/small-30c10deadb3f8fc48dc3c322ce597394.jpg",copy_source:"app-dev/store/user/1/profile_file/small-083a8316eee946cf960bb352ee04af41.jpg")

[Aws::S3::Client 200 0.19018 0 retries] copy_object(content_type:"image/jpeg",content_disposition:"inline; filename=\"prada-fw0708-04.jpg\"; filename*=UTF-8''prada-fw0708-04.jpg",acl:"public-read",cache_control:"public, max-age=315569260",expires:1970-01-08 00:00:00 UTC,bucket:"app-dev",key:"store/original-878f4af4d4526e010904e8171d6f58c9.jpg",copy_source:"app-dev/store/user/1/profile_file/original-d1d49404969fb16a8ab624d1bb584b00.jpg")

STORE ProfileUploader 3 files (1.02s)
=> {:medium=># irb(main):005:0> partner.profile_file_attacher.set(copied_file)
Traceback (most recent call last):
2: from (irb):5
1: from app/uploaders/profile_uploader.rb:17:in block in <class:ProfileUploader>' NoMethodError (undefined methodextension' for #)
Did you mean? extend`

[Menelle]

And here is the relevant part of the ProfileUploader :
Attacher.validate do validate_extension_inclusion %w[jpg jpeg png gif] # line 17 causing exception validate_mime_type_inclusion %w[image/jpeg image/png image/gif] validate_max_size 2*1024*1024 # 2 MB validate_min_size 10*1024 # 10 KB validate_max_width 1500 validate_max_height 1500 end

[janko]

On Shrine 2.x, Attacher#set still runs validations, that's what's causing the error. You need to either use Attacher#_set (which is private):

attacher.send(:_set, copied_file)

or skip validations when versions are assigned:

Attacher.validate do
  next if get.is_a?(Hash)
  # ...
end

[Menelle]

I did went with the second solution as a fix. But the attacher#_set seems cleaner. Thank you for your support, and your work.