From e83d35d6f3e832f42a51ff6b3d5655f7fb6b60b5 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 6 Oct 2022 13:44:53 +0100 Subject: [PATCH] FIX: Improve error handling for `calculate_dominant_color!` (#18503) These errors tend to indicate that the upload is missing on the remote store. This is bad, but we don't want it to block the dominant-color calculation process. This commit catches errors when there is an HTTP error, and fixes the `base_store.rb` implementation when `FileHelper.download` returns nil. --- app/models/upload.rb | 4 ++++ lib/file_store/base_store.rb | 2 ++ spec/models/upload_spec.rb | 11 ++++++++++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/models/upload.rb b/app/models/upload.rb index 7bd583f2a6c..8ffbbe99c7e 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -373,6 +373,10 @@ class Upload < ActiveRecord::Base raise "Calculated dominant color but unable to parse output:\n#{data}" if color.nil? color + rescue OpenURI::HTTPError => e + # Some issue with downloading the image from a remote store. + # Assume the upload is broken and save an empty string to prevent re-evaluation + "" rescue Discourse::Utils::CommandError => e # Timeout or unable to parse image # This can happen due to bad user input - ignore and save diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb index eec747ba080..8b4c41cf6f4 100644 --- a/lib/file_store/base_store.rb +++ b/lib/file_store/base_store.rb @@ -116,6 +116,8 @@ module FileStore follow_redirect: true ) + return nil if file.nil? + cache_file(file, filename) file = get_from_cache(filename) end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index a2441540ccc..fb4e6487a09 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -703,7 +703,7 @@ RSpec.describe Upload do expect(invalid_image.dominant_color).to eq(nil) end - it "correctly handles download failures" do + it "correctly handles error when file is too large to download" do white_image.stubs(:local?).returns(true) Discourse.store.stubs(:download).returns(nil) @@ -712,6 +712,15 @@ RSpec.describe Upload do expect(invalid_image.dominant_color).to eq("") end + it "correctly handles error when file has HTTP error" do + white_image.stubs(:local?).returns(true) + Discourse.store.stubs(:download).raises(OpenURI::HTTPError) + + expect(invalid_image.dominant_color).to eq(nil) + expect(invalid_image.dominant_color(calculate_if_missing: true)).to eq("") + expect(invalid_image.dominant_color).to eq("") + end + it "is validated for length" do u = Fabricate(:upload)