Skip to content

Commit

Permalink
FIX: Backups should use relative paths for local uploads
Browse files Browse the repository at this point in the history 
This also ensures that restoring a backup works when it was created with the wrong upload paths in the time between ab4c0a4 (shortly after v2.6.0.beta1) and this fix.
  • Loading branch information
@gschlager
gschlager committed Aug 21, 2020
1 parent 2fac77c commit f51ccea
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 5 deletions.
2 changes: 2 additions & 0 deletions lib/backup_restore/backup_file_handler.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ def decompress_archive
log "Unzipping archive, this may take a while..."
Discourse::Utils.execute_command(
'tar', '--extract', '--gzip', '--file', @archive_path, '--directory', @tmp_directory,
'--transform', 's|var/www/discourse/public/uploads/|uploads/|',
# the transformation is a workaround for a bug which existed between v2.6.0.beta1 and v2.6.0.beta2
failure_message: "Failed to decompress archive."
)
end
Expand Down
6 changes: 4 additions & 2 deletions lib/backup_restore/backuper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,15 +234,17 @@ def add_local_uploads_to_archive(tar_filename)
log "Archiving uploads..."

if has_local_uploads?
upload_directory = Discourse.store.upload_path

if SiteSetting.include_thumbnails_in_backups
exclude_optimized = ""
else
optimized_path = File.join(local_uploads_directory, 'optimized')
optimized_path = File.join(upload_directory, 'optimized')
exclude_optimized = "--exclude=#{optimized_path}"
end

Discourse::Utils.execute_command(
'tar', '--append', '--dereference', exclude_optimized, '--file', tar_filename, local_uploads_directory,
'tar', '--append', '--dereference', exclude_optimized, '--file', tar_filename, upload_directory,
failure_message: "Failed to archive uploads.", success_status_codes: [0, 1],
chdir: File.join(Rails.root, "public")
)
Expand Down
Binary file added spec/fixtures/backups/backup_with_wrong_upload_path.tar.gz
View file
Binary file not shown.
33 changes: 30 additions & 3 deletions spec/lib/backup_restore/backup_file_handler_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
include_context "shared stuff"

def expect_decompress_and_clean_up_to_work(backup_filename:, expected_dump_filename: "dump.sql",
require_metadata_file:, require_uploads:)
require_metadata_file:, require_uploads:, expected_upload_paths: nil)

freeze_time(DateTime.parse('2019-12-24 14:31:48'))

Expand All @@ -31,8 +31,13 @@ def expect_decompress_and_clean_up_to_work(backup_filename:, expected_dump_filen
expect(File.exist?(File.join(tmp_directory, "meta.json"))).to eq(require_metadata_file)

if require_uploads
upload_filename = "uploads/default/original/3X/b/d/bd269860bb508aebcb6f08fe7289d5f117830383.png"
expect(File.exist?(File.join(tmp_directory, upload_filename))).to eq(true)
expected_upload_paths ||= ["uploads/default/original/3X/b/d/bd269860bb508aebcb6f08fe7289d5f117830383.png"]

expected_upload_paths.each do |upload_path|
absolute_upload_path = File.join(tmp_directory, upload_path)
expect(File.exist?(absolute_upload_path)).to eq(true), "expected file #{upload_path} does not exist"
yield(absolute_upload_path) if block_given?
end
else
expect(Dir.exist?(File.join(tmp_directory, "uploads"))).to eq(false)
end
Expand Down Expand Up @@ -74,4 +79,26 @@ def expect_decompress_and_clean_up_to_work(backup_filename:, expected_dump_filen
require_uploads: false
)
end

it "works with backup file which uses wrong upload path" do
expect_decompress_and_clean_up_to_work(
backup_filename: "backup_with_wrong_upload_path.tar.gz",
require_metadata_file: false,
require_uploads: true,
expected_upload_paths: [
"uploads/default/original/1X/both.txt",
"uploads/default/original/1X/only-uploads.txt",
"uploads/default/original/1X/only-var.txt"
]
) do |upload_path|
content = File.read(upload_path).chomp

case File.basename(upload_path)
when "both.txt", "only-var.txt"
expect(content).to eq("var")
when "only-uploads.txt"
expect(content).to eq("uploads")
end
end
end
end

6 comments on commit f51ccea

@discoursebot
Copy link

Choose a reason for hiding this comment

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

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/old-image-uploads-become-broken-images/47236/55

@CvX
Copy link
Contributor

@CvX CvX commented on f51ccea Aug 27, 2020

Choose a reason for hiding this comment

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

@gschlager bsdtar bundled with macOS Catalina doesn't support --transform flag:

RuntimeError:
  lib/discourse.rb:92:in `exec': Failed to decompress archive.
  tar: Option --transform is not supported
  Usage:
    List:    tar -tf <archive-filename>
    Extract: tar -xf <archive-filename>
    Create:  tar -cf <archive-filename> [filenames...]
    Help:    tar --help

❯ tar --help
tar(bsdtar): manipulate archive files
First option must be a mode specifier:
  -c Create  -r Add/Replace  -t List  -u Update  -x Extract
Common Options:
  -b #  Use # 512-byte records per I/O block
  -f <filename>  Location of archive
  -v    Verbose
  -w    Interactive
Create: tar -c [options] [<file> | <dir> | @<archive> | -C <dir> ]
  <file>, <dir>  add these items to archive
  -z, -j, -J, --lzma  Compress archive with gzip/bzip2/xz/lzma
  --format {ustar|pax|cpio|shar}  Select archive format
  --exclude <pattern>  Skip files that match pattern
  -C <dir>  Change to <dir> before processing remaining files
  @<archive>  Add entries from <archive> to output
List: tar -t [options] [<patterns>]
  <patterns>  If specified, list only entries that match
Extract: tar -x [options] [<patterns>]
  <patterns>  If specified, extract only entries that match
  -k    Keep (don't overwrite) existing files
  -m    Don't restore modification times
  -O    Write entries to stdout, don't restore to disk
  -p    Restore permissions (including ACLs, owner, file flags)
bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.6

It does work with gnu-tar:

❯ brew install gnu-tar
❯ PATH="/usr/local/opt/gnu-tar/libexec/gnubin:$PATH" bundle exec rspec spec/lib/backup_restore/backup_file_handler_spec.rb

Randomized with seed 20132
....

Finished in 0.56633 seconds (files took 5.68 seconds to load)
4 examples, 0 failures

Randomized with seed 20132

Is it possible to fix the bug without this flag?
If not, we'll have to update macOS setup guide with that brew install command and a note about adding "gnubin" directory to bashrc.

@gschlager
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we can get it to work with bsdtar by using the -s flag.
https://stackoverflow.com/a/45300269

@CvX
Copy link
Contributor

@CvX CvX commented on f51ccea Aug 27, 2020

Choose a reason for hiding this comment

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

Yup, I can confirm this

'-s', '|var/www/discourse/public/uploads/|uploads/|',

does work with bsdtar:

❯ b rspec spec/lib/backup_restore/backup_file_handler_spec.rb

Randomized with seed 55698
....

Finished in 0.24695 seconds (files took 4.08 seconds to load)
4 examples, 0 failures

Randomized with seed 55698

It's not supported by gnu-tar, so we'll have to detect bsdtar or just catch the error and retry with the other argument.

@gschlager
Copy link
Member Author

Choose a reason for hiding this comment

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

What's the output of tar --version on macOS?

@CvX
Copy link
Contributor

@CvX CvX commented on f51ccea Aug 27, 2020

Choose a reason for hiding this comment

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

It's already hiding at the end of --help output above 😅

❯ tar --version
bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.6

Please sign in to comment.