Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AO3-5631 Make /tmp download directory unique per generation #3532

Merged
merged 2 commits into from Mar 3, 2019
Merged

AO3-5631 Make /tmp download directory unique per generation #3532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
920756f
AO3-5631 Make /tmp download directory unique per generation
redsummernight Feb 27, 2019
05db279
AO3-5631 Use Dir.mktmpdir
redsummernight Mar 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions app/controllers/downloads_controller.rb
View file
Open in desktop
Expand Up @@ -8,10 +8,11 @@ class DownloadsController < ApplicationController

def show
respond_to :html, :pdf, :mobi, :epub, :azw3
@download = Download.generate(@work, mime_type: request.format)
@download = Download.new(@work, mime_type: request.format)
@download.generate

# Make sure we were able to generate the download.
unless @download.present? && @download.exists?
unless @download.exists?
flash[:error] = ts("We were not able to render this work. Please try again in a little while or try another format.")
redirect_to work_path(@work)
return
Expand Down Expand Up @@ -47,7 +48,7 @@ def load_work
def remove_downloads
yield
ensure
Download.remove(@work) unless Rails.env.test?
@download.remove
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to delete the directory and things like the zip file ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, @download.remove does that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileUtils.rm_rf(dir) is not subtle

end

# We can't use check_visibility because this controller doesn't have access to
Expand Down
26 changes: 12 additions & 14 deletions app/models/download.rb
View file
Open in desktop
@@ -1,14 +1,4 @@
class Download
# Given a work and a format or mime type, generate a download file
def self.generate(work, options = {})
new(work, options).generate
end

# Remove all downloads for this work
def self.remove(work)
new(work).remove
end

attr_reader :work, :file_type, :mime_type

def initialize(work, options = {})
Expand Down Expand Up @@ -72,7 +62,7 @@ def public_path
"/downloads/#{work.id}/#{file_name}.#{file_type}"
end

# The path to the zip file (eg, "/tmp/42/42.zip")
# The path to the zip file (eg, "/tmp/42_epub_20190301-24600-17164a8/42.zip")
def zip_path
"#{dir}/#{work.id}.zip"
end
Expand All @@ -82,14 +72,22 @@ def assets_path
"#{dir}/assets"
end

# The full path to the file (eg, "/tmp/42/The Hobbit.epub")
# The full path to the HTML file (eg, "/tmp/42_epub_20190301-24600-17164a8/The Hobbit.html")
def html_file_path
"#{dir}/#{file_name}.html"
end

# The full path to the file (eg, "/tmp/42_epub_20190301-24600-17164a8/The Hobbit.epub")
def file_path
"#{dir}/#{file_name}.#{file_type}"
end

# Write to temp and then immediately clean it up
# Get the temporary directory where downloads will be generated,
# creating the directory if it doesn't exist.
def dir
"/tmp/#{work.id}"
return @tmpdir if @tmpdir
@tmpdir = Dir.mktmpdir("#{work.id}_#{file_type}_")
@tmpdir
end

# Utility methods which clean up work data for use in downloads
Expand Down
15 changes: 6 additions & 9 deletions app/models/download_writer.rb
View file
Open in desktop
@@ -1,17 +1,14 @@
require 'open3'

class DownloadWriter
attr_reader :download, :work, :html_download
attr_reader :download, :work

def initialize(download)
@download = download
@work = download.work
@html_download = Download.new(work, format: "html")
end

def write
# Create the directory
FileUtils.mkdir_p download.dir
generate_html_download
generate_ebook_download unless download.file_type == "html"
download
Expand All @@ -21,7 +18,7 @@ def write

# Write the HTML version
def generate_html_download
return if html_download.exists?
return if download.exists?

renderer = ApplicationController.renderer.new(
http_host: ArchiveConfig.APP_HOST
Expand All @@ -35,9 +32,9 @@ def generate_html_download
chapters: download.chapters
}
)

# write to file
File.open(html_download.file_path, 'w:UTF-8') { |f| f.write(@html) }
File.open(download.html_file_path, 'w:UTF-8') { |f| f.write(@html) }
end

# transform HTML version into ebook version
Expand Down Expand Up @@ -74,7 +71,7 @@ def get_pdf_command
'--disable-smart-shrinking',
'--log-level', 'none',
'--title', download.file_name,
html_download.file_path, download.file_path
download.html_file_path, download.file_path
]
end

Expand Down Expand Up @@ -126,7 +123,7 @@ def get_web2disk_command
'--base-dir', download.assets_path,
'--max-recursions', '0',
'--dont-download-stylesheets',
"file://#{html_download.file_path}"
"file://#{download.html_file_path}"
]
end

Expand Down
5 changes: 0 additions & 5 deletions app/models/work.rb
View file
Open in desktop
Expand Up @@ -846,11 +846,6 @@ def set_word_count(preview = false)
end
end

after_update :remove_outdated_downloads
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that more files will be left around ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because we delete them after we generate and send them off.

def remove_outdated_downloads
Download.remove(self)
end

#######################################################################
# TAGGING
# Works are taggable objects.
Expand Down
51 changes: 10 additions & 41 deletions features/step_definitions/work_download_steps.rb
View file
Open in desktop
@@ -1,56 +1,25 @@
Then /^I should receive a file of type "([^"]*)"$/ do |filetype|
Then /^I should receive a file of type "(.*?)"$/ do |filetype|
redsummernight marked this conversation as resolved.
Show resolved Hide resolved
mime_type = filetype == "azw3" ? "application/x-mobi8-ebook" : MIME::Types.type_for("foo.#{filetype}").first
page.driver.response.headers['Content-Disposition'].should =~ /filename=.+\.#{filetype}/
page.response_headers['Content-Type'].should == mime_type
expect(page.response_headers['Content-Disposition']).to match(/filename=.+\.#{filetype}/)
expect(page.response_headers['Content-Length'].to_i).to be_positive
expect(page.response_headers['Content-Type']).to eq(mime_type)
end

Then /^I should be able to download all versions of "(.*)"$/ do |title|
Then /^I should be able to download all versions of "(.*?)"$/ do |title|
(ArchiveConfig.DOWNLOAD_FORMATS - ['html']).each do |filetype|
step %{I should be able to download the #{filetype} version of "#{title}"}
end
end

Then /^I (?:should be able to )?download the (\w+) version of "(.*)"$/ do |filetype, title|
Then /^I should be able to download the (\w+) version of "(.*?)"$/ do |filetype, title|
work = Work.find_by_title(title)
download = Download.new(work, format: filetype)
visit work_url(work)
step %{I follow "#{filetype.upcase}"}
filename = download.file_path # the full path of the download file we expect to exist
mime_type = filetype == "azw3" ? "application/x-mobi8-ebook" : MIME::Types.type_for(filename).first
assert File.exist?(filename), "#{filename} does not exist"
page.driver.response.headers['Content-Disposition'].should =~ /filename=\"#{File.basename(filename)}\"/
page.response_headers['Content-Type'].should == mime_type
end

Then /^I should not be able to download the (\w+) version of "(.*)"$/ do |filetype, title|
work = Work.find_by_title(title)
download = Download.new(work, format: filetype)
visit work_url(work)
step %{I follow "#{filetype.upcase}"}
filename = download.file_path # the full path of the download file we expect to exist
filename = "#{download.file_name}.#{download.file_type}"
mime_type = filetype == "azw3" ? "application/x-mobi8-ebook" : MIME::Types.type_for(filename).first
page.driver.response.headers['Content-Disposition'].should_not =~ /filename=\"#{File.basename(filename)}\"/
page.response_headers['Content-Type'].should_not == mime_type
end

Then /^I should not be able to manually download the (\w+) version of "(.*)"$/ do |filetype, title|
work = Work.find_by_title(title)
download = Download.new(work, format: filetype)
download_url = "#{ArchiveConfig.APP_URL}#{download.public_path}"
filename = download.file_path # the full path of the download file we expect to exist
mime_type = filetype == "azw3" ? "application/x-mobi8-ebook" : MIME::Types.type_for(filename).first
visit download_url
page.driver.response.headers['Content-Disposition'].should_not =~ /filename=\"#{File.basename(filename)}\"/
page.response_headers['Content-Type'].should_not == mime_type
end

Then /^the (.*) version of "([^"]*)" should exist$/ do |filetype, title|
work = Work.find_by_title(title)
filename = "#{work.download_basename}.#{filetype}" # the full path of the download file we expect to exist
assert Download.new(work, format: filetype).exists?, "#{filename} does not exist"
end

Then /^the (.*) version of "([^"]*)" should not exist$/ do |filetype, title|
work = Work.find_by_title(title)
assert !Download.new(work, format: filetype).exists?, "#{filename} does exist"
expect(page.response_headers['Content-Disposition']).to match(/filename="#{filename}"/)
expect(page.response_headers['Content-Length'].to_i).to be_positive
expect(page.response_headers['Content-Type']).to eq(mime_type)
end