carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks
https://github.com/carrierwaveuploader/carrierwave
8.78k stars 1.65k forks source link

Filenames Prefixed With Long String of Numbers After Upgrade to 3.0.7 #2736

Open DBugger32 opened 3 months ago

DBugger32 commented 3 months ago

So much for a minor update....

Simple method for attaching a PDF to an active record object (puts statements for debugging)

 def attach_pdf(pdf_data)
    pdf_file = File.new("#{Dir.tmpdir}/#{self.summary_file_name}.pdf", "wb")
    pdf_file.write(pdf_data)
    doc_type = DocumentType.find_by(key: "claim_summary")
    a = Attachment.new(attachment_file: pdf_file, document_type: doc_type, source: "CO")
    a.save!
    puts "Original File Path - #{pdf_file.path}"
    puts "After Save - #{a.attachment_file}"
    a.reload
    puts "After Reload - #{a.attachment_file}"
end

Output 3.0.7

Original File Path - /var/folders/jt/rm2yhr0n72n3strzh46jxsr80000gp/T/Claim_Summary_Smith_Paula.pdf After Save - /uploads/attachment/attachment_file/1010673124/Claim_Summary_Smith_Paula.pdf After Reload - /uploads/attachment/attachment_file/1010673124/1712335404-594640814208633-0001-0461/ Claim_Summary_Smith_Paula.pdf

Where did 1712335404-594640814208633-0001-0461/ come from? How can I stop it?

This what used to happen, and what I expected to continue to happen...

Output 3.0.5

Original File Path - /var/folders/jt/rm2yhr0n72n3strzh46jxsr80000gp/T/Claim_Summary_Smith_Paula.pdf After Save - /uploads/attachment/attachment_file/1010673124/Claim_Summary_Smith_Paula.pdf After Reload - /uploads/attachment/attachment_file/1010673124/Claim_Summary_Smith_Paula.pdf

The uploader mount on the Attachment object...

mount_uploader :attachment_file, AttachmentUploader, mount_on: :file_name

The uploader...

class AttachmentUploader < CarrierWave::Uploader::Base

  include Rails.application.routes.url_helpers

  # Override the directory where uploaded files will be stored.
  # This is a sensible default for uploaders that are meant to be mounted:
  def store_dir
    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
  end

  def extension_allowlist
    %w(pdf png jpeg jpg tiff tif)
  end

end

Curious where the prefix folder 1712335404-594640814208633-0001-0461/ came from in the file name and how it can be prevented.

Note the reload was added to the method to demonstrate the problem. Issue manifests itself when attempting to access the file after it was attached.

Rails 6.1.7.7 Ruby 3.2.2

mshibuya commented 3 months ago

This can be related to https://github.com/carrierwaveuploader/carrierwave/commit/4c65b393cd85b66bc256d04363cf3e3a97c8fd64, but I couldn't reproduce the issue using this single-file snippet:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails"
  gem "sqlite3"
  # gem "carrierwave", "3.0.5"
  gem "carrierwave", "3.0.7"
end

require "rails/all"
require 'logger'

Rails.logger = Logger.new(STDOUT)

class App < ::Rails::Application
end

require "carrierwave"
require "carrierwave/orm/activerecord"
require "minitest/autorun"

CarrierWave.root = File.expand_path(".")

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(File::NULL)

ActiveRecord::Schema.define do
  create_table :attachments, force: true do |t|
    t.string :file_name
  end
end

class AttachmentUploader < CarrierWave::Uploader::Base

  include Rails.application.routes.url_helpers

  # Override the directory where uploaded files will be stored.
  # This is a sensible default for uploaders that are meant to be mounted:
  def store_dir
    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
  end

  def extension_allowlist
    %w(pdf png jpeg jpg tiff tif)
  end

end

class Attachment < ActiveRecord::Base
  mount_uploader :attachment_file, AttachmentUploader, mount_on: :file_name
end

class Test < Minitest::Test
  def test_reload
    pdf_file = File.new("#{Dir.tmpdir}/test.pdf", "wb")
    pdf_file.write("test")
    a = Attachment.new(attachment_file: pdf_file)
    a.save!
    puts "Original File Path - #{pdf_file.path}"
    puts "After Save - #{a.attachment_file}"
    a.reload
    puts "After Reload - #{a.attachment_file}"
  end
end
% ruby test.rb
...
Original File Path - /var/folders/70/tt7k53x14gbbyzpk0t6hv9ym0000gn/T/test.pdf
After Save - /uploads/attachment/attachment_file/1/test.pdf
After Reload - /uploads/attachment/attachment_file/1/test.pdf

Can you try to reproduce by using this?

DBugger32 commented 3 months ago

Same result. Magic generation ID prepended using version 3.0.7

Original File Path - /var/folders/jt/rm2yhr0n72n3strzh46jxsr80000gp/T/test.pdf
After Save - /uploads/attachment/attachment_file/1010673124/test.pdf
After Reload - /uploads/attachment/attachment_file/1010673124/1712402783-259480583918325-0001-4281/test.pdf
mshibuya commented 3 months ago

OK could you paste the full output? Here's my result:

% ruby -v
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]
% ruby test.rb
Fetching gem metadata from https://rubygems.org/...........
Resolving dependencies...
Using rake 13.2.1
Using base64 0.2.0
Using bigdecimal 3.1.7
Using concurrent-ruby 1.2.3
Using connection_pool 2.4.1
Using minitest 5.22.3
Using mutex_m 0.2.0
Using erubi 1.12.0
Using mini_portile2 2.8.5
Using racc 1.7.3
Using crass 1.0.6
Using rack 3.0.10
Using nio4r 2.7.1
Using websocket-extensions 0.1.5
Using zeitwerk 2.6.13
Using timeout 0.4.1
Using mini_mime 1.1.5
Using date 3.3.4
Using public_suffix 5.0.5
Using bundler 2.3.26
Using mini_magick 4.12.0
Using ffi 1.16.3
Using ssrf_filter 1.1.2
Using io-console 0.7.2
Using stringio 3.1.0
Using thor 1.3.1
Using drb 2.2.1
Using builder 3.2.4
Using rack-test 2.1.0
Using webrick 1.8.1
Using rack-session 2.0.0
Using i18n 1.14.4
Using tzinfo 2.0.6
Using psych 5.1.2
Using websocket-driver 0.7.6
Using rackup 2.1.0
Using activesupport 7.1.3.2
Using sqlite3 1.7.3
Using marcel 1.0.4
Using net-protocol 0.2.2
Using addressable 2.8.6
Using globalid 1.2.1
Using activemodel 7.1.3.2
Using net-imap 0.4.10
Using activejob 7.1.3.2
Using activerecord 7.1.3.2
Using reline 0.5.0
Using rdoc 6.6.3.1
Using net-smtp 0.5.0
Using irb 1.12.0
Using nokogiri 1.16.3
Using ruby-vips 2.2.1
Using net-pop 0.1.2
Using rails-dom-testing 2.2.0
Using loofah 2.22.0
Using image_processing 1.12.2
Using rails-html-sanitizer 1.6.0
Using mail 2.8.1
Using carrierwave 3.0.7
Using actionview 7.1.3.2
Using actionpack 7.1.3.2
Using actioncable 7.1.3.2
Using activestorage 7.1.3.2
Using actionmailer 7.1.3.2
Using actionmailbox 7.1.3.2
Using actiontext 7.1.3.2
Using railties 7.1.3.2
Using rails 7.1.3.2
-- create_table(:attachments, {:force=>true})
   -> 0.0082s
Run options: --seed 28187

# Running:

Original File Path - /var/folders/70/tt7k53x14gbbyzpk0t6hv9ym0000gn/T/test.pdf
After Save - /uploads/attachment/attachment_file/1/test.pdf
After Reload - /uploads/attachment/attachment_file/1/test.pdf
.

Finished in 0.008023s, 124.6417 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 0 errors, 0 skips
skyeagle commented 2 months ago

We hit the same issue and tracked it down to version 3.0.6, but could not come up with a reproducible example yet.

There is one thing that may indicate an issue:

  def test_reload
    pdf_file = File.new("#{Dir.tmpdir}/test.pdf", "wb")
    pdf_file.write("test")
    a = Attachment.new(attachment_file: pdf_file)
    puts "Column value before save - #{a.file_name}"
    a.save!
    puts "Column value after save - #{a.file_name}"
    puts "Original File Path - #{pdf_file.path}"
    puts "After Save - #{a.attachment_file}"
    a.reload
    puts "After Reload - #{a.attachment_file}"
  end

and the result

Column value before save - 1713218829-768556429375302-0001-8638/test.pdf
Column value after save - test.pdf
Original File Path - /tmp/test.pdf
After Save - /uploads/attachment/attachment_file/1/test.pdf
After Reload - /uploads/attachment/attachment_file/1/test.pdf
DBugger32 commented 2 months ago

Running your supplied snippet produced the same results as yours.

I provided an example of the behavior that I found from one of my unit tests that suddenly went red when I attempted to move from 3.0.5 to 3.0.7.

I appreciate the need to reproduce the error to fix it but I am at a loss as what extra information is needed.

DBugger32 commented 2 months ago

This is definitely related to 4c65b393.

Hated to do it, but it is monkey patch time. Added an initializer with this...

class CarrierWave::Uploader::Base

  def temporary_identifier
    @original_filename || @identifier
  end

end

And my tests have returned to green.