Commit 6902f950 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'rs-carrierwave-db' into 'master'

Record file uploads in the database

See merge request !8893
parents 0cc4afc9 d617182a
......@@ -79,7 +79,7 @@ gem 'kaminari', '~> 0.17.0'
gem 'hamlit', '~> 2.6.1'
# Files attachments
gem 'carrierwave', '~> 0.10.0'
gem 'carrierwave', '~> 0.11.0'
# Drag and Drop UI
gem 'dropzonejs-rails', '~> 0.7.1'
......
......@@ -103,11 +103,12 @@ GEM
capybara-screenshot (1.0.11)
capybara (>= 1.0, < 3)
launchy
carrierwave (0.10.0)
carrierwave (0.11.2)
activemodel (>= 3.2.0)
activesupport (>= 3.2.0)
json (>= 1.7)
mime-types (>= 1.16)
mimemagic (>= 0.3.0)
cause (0.1)
charlock_holmes (0.7.3)
chronic (0.10.2)
......@@ -850,7 +851,7 @@ DEPENDENCIES
bundler-audit (~> 0.5.0)
capybara (~> 2.6.2)
capybara-screenshot (~> 1.0.0)
carrierwave (~> 0.10.0)
carrierwave (~> 0.11.0)
charlock_holmes (~> 0.7.3)
chronic (~> 0.10.2)
chronic_duration (~> 0.10.6)
......
......@@ -10,4 +10,5 @@ class Appearance < ActiveRecord::Base
mount_uploader :logo, AttachmentUploader
mount_uploader :header_logo, AttachmentUploader
has_many :uploads, as: :model, dependent: :destroy
end
......@@ -28,6 +28,7 @@ class Group < Namespace
validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
mount_uploader :avatar, AvatarUploader
has_many :uploads, as: :model, dependent: :destroy
after_create :post_create_hook
after_destroy :post_destroy_hook
......
......@@ -212,6 +212,7 @@ class Project < ActiveRecord::Base
before_save :ensure_runners_token
mount_uploader :avatar, AvatarUploader
has_many :uploads, as: :model, dependent: :destroy
# Scopes
default_scope { where(pending_delete: false) }
......
class Upload < ActiveRecord::Base
# Upper limit for foreground checksum processing
CHECKSUM_THRESHOLD = 100.megabytes
belongs_to :model, polymorphic: true
validates :size, presence: true
validates :path, presence: true
validates :model, presence: true
validates :uploader, presence: true
before_save :calculate_checksum, if: :foreground_checksum?
after_commit :schedule_checksum, unless: :foreground_checksum?
def self.remove_path(path)
where(path: path).destroy_all
end
def self.record(uploader)
remove_path(uploader.relative_path)
create(
size: uploader.file.size,
path: uploader.relative_path,
model: uploader.model,
uploader: uploader.class.to_s
)
end
def absolute_path
return path unless relative_path?
uploader_class.absolute_path(self)
end
def calculate_checksum
return unless exist?
self.checksum = Digest::SHA256.file(absolute_path).hexdigest
end
def exist?
File.exist?(absolute_path)
end
private
def foreground_checksum?
size <= CHECKSUM_THRESHOLD
end
def schedule_checksum
UploadChecksumWorker.perform_async(id)
end
def relative_path?
!path.start_with?('/')
end
def uploader_class
Object.const_get(uploader)
end
end
......@@ -191,6 +191,7 @@ class User < ActiveRecord::Base
end
mount_uploader :avatar, AvatarUploader
has_many :uploads, as: :model, dependent: :destroy
# Scopes
scope :admins, -> { where(admin: true) }
......
class AttachmentUploader < GitlabUploader
include RecordsUploads
include UploaderHelper
storage :file
......
class AvatarUploader < GitlabUploader
include RecordsUploads
include UploaderHelper
storage :file
......
class FileUploader < GitlabUploader
include RecordsUploads
include UploaderHelper
MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
storage :file
def self.absolute_path(upload_record)
File.join(
self.dynamic_path_segment(upload_record.model),
upload_record.path
)
end
# Returns the part of `store_dir` that can change based on the model's current
# path
#
# This is used to build Upload paths dynamically based on the model's current
# namespace and path, allowing us to ignore renames or transfers.
#
# model - Object that responds to `path_with_namespace`
#
# Returns a String without a trailing slash
def self.dynamic_path_segment(model)
File.join(CarrierWave.root, base_dir, model.path_with_namespace)
end
attr_accessor :project
attr_reader :secret
......@@ -13,13 +35,21 @@ class FileUploader < GitlabUploader
end
def store_dir
File.join(base_dir, @project.path_with_namespace, @secret)
File.join(dynamic_path_segment, @secret)
end
def cache_dir
File.join(base_dir, 'tmp', @project.path_with_namespace, @secret)
end
def model
project
end
def relative_path
self.file.path.sub("#{dynamic_path_segment}/", '')
end
def to_markdown
to_h[:markdown]
end
......@@ -40,6 +70,10 @@ class FileUploader < GitlabUploader
private
def dynamic_path_segment
self.class.dynamic_path_segment(model)
end
def generate_secret
SecureRandom.hex
end
......
class GitlabUploader < CarrierWave::Uploader::Base
def self.absolute_path(upload_record)
File.join(CarrierWave.root, upload_record.path)
end
def self.base_dir
'uploads'
end
......@@ -18,4 +22,15 @@ class GitlabUploader < CarrierWave::Uploader::Base
def move_to_store
true
end
# Designed to be overridden by child uploaders that have a dynamic path
# segment -- that is, a path that changes based on mutable attributes of its
# associated model
#
# For example, `FileUploader` builds the storage path based on the associated
# project model's `path_with_namespace` value, which can change when the
# project or its containing namespace is moved or renamed.
def relative_path
self.file.path.sub("#{root}/", '')
end
end
module RecordsUploads
extend ActiveSupport::Concern
included do
after :store, :record_upload
before :remove, :destroy_upload
end
private
# After storing an attachment, create a corresponding Upload record
#
# NOTE: We're ignoring the argument passed to this callback because we want
# the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the
# `Tempfile` object the callback gets.
#
# Called `after :store`
def record_upload(_tempfile)
return unless file_storage?
return unless file.exists?
Upload.record(self)
end
# Before removing an attachment, destroy any Upload records at the same path
#
# Called `before :remove`
def destroy_upload(*args)
return unless file_storage?
return unless file
Upload.remove_path(relative_path)
end
end
class UploadChecksumWorker
include Sidekiq::Worker
include DedicatedSidekiqQueue
def perform(upload_id)
upload = Upload.find(upload_id)
upload.calculate_checksum
upload.save!
rescue ActiveRecord::RecordNotFound
Rails.logger.error("UploadChecksumWorker: couldn't find upload #{upload_id}, skipping")
end
end
......@@ -29,6 +29,7 @@
- [email_receiver, 2]
- [emails_on_push, 2]
- [mailers, 2]
- [upload_checksum, 1]
- [use_key, 1]
- [repository_fork, 1]
- [repository_import, 1]
......
class CreateUploads < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
create_table :uploads do |t|
t.integer :size, limit: 8, null: false
t.string :path, null: false
t.string :checksum, limit: 64
t.references :model, polymorphic: true
t.string :uploader, null: false
t.datetime :created_at, null: false
end
add_index :uploads, :path
add_index :uploads, :checksum
add_index :uploads, [:model_id, :model_type]
end
end
......@@ -1224,6 +1224,20 @@ ActiveRecord::Schema.define(version: 20170306170512) do
add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree
add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree
create_table "uploads", force: :cascade do |t|
t.integer "size", limit: 8, null: false
t.string "path", null: false
t.string "checksum", limit: 64
t.integer "model_id"
t.string "model_type"
t.string "uploader", null: false
t.datetime "created_at", null: false
end
add_index "uploads", ["checksum"], name: "index_uploads_on_checksum", using: :btree
add_index "uploads", ["model_id", "model_type"], name: "index_uploads_on_model_id_and_model_type", using: :btree
add_index "uploads", ["path"], name: "index_uploads_on_path", using: :btree
create_table "user_agent_details", force: :cascade do |t|
t.string "user_agent", null: false
t.string "ip_address", null: false
......
......@@ -35,6 +35,19 @@ describe Projects::UploadsController do
expect(response.body).to match '\"alt\":\"rails_sample\"'
expect(response.body).to match "\"url\":\"/uploads"
end
# NOTE: This is as close as we're getting to an Integration test for this
# behavior. We're avoiding a proper Feature test because those should be
# testing things entirely user-facing, which the Upload model is very much
# not.
it 'creates a corresponding Upload record' do
upload = Upload.last
aggregate_failures do
expect(upload).to exist
expect(upload.model).to eq project
end
end
end
context 'with valid non-image file' do
......
......@@ -199,6 +199,7 @@ project:
- project_authorizations
- route
- statistics
- uploads
award_emoji:
- awardable
- user
......
......@@ -7,4 +7,6 @@ RSpec.describe Appearance, type: :model do
it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_presence_of(:description) }
it { is_expected.to have_many(:uploads).dependent(:destroy) }
end
......@@ -13,6 +13,7 @@ describe Group, models: true do
it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
it { is_expected.to have_many(:labels).class_name('GroupLabel') }
it { is_expected.to have_many(:uploads).dependent(:destroy) }
it { is_expected.to have_one(:chat_team) }
describe '#members & #requesters' do
......
......@@ -71,6 +71,7 @@ describe Project, models: true do
it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
it { is_expected.to have_many(:forks).through(:forked_project_links) }
it { is_expected.to have_many(:uploads).dependent(:destroy) }
context 'after initialized' do
it "has a project_feature" do
......
require 'rails_helper'
describe Upload, type: :model do
describe 'assocations' do
it { is_expected.to belong_to(:model) }
end
describe 'validations' do
it { is_expected.to validate_presence_of(:size) }
it { is_expected.to validate_presence_of(:path) }
it { is_expected.to validate_presence_of(:model) }
it { is_expected.to validate_presence_of(:uploader) }
end
describe 'callbacks' do
context 'for a file above the checksum threshold' do
it 'schedules checksum calculation' do
stub_const('UploadChecksumWorker', spy)
upload = described_class.create(
path: __FILE__,
size: described_class::CHECKSUM_THRESHOLD + 1.kilobyte,
model: build_stubbed(:user),
uploader: double('ExampleUploader')
)
expect(UploadChecksumWorker)
.to have_received(:perform_async).with(upload.id)
end
end
context 'for a file at or below the checksum threshold' do
it 'calculates checksum immediately before save' do
upload = described_class.new(
path: __FILE__,
size: described_class::CHECKSUM_THRESHOLD,
model: build_stubbed(:user),
uploader: double('ExampleUploader')
)
expect { upload.save }
.to change { upload.checksum }.from(nil)
.to(a_string_matching(/\A\h{64}\z/))
end
end
end
describe '.remove_path' do
it 'removes all records at the given path' do
described_class.create!(
size: File.size(__FILE__),
path: __FILE__,
model: build_stubbed(:user),
uploader: 'AvatarUploader'
)
expect { described_class.remove_path(__FILE__) }.
to change { described_class.count }.from(1).to(0)
end
end
describe '.record' do
let(:fake_uploader) do
double(
file: double(size: 12_345),
relative_path: 'foo/bar.jpg',
model: build_stubbed(:user),
class: 'AvatarUploader'
)
end
it 'removes existing paths before creation' do
expect(described_class).to receive(:remove_path)
.with(fake_uploader.relative_path)
described_class.record(fake_uploader)
end
it 'creates a new record and assigns size, path, model, and uploader' do
upload = described_class.record(fake_uploader)
aggregate_failures do
expect(upload).to be_persisted
expect(upload.size).to eq fake_uploader.file.size
expect(upload.path).to eq fake_uploader.relative_path
expect(upload.model_id).to eq fake_uploader.model.id
expect(upload.model_type).to eq fake_uploader.model.class.to_s
expect(upload.uploader).to eq fake_uploader.class
end
end
end
describe '#absolute_path' do
it 'returns the path directly when already absolute' do
path = '/path/to/namespace/project/secret/file.jpg'
upload = described_class.new(path: path)
expect(upload).not_to receive(:uploader_class)
expect(upload.absolute_path).to eq path
end
it "delegates to the uploader's absolute_path method" do
uploader = spy('FakeUploader')
upload = described_class.new(path: 'secret/file.jpg')
expect(upload).to receive(:uploader_class).and_return(uploader)
upload.absolute_path
expect(uploader).to have_received(:absolute_path).with(upload)
end
end
describe '#calculate_checksum' do
it 'calculates the SHA256 sum' do
upload = described_class.new(
path: __FILE__,
size: described_class::CHECKSUM_THRESHOLD - 1.megabyte
)
expected = Digest::SHA256.file(__FILE__).hexdigest
expect { upload.calculate_checksum }
.to change { upload.checksum }.from(nil).to(expected)
end
it 'returns nil for a non-existant file' do
upload = described_class.new(
path: __FILE__,
size: described_class::CHECKSUM_THRESHOLD - 1.megabyte
)
expect(upload).to receive(:exist?).and_return(false)
expect(upload.calculate_checksum).to be_nil
end
end
describe '#exist?' do
it 'returns true when the file exists' do
upload = described_class.new(path: __FILE__)
expect(upload).to exist
end
it 'returns false when the file does not exist' do
upload = described_class.new(path: "#{__FILE__}-nope")
expect(upload).not_to exist
end
end
end
......@@ -36,6 +36,7 @@ describe User, models: true do
it { is_expected.to have_many(:builds).dependent(:nullify) }
it { is_expected.to have_many(:pipelines).dependent(:nullify) }
it { is_expected.to have_many(:chat_names).dependent(:destroy) }
it { is_expected.to have_many(:uploads).dependent(:destroy) }
describe '#group_members' do
it 'does not include group memberships for which user is a requester' do
......
......@@ -10,7 +10,7 @@ describe Projects::UploadService, services: true do
context 'for valid gif file' do
before do
gif = fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif')
@link_to_file = upload_file(@project.repository, gif)
@link_to_file = upload_file(@project, gif)
end
it { expect(@link_to_file).to have_key(:alt) }
......@@ -23,7 +23,7 @@ describe Projects::UploadService, services: true do
before do
png = fixture_file_upload(Rails.root + 'spec/fixtures/dk.png',
'image/png')
@link_to_file = upload_file(@project.repository, png)
@link_to_file = upload_file(@project, png)
end
it { expect(@link_to_file).to have_key(:alt) }
......@@ -35,7 +35,7 @@ describe Projects::UploadService, services: true do
context 'for valid jpg file' do
before do
jpg = fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg')
@link_to_file = upload_file(@project.repository, jpg)
@link_to_file = upload_file(@project, jpg)
end
it { expect(@link_to_file).to have_key(:alt) }
......@@ -47,7 +47,7 @@ describe Projects::UploadService, services: true do
context 'for txt file' do
before do
txt = fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain')
@link_to_file = upload_file(@project.repository, txt)
@link_to_file = upload_file(@project, txt)
end
it { expect(@link_to_file).to have_key(:alt) }
......@@ -60,14 +60,14 @@ describe Projects::UploadService, services: true do
before do
txt = fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain')
allow(txt).to receive(:size) { 1000.megabytes.to_i }
@link_to_file = upload_file(@project.repository, txt)
@link_to_file = upload_file(@project, txt)
end
it { expect(@link_to_file).to eq(nil) }
end
end
def upload_file(repository, file)
Projects::UploadService.new(repository, file).execute
def upload_file(project, file)
Projects::UploadService.new(project, file).execute
end
end
CarrierWave.root = 'tmp/tests/uploads'
CarrierWave.root = File.expand_path('tmp/tests/public', Rails.root)
RSpec.configure do |config|
config.after(:each) do
FileUtils.rm_rf('tmp/tests/uploads')
FileUtils.rm_rf(CarrierWave.root)
end
end
require 'spec_helper'
describe FileUploader do
let(:uploader) { described_class.new(build_stubbed(:project)) }
let(:uploader) { described_class.new(build_stubbed(:empty_project)) }
describe '.absolute_path' do
it 'returns the correct absolute path by building it dynamically' do
project = build_stubbed(:project)
upload = double(model: project, path: 'secret/foo.jpg')
dynamic_segment = project.path_with_namespace
expect(described_class.absolute_path(upload))
.to end_with("#{dynamic_segment}/secret/foo.jpg")
end
end
describe 'initialize' do
it 'generates a secret if none is provided' do
......@@ -32,4 +44,13 @@ describe FileUploader do
expect(uploader.move_to_store).to eq(true)
end
end
describe '#relative_path' do
it 'removes the leading dynamic path segment' do
fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
uploader.store!(fixture_file_upload(fixture))
expect(uploader.relative_path).to match(/\A\h{32}\/rails_sample.jpg\z/)
end
end
end
require 'rails_helper'
describe RecordsUploads do
let(:uploader) do
class RecordsUploadsExampleUploader < GitlabUploader
include RecordsUploads
storage :file
def model
FactoryGirl.build_stubbed(:user)
end
end
RecordsUploadsExampleUploader.new
end
def upload_fixture(filename)
fixture_file_upload(Rails.root.join('spec', 'fixtures', filename))
end
describe 'callbacks' do
it 'calls `record_upload` after `store`' do
expect(uploader).to receive(:record_upload).once
uploader.store!(upload_fixture('doc_sample.txt'))
end
it 'calls `destroy_upload` after `remove`' do
expect(uploader).to receive(:destroy_upload).once
uploader.store!(upload_fixture('doc_sample.txt'))
uploader.remove!
end
end
describe '#record_upload callback' do
it 'returns early when not using file storage' do
allow(uploader).to receive(:file_storage?).and_return(false)
expect(Upload).not_to receive(:record)
uploader.store!(upload_fixture('rails_sample.jpg'))
end
it "returns early when the file doesn't exist" do
allow(uploader).to receive(:file).and_return(double(exists?: false))
expect(Upload).not_to receive(:record)
uploader.store!(upload_fixture('rails_sample.jpg'))
end
it 'creates an Upload record after store' do
expect(Upload).to receive(:record)
.with(uploader)
uploader.store!(upload_fixture('rails_sample.jpg'))
end
it 'it destroys Upload records at the same path before recording' do
existing = Upload.create!(
path: File.join('uploads', 'rails_sample.jpg'),
size: 512.kilobytes,
model: build_stubbed(:user),
uploader: uploader.class.to_s
)
uploader.store!(upload_fixture('rails_sample.jpg'))
expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect(Upload.count).to eq 1
end
end
describe '#destroy_upload callback' do
it 'returns early when not using file storage' do
uploader.store!(upload_fixture('rails_sample.jpg'))
allow(uploader).to receive(:file_storage?).and_return(false)
expect(Upload).not_to receive(:remove_path)
uploader.remove!
end
it 'returns early when file is nil' do
expect(Upload).not_to receive(:remove_path)
uploader.remove!
end
it 'it destroys Upload records at the same path after removal' do
uploader.store!(upload_fixture('rails_sample.jpg'))
expect { uploader.remove! }.to change { Upload.count }.from(1).to(0)
end
end
end
require 'rails_helper'
describe UploaderHelper do
class ExampleUploader < CarrierWave::Uploader::Base
include UploaderHelper
let(:uploader) do
example_uploader = Class.new(CarrierWave::Uploader::Base) do
include UploaderHelper
storage :file
storage :file
end
example_uploader.new
end
def upload_fixture(filename)
......@@ -12,8 +16,6 @@ describe UploaderHelper do
end
describe '#image_or_video?' do
let(:uploader) { ExampleUploader.new }
it 'returns true for an image file' do
uploader.store!(upload_fixture('dk.png'))
......
require 'rails_helper'
describe UploadChecksumWorker do
describe '#perform' do
it 'rescues ActiveRecord::RecordNotFound' do
expect { described_class.new.perform(999_999) }.not_to raise_error
end
it 'calls calculate_checksum_without_delay and save!' do
upload = spy
expect(Upload).to receive(:find).with(999_999).and_return(upload)
described_class.new.perform(999_999)
expect(upload).to have_received(:calculate_checksum)
expect(upload).to have_received(:save!)
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment