diff --git a/app/controllers/onebox_controller.rb b/app/controllers/onebox_controller.rb index 8478269e947..83f0efa9622 100644 --- a/app/controllers/onebox_controller.rb +++ b/app/controllers/onebox_controller.rb @@ -3,9 +3,7 @@ require_dependency 'oneboxer' class OneboxController < ApplicationController def show - Oneboxer.invalidate(params[:url]) if params[:refresh].present? - - result = Oneboxer.preview(params[:url]) + result = Oneboxer.onebox(params[:url], invalidate_oneboxes: params[:refresh] == 'true') result.strip! if result.present? # If there is no result, return a 404 diff --git a/app/models/onebox_render.rb b/app/models/onebox_render.rb deleted file mode 100644 index 9da3cfffa11..00000000000 --- a/app/models/onebox_render.rb +++ /dev/null @@ -1,8 +0,0 @@ -class OneboxRender < ActiveRecord::Base - validates_presence_of :url - validates_presence_of :cooked - validates_presence_of :expires_at - - has_many :post_onebox_renders, dependent: :delete_all - has_many :posts, through: :post_onebox_renders -end diff --git a/app/models/post.rb b/app/models/post.rb index 4408cc4910c..e6025eedce2 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -219,7 +219,7 @@ class Post < ActiveRecord::Base doc = Oneboxer.each_onebox_link(cooked) do |url, elem| cached = Oneboxer.render_from_cache(url) if cached.present? - elem.swap(cached.cooked) + elem.swap(cached) dirty = true end end @@ -268,7 +268,7 @@ class Post < ActiveRecord::Base # Various callbacks before_create do if reply_to_post_number.present? - self.reply_to_user_id ||= Post.select(:user_id).where(topic_id: topic_id, post_number: reply_to_post_number).first.try(:user_id) + self.reply_to_user_id ||= Post.select(:user_id).where(topic_id: topic_id, post_number: reply_to_post_number).first.try(:user_id) end self.post_number ||= Topic.next_post_number(topic_id, reply_to_post_number.present?) diff --git a/app/models/post_onebox_render.rb b/app/models/post_onebox_render.rb deleted file mode 100644 index cc3c0931bf8..00000000000 --- a/app/models/post_onebox_render.rb +++ /dev/null @@ -1,6 +0,0 @@ -class PostOneboxRender < ActiveRecord::Base - belongs_to :post - belongs_to :onebox_render - - validates_uniqueness_of :post_id, scope: :onebox_render_id -end diff --git a/db/migrate/20130321154905_remove_oneboxes_from_db.rb b/db/migrate/20130321154905_remove_oneboxes_from_db.rb new file mode 100644 index 00000000000..8cf4c1cfef6 --- /dev/null +++ b/db/migrate/20130321154905_remove_oneboxes_from_db.rb @@ -0,0 +1,9 @@ +class RemoveOneboxesFromDb < ActiveRecord::Migration + def up + drop_table :post_onebox_renders + drop_table :onebox_renders + end + + def down + end +end diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 1a02ea785d1..82b0d59625d 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -1,4 +1,5 @@ require 'open-uri' +require 'digest/sha1' require_dependency 'oneboxer/base' require_dependency 'oneboxer/whitelist' @@ -14,7 +15,7 @@ module Oneboxer end def self.default_expiry - 1.month + 1.day end # Return a oneboxer for a given URL @@ -79,72 +80,39 @@ module Oneboxer doc end - def self.create_post_reference(result, args={}) - result.post_onebox_renders.create(post_id: args[:post_id]) if args[:post_id].present? - rescue ActiveRecord::RecordNotUnique + def self.cache_key_for(url) + "onebox:#{Digest::SHA1.hexdigest(url)}" end - def self.render_from_cache(url, args={}) - result = OneboxRender.where(url: url).first - - # Return the result but also create a reference to it - if result.present? - create_post_reference(result, args) - return result - end - nil + def self.render_from_cache(url) + Rails.cache.read(cache_key_for(url)) end # Cache results from a onebox call def self.fetch_and_cache(url, args) - cooked, preview = onebox_nocache(url) - return nil if cooked.blank? + contents = onebox_nocache(url) + return nil if contents.blank? - # Store a cooked version in the database - OneboxRender.transaction do - begin - render = OneboxRender.create(url: url, preview: preview, cooked: cooked, expires_at: Oneboxer.default_expiry.from_now) - create_post_reference(render, args) - rescue ActiveRecord::RecordNotUnique - end - end - - [cooked, preview] - end - - # Retrieve a preview of a onebox, caching the result for performance - def self.preview(url, args={}) - cached = render_from_cache(url, args) unless args[:no_cache].present? - - # If we have a preview stored, return that. Otherwise return cooked content. - if cached.present? - return cached.preview if cached.preview.present? - return cached.cooked - end - cooked, preview = fetch_and_cache(url, args) - - return preview if preview.present? - cooked + Rails.cache.write(cache_key_for(url), contents, expires_in: default_expiry) + contents end def self.invalidate(url) - OneboxRender.destroy_all(url: url) + Rails.cache.delete(cache_key_for(url)) end # Return the cooked content for a url, caching the result for performance def self.onebox(url, args={}) - if args[:invalidate_oneboxes].present? + if args[:invalidate_oneboxes] # Remove the onebox from the cache Oneboxer.invalidate(url) else - cached = render_from_cache(url, args) unless args[:no_cache].present? - return cached.cooked if cached.present? + contents = render_from_cache(url) + return contents if contents.present? end - - cooked, preview = fetch_and_cache(url, args) - cooked + fetch_and_cache(url, args) end end diff --git a/spec/components/oneboxer_spec.rb b/spec/components/oneboxer_spec.rb index b2852bd98de..4a26e486ac1 100644 --- a/spec/components/oneboxer_spec.rb +++ b/spec/components/oneboxer_spec.rb @@ -46,16 +46,68 @@ describe Oneboxer do end end + let(:dummy_onebox_url) { "http://dummy.localhost/dummy-object" } before do Oneboxer.add_onebox DummyOnebox - @dummy_onebox_url = "http://dummy.localhost/dummy-object" end it 'should have matchers set up by default' do Oneboxer.matchers.should be_present end + context 'caching' do + + let(:result) { "onebox result string" } + + context "with invalidate_oneboxes true" do + + it "invalidates the url" do + Oneboxer.expects(:invalidate).with(dummy_onebox_url) + Oneboxer.onebox(dummy_onebox_url, invalidate_oneboxes: true) + end + + it "doesn't render from cache" do + Oneboxer.expects(:render_from_cache).never + Oneboxer.onebox(dummy_onebox_url, invalidate_oneboxes: true) + end + + it "calls fetch and cache" do + Oneboxer.expects(:fetch_and_cache).returns(result) + Oneboxer.onebox(dummy_onebox_url, invalidate_oneboxes: true).should == result + end + + end + + context 'with invalidate_oneboxes false' do + + it "doesn't invalidate the url" do + Oneboxer.expects(:invalidate).with(dummy_onebox_url).never + Oneboxer.onebox(dummy_onebox_url, invalidate_oneboxes: false) + end + + it "returns render_from_cache if present" do + Oneboxer.expects(:render_from_cache).with(dummy_onebox_url).returns(result) + Oneboxer.onebox(dummy_onebox_url, invalidate_oneboxes: false).should == result + end + + it "doesn't call fetch_and_cache" do + Oneboxer.expects(:render_from_cache).with(dummy_onebox_url).returns(result) + Oneboxer.expects(:fetch_and_cache).never + Oneboxer.onebox(dummy_onebox_url, invalidate_oneboxes: false) + end + + + it "calls fetch_and_cache if render from cache is blank" do + Oneboxer.stubs(:render_from_cache) + Oneboxer.expects(:fetch_and_cache).returns(result) + Oneboxer.onebox(dummy_onebox_url, invalidate_oneboxes: false).should == result + end + + end + + end + context 'find onebox for url' do it 'returns blank with an unknown url' do @@ -63,11 +115,11 @@ describe Oneboxer do end it 'returns something when matched' do - Oneboxer.onebox_for_url(@dummy_onebox_url).should be_present + Oneboxer.onebox_for_url(dummy_onebox_url).should be_present end it 'returns an instance of our class when matched' do - Oneboxer.onebox_for_url(@dummy_onebox_url).kind_of?(DummyOnebox).should be_true + Oneboxer.onebox_for_url(dummy_onebox_url).kind_of?(DummyOnebox).should be_true end end @@ -92,69 +144,10 @@ describe Oneboxer do context 'without caching' do it 'calls the onebox method of our matched class' do - Oneboxer.onebox_nocache(@dummy_onebox_url).should == 'dummy!' + Oneboxer.onebox_nocache(dummy_onebox_url).should == 'dummy!' end end - context 'with caching' do - - context 'initial cache is empty' do - - it 'has no OneboxRender records' do - OneboxRender.count.should == 0 - end - - it 'calls the onebox_nocache method if there is no cache record yet' do - Oneboxer.expects(:onebox_nocache).with(@dummy_onebox_url).once - Oneboxer.onebox(@dummy_onebox_url) - end - end - - context 'caching result' do - before do - @post = Fabricate(:post) - @result = Oneboxer.onebox(@dummy_onebox_url, post_id: @post.id) - @onebox_render = OneboxRender.where(url: @dummy_onebox_url).first - end - - it "returns the correct result" do - @result.should == 'dummy!' - end - - it "created a OneboxRender record with the url" do - @onebox_render.should be_present - end - - it "created a OneboxRender record with the url" do - @onebox_render.url.should == @dummy_onebox_url - end - - it "associated the render with a post" do - @onebox_render.posts.should == [@post] - end - - it "has an expires_at value" do - @onebox_render.expires_at.should be_present - end - - it "doesn't call onebox_nocache on a cache hit" do - Oneboxer.expects(:onebox_nocache).never - Oneboxer.onebox(@dummy_onebox_url).should == 'dummy!' - end - - context 'invalidating cache' do - - it "deletes the onebox render" do - Oneboxer.expects(:onebox_nocache).once.returns('new cache value!') - Oneboxer.onebox(@dummy_onebox_url, invalidate_oneboxes: true).should == 'new cache value!' - end - - end - - end - - end - context 'each_onebox_link' do before do diff --git a/spec/controllers/onebox_controller_spec.rb b/spec/controllers/onebox_controller_spec.rb index 89175acfd5f..26bf29da420 100644 --- a/spec/controllers/onebox_controller_spec.rb +++ b/spec/controllers/onebox_controller_spec.rb @@ -5,8 +5,8 @@ describe OneboxController do let(:url) { "http://google.com" } it 'invalidates the cache if refresh is passed' do - Oneboxer.expects(:invalidate).with(url) - xhr :get, :show, url: url, refresh: true + Oneboxer.expects(:onebox).with(url, invalidate_oneboxes: true) + xhr :get, :show, url: url, refresh: 'true' end describe "found onebox" do @@ -14,7 +14,7 @@ describe OneboxController do let(:body) { "this is the onebox body"} before do - Oneboxer.expects(:preview).with(url).returns(body) + Oneboxer.expects(:onebox).with(url, invalidate_oneboxes: false).returns(body) xhr :get, :show, url: url end @@ -31,13 +31,13 @@ describe OneboxController do describe "missing onebox" do it "returns 404 if the onebox is nil" do - Oneboxer.expects(:preview).with(url).returns(nil) + Oneboxer.expects(:onebox).with(url, invalidate_oneboxes: false).returns(nil) xhr :get, :show, url: url response.response_code.should == 404 end it "returns 404 if the onebox is an empty string" do - Oneboxer.expects(:preview).with(url).returns(" \t ") + Oneboxer.expects(:onebox).with(url, invalidate_oneboxes: false).returns(" \t ") xhr :get, :show, url: url response.response_code.should == 404 end diff --git a/spec/models/onebox_render_spec.rb b/spec/models/onebox_render_spec.rb deleted file mode 100644 index 637c2084039..00000000000 --- a/spec/models/onebox_render_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -require 'spec_helper' - -describe OneboxRender do - - it { should validate_presence_of :url } - it { should validate_presence_of :cooked } - it { should validate_presence_of :expires_at } - it { should have_many :post_onebox_renders } - it { should have_many :posts } - -end diff --git a/spec/models/post_onebox_render_spec.rb b/spec/models/post_onebox_render_spec.rb deleted file mode 100644 index 21b52996092..00000000000 --- a/spec/models/post_onebox_render_spec.rb +++ /dev/null @@ -1,8 +0,0 @@ -require 'spec_helper' - -describe PostOneboxRender do - - it { should belong_to :onebox_render } - it { should belong_to :post } - -end