Commit 940ad0c7 authored by Stan Hu's avatar Stan Hu

Fix 404s with snippet uploads in object storage

Previously, an HTTP request for
`/uploads/-/system/personal_snippet/:snippet_id/:hash/:filename` would
look for an uploader of `PersonalFileUploader` class and use
`PersonalFileUploader#upload_paths` to search the datbase for one of the
following paths:

1. `:hash/:filename`
2. `uploads/-/system/personal_snippet/:id/:hash/:filename`

However, when the upload were stored in object storage,
`PersonalFileUploader#store_dirs` stored the path as:

`personal_snippet/:snippet_id/:hash`

The extraneous `uploads/-/system` prefix prevented the path from being
matched, and uploads in object storage would return a 404 error. Uploads
in local storage would work fine.

To fix this, we set the `#base_dir` properly so that `#upload_paths`
generates the right value for object storage. Note that this also makes
`#store_dirs` do the right thing in `FileUploader`.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/52595
parent c141d0af
...@@ -6,8 +6,15 @@ class PersonalFileUploader < FileUploader ...@@ -6,8 +6,15 @@ class PersonalFileUploader < FileUploader
options.storage_path options.storage_path
end end
def self.base_dir(model, _store = nil) def self.base_dir(model, store = nil)
File.join(options.base_dir, model_path_segment(model)) base_dirs(model)[store || Store::LOCAL]
end
def self.base_dirs(model)
{
Store::LOCAL => File.join(options.base_dir, model_path_segment(model)),
Store::REMOTE => model_path_segment(model)
}
end end
def self.model_path_segment(model) def self.model_path_segment(model)
...@@ -33,13 +40,6 @@ class PersonalFileUploader < FileUploader ...@@ -33,13 +40,6 @@ class PersonalFileUploader < FileUploader
store_dirs[object_store] store_dirs[object_store]
end end
def store_dirs
{
Store::LOCAL => File.join(base_dir, dynamic_segment),
Store::REMOTE => File.join(self.class.model_path_segment(model), dynamic_segment)
}
end
private private
def secure_url def secure_url
......
---
title: Fix 404s with snippet uploads in object storage
merge_request: 24550
author:
type: fixed
...@@ -4,19 +4,13 @@ describe PersonalFileUploader do ...@@ -4,19 +4,13 @@ describe PersonalFileUploader do
let(:model) { create(:personal_snippet) } let(:model) { create(:personal_snippet) }
let(:uploader) { described_class.new(model) } let(:uploader) { described_class.new(model) }
let(:upload) { create(:upload, :personal_snippet_upload) } let(:upload) { create(:upload, :personal_snippet_upload) }
let(:identifier) { %r{\h+/\S+} }
subject { uploader } subject { uploader }
it_behaves_like 'builds correct paths' do it_behaves_like 'builds correct paths',
let(:patterns) do
{
store_dir: %r[uploads/-/system/personal_snippet/\d+], store_dir: %r[uploads/-/system/personal_snippet/\d+],
upload_path: identifier, upload_path: %r[\h+/\S+],
absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet/\d+/#{identifier}] absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet\/\d+\/\h+\/\S+$]
}
end
end
context "object_store is REMOTE" do context "object_store is REMOTE" do
before do before do
...@@ -25,13 +19,17 @@ describe PersonalFileUploader do ...@@ -25,13 +19,17 @@ describe PersonalFileUploader do
include_context 'with storage', described_class::Store::REMOTE include_context 'with storage', described_class::Store::REMOTE
it_behaves_like 'builds correct paths' do it_behaves_like 'builds correct paths',
let(:patterns) do
{
store_dir: %r[\d+/\h+], store_dir: %r[\d+/\h+],
upload_path: identifier upload_path: %r[^personal_snippet\/\d+\/\h+\/<filename>]
}
end end
describe '#upload_paths' do
it 'builds correct paths for both local and remote storage' do
paths = uploader.upload_paths('test.jpg')
expect(paths.first).to match(%r[\h+\/test.jpg])
expect(paths.second).to match(%r[^personal_snippet\/\d+\/\h+\/test.jpg])
end 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