From 76c86a426910d84602018ee2063c0b68b0056bb6 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 12 Oct 2022 15:50:44 +0100 Subject: [PATCH] FIX: Correctly handle HTTP errors during dominant color calculation (#18565) The previous fix in e83d35d6 was incorrect, and the stub in the test was never actually hit. This commit moves the error handling to the right place and updates the specs to ensure the stub is always used. --- app/models/upload.rb | 12 +++++++----- spec/models/upload_spec.rb | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/models/upload.rb b/app/models/upload.rb index 8ffbbe99c7e..813c5e57e91 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -340,7 +340,13 @@ class Upload < ActiveRecord::Base if local? Discourse.store.path_for(self) else - Discourse.store.download(self)&.path + begin + Discourse.store.download(self)&.path + 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 + nil + end end if local_path.nil? @@ -373,10 +379,6 @@ 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/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index fb4e6487a09..642061904e3 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -704,21 +704,21 @@ RSpec.describe Upload do end it "correctly handles error when file is too large to download" do - white_image.stubs(:local?).returns(true) - Discourse.store.stubs(:download).returns(nil) + white_image.stubs(:local?).returns(false) + FileStore::LocalStore.any_instance.stubs(:download).returns(nil).once - 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("") + expect(white_image.dominant_color).to eq(nil) + expect(white_image.dominant_color(calculate_if_missing: true)).to eq("") + expect(white_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) + white_image.stubs(:local?).returns(false) + FileStore::LocalStore.any_instance.stubs(:download).raises(OpenURI::HTTPError.new("Error", nil)).once - 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("") + expect(white_image.dominant_color).to eq(nil) + expect(white_image.dominant_color(calculate_if_missing: true)).to eq("") + expect(white_image.dominant_color).to eq("") end it "is validated for length" do