Commit f54290de authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Remove caching of CSV file

Load whole file in memory to simplify code
parent e2698d5d
...@@ -7,12 +7,12 @@ module UploadsActions ...@@ -7,12 +7,12 @@ module UploadsActions
UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
def create def create
link_to_file = UploadService.new(model, params[:file], uploader_class).execute uploader = UploadService.new(model, params[:file], uploader_class).execute
respond_to do |format| respond_to do |format|
if link_to_file if uploader
format.json do format.json do
render json: { link: link_to_file.to_h } render json: { link: uploader.to_h }
end end
else else
format.json do format.json do
......
...@@ -222,7 +222,7 @@ class ProjectPolicy < BasePolicy ...@@ -222,7 +222,7 @@ class ProjectPolicy < BasePolicy
rule { owner | admin | guest | group_member }.prevent :request_access rule { owner | admin | guest | group_member }.prevent :request_access
rule { ~request_access_enabled }.prevent :request_access rule { ~request_access_enabled }.prevent :request_access
rule { developer & can?(:create_issue) }.enable :import_issues rule { can?(:developer_access) & can?(:create_issue) }.enable :import_issues
rule { can?(:developer_access) }.policy do rule { can?(:developer_access) }.policy do
enable :admin_merge_request enable :admin_merge_request
......
...@@ -2,29 +2,26 @@ ...@@ -2,29 +2,26 @@
module Issues module Issues
class ImportCsvService class ImportCsvService
def initialize(user, project, upload) def initialize(user, project, csv_io)
@user = user @user = user
@project = project @project = project
@uploader = upload.build_uploader @csv_io = csv_io
@results = { success: 0, error_lines: [], parse_error: false } @results = { success: 0, error_lines: [], parse_error: false }
end end
def execute def execute
# Cache remote file locally for processing
@uploader.cache_stored_file! unless @uploader.file_storage?
process_csv process_csv
email_results_to_user email_results_to_user
cleanup_cache unless @uploader.file_storage?
@results @results
end end
private private
def process_csv def process_csv
CSV.foreach(@uploader.file.path, col_sep: detect_col_sep, headers: true).with_index(2) do |row, line_no| csv_data = @csv_io.open(&:read).force_encoding(Encoding::UTF_8)
CSV.new(csv_data, col_sep: detect_col_sep(csv_data.lines.first), headers: true).each.with_index(2) do |row, line_no|
issue = Issues::CreateService.new(@project, @user, title: row[0], description: row[1]).execute issue = Issues::CreateService.new(@project, @user, title: row[0], description: row[1]).execute
if issue.persisted? if issue.persisted?
...@@ -41,9 +38,7 @@ module Issues ...@@ -41,9 +38,7 @@ module Issues
Notify.import_issues_csv_email(@user.id, @project.id, @results).deliver_now Notify.import_issues_csv_email(@user.id, @project.id, @results).deliver_now
end end
def detect_col_sep def detect_col_sep(header)
header = File.open(@uploader.file.path, &:readline)
if header.include?(",") if header.include?(",")
"," ","
elsif header.include?(";") elsif header.include?(";")
...@@ -54,12 +49,5 @@ module Issues ...@@ -54,12 +49,5 @@ module Issues
raise CSV::MalformedCSVError raise CSV::MalformedCSVError
end end
end end
def cleanup_cache
cached_file_path = @uploader.file.cache_path
File.delete(cached_file_path)
Dir.delete(File.dirname(cached_file_path))
end
end end
end end
...@@ -15,7 +15,7 @@ ...@@ -15,7 +15,7 @@
.form-group .form-group
= label_tag :file, _('Upload CSV file'), class: 'label-bold' = label_tag :file, _('Upload CSV file'), class: 'label-bold'
%div %div
= file_field_tag :file, accept: 'text/csv', required: true = file_field_tag :file, accept: '.csv,text/csv', required: true
%p.text-secondary %p.text-secondary
= _('It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected.') = _('It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected.')
= _('The maximum file size allowed is %{size}.') % { size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes) } = _('The maximum file size allowed is %{size}.') % { size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes) }
......
- button_path = local_assigns.fetch(:button_path, false) - button_path = local_assigns.fetch(:button_path, false)
- project_select_button = local_assigns.fetch(:project_select_button, false) - project_select_button = local_assigns.fetch(:project_select_button, false)
- show_import_button = local_assigns.fetch(:show_import_button, false) - show_import_button = local_assigns.fetch(:show_import_button, false) && Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, @project)
- has_button = button_path || project_select_button - has_button = button_path || project_select_button
.row.empty-state .row.empty-state
......
...@@ -3,12 +3,16 @@ ...@@ -3,12 +3,16 @@
class ImportIssuesCsvWorker class ImportIssuesCsvWorker
include ApplicationWorker include ApplicationWorker
sidekiq_retries_exhausted do |job|
Upload.find(job['args'][2]).destroy
end
def perform(current_user_id, project_id, upload_id) def perform(current_user_id, project_id, upload_id)
@user = User.find(current_user_id) @user = User.find(current_user_id)
@project = Project.find(project_id) @project = Project.find(project_id)
@upload = Upload.find(upload_id) @upload = Upload.find(upload_id)
importer = Issues::ImportCsvService.new(@user, @project, @upload) importer = Issues::ImportCsvService.new(@user, @project, @upload.build_uploader)
importer.execute importer.execute
@upload.destroy @upload.destroy
......
...@@ -23,8 +23,8 @@ module Gitlab ...@@ -23,8 +23,8 @@ module Gitlab
content_type: attachment.content_type content_type: attachment.content_type
} }
link = UploadService.new(project, file).execute.to_h uploader = UploadService.new(project, file).execute
attachments << link if link attachments << uploader.to_h if uploader
ensure ensure
tmp.close! tmp.close!
end end
......
...@@ -13,19 +13,19 @@ describe Emails::Issues do ...@@ -13,19 +13,19 @@ describe Emails::Issues do
subject { Notify.import_issues_csv_email(user.id, project.id, @results) } subject { Notify.import_issues_csv_email(user.id, project.id, @results) }
it "shows number of successful issues imported" do it "shows number of successful issues imported" do
@results = { success: 165, errors: [], valid_file: true } @results = { success: 165, error_lines: [], parse_error: false }
expect(subject).to have_body_text "165 issues imported" expect(subject).to have_body_text "165 issues imported"
end end
it "shows error when file is invalid" do it "shows error when file is invalid" do
@results = { success: 0, errors: [], valid_file: false } @results = { success: 0, error_lines: [], parse_error: true }
expect(subject).to have_body_text "Error parsing CSV" expect(subject).to have_body_text "Error parsing CSV"
end end
it "shows line numbers with errors" do it "shows line numbers with errors" do
@results = { success: 0, errors: [23, 34, 58], valid_file: false } @results = { success: 0, error_lines: [23, 34, 58], parse_error: false }
expect(subject).to have_body_text "23, 34, 58" expect(subject).to have_body_text "23, 34, 58"
end end
......
...@@ -10,7 +10,7 @@ describe Issues::ImportCsvService do ...@@ -10,7 +10,7 @@ describe Issues::ImportCsvService do
uploader = FileUploader.new(project) uploader = FileUploader.new(project)
uploader.store!(file) uploader.store!(file)
described_class.new(user, project, uploader.upload).execute described_class.new(user, project, uploader).execute
end end
describe '#execute' do describe '#execute' do
......
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