diff --git a/app/controllers/onebox_controller.rb b/app/controllers/onebox_controller.rb index 10c500317b7..f656ba34146 100644 --- a/app/controllers/onebox_controller.rb +++ b/app/controllers/onebox_controller.rb @@ -19,6 +19,8 @@ class OneboxController < ApplicationController invalidate = params[:refresh] == 'true' url = params[:url] + return render(body: nil, status: 404) if Oneboxer.recently_failed?(url) + hijack do Oneboxer.preview_onebox!(user_id) @@ -34,6 +36,7 @@ class OneboxController < ApplicationController Oneboxer.onebox_previewed!(user_id) if preview.blank? + Oneboxer.cache_failed!(url) render body: nil, status: 404 else render plain: preview diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 695f2f19149..a594499f266 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -72,6 +72,7 @@ module Oneboxer def self.invalidate(url) Discourse.cache.delete(onebox_cache_key(url)) + Discourse.cache.delete(onebox_failed_cache_key(url)) end # Parse URLs out of HTML, returning the document when finished. @@ -136,6 +137,14 @@ module Oneboxer Onebox::Matcher.new(url).oneboxed end + def self.recently_failed?(url) + Discourse.cache.read(onebox_failed_cache_key(url)).present? + end + + def self.cache_failed!(url) + Discourse.cache.write(onebox_failed_cache_key(url), true, expires_in: 1.hour) + end + private def self.preview_key(user_id) @@ -150,6 +159,10 @@ module Oneboxer "onebox__#{url}" end + def self.onebox_failed_cache_key(url) + "onebox_failed__#{url}" + end + def self.onebox_raw(url, opts = {}) url = URI(url).to_s local_onebox(url, opts) || external_onebox(url) diff --git a/spec/components/oneboxer_spec.rb b/spec/components/oneboxer_spec.rb index f90eb5ba755..c04287ecb9d 100644 --- a/spec/components/oneboxer_spec.rb +++ b/spec/components/oneboxer_spec.rb @@ -12,6 +12,17 @@ describe Oneboxer do expect(Oneboxer.onebox("http://boom.com")).to eq("") end + describe "#invalidate" do + let(:url) { "http://test.com" } + it "clears the cached preview for the onebox URL and the failed URL cache" do + Discourse.cache.write(Oneboxer.onebox_cache_key(url), "test") + Discourse.cache.write(Oneboxer.onebox_failed_cache_key(url), true) + Oneboxer.invalidate(url) + expect(Discourse.cache.read(Oneboxer.onebox_cache_key(url))).to eq(nil) + expect(Discourse.cache.read(Oneboxer.onebox_failed_cache_key(url))).to eq(nil) + end + end + context "local oneboxes" do def link(url) diff --git a/spec/requests/onebox_controller_spec.rb b/spec/requests/onebox_controller_spec.rb index 1d14499e414..babe3104760 100644 --- a/spec/requests/onebox_controller_spec.rb +++ b/spec/requests/onebox_controller_spec.rb @@ -4,6 +4,10 @@ require 'rails_helper' describe OneboxController do + before do + Discourse.cache.delete(Oneboxer.onebox_failed_cache_key(url)) + end + let(:url) { "http://google.com" } it "requires the user to be logged in" do @@ -112,19 +116,34 @@ describe OneboxController do end describe "missing onebox" do - it "returns 404 if the onebox is nil" do + def stub_request_to_onebox_url(response_body) stub_request(:head, url) - stub_request(:get, url).to_return(body: nil).then.to_raise + stub_request(:get, url).to_return(body: response_body).then.to_raise + end + + it "returns 404 if the onebox is nil" do + stub_request_to_onebox_url(nil) get "/onebox.json", params: { url: url, refresh: "true" } expect(response.response_code).to eq(404) end it "returns 404 if the onebox is an empty string" do - stub_request(:head, url) - stub_request(:get, url).to_return(body: " \t ").then.to_raise + stub_request_to_onebox_url(" \t ") get "/onebox.json", params: { url: url, refresh: "true" } expect(response.response_code).to eq(404) end + + it "cases missing onebox URLs so we do not attempt to preview again" do + stub_request_to_onebox_url(nil) + get "/onebox.json", params: { url: url, refresh: "true" } + expect(response.response_code).to eq(404) + Oneboxer.expects(:preview_onebox!).never + get "/onebox.json", params: { url: url, refresh: "true" } + expect(response.response_code).to eq(404) + expect( + Discourse.cache.read(Oneboxer.onebox_failed_cache_key(url)) + ).not_to eq(nil) + end end describe "local onebox" do