From 0920c4bea683e12821b416f1f174cdfbae17b082 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 4 Aug 2014 11:06:06 +1000 Subject: [PATCH] PERF: reduce storage requirements for incoming links Only store incoming links for topics. --- app/controllers/application_controller.rb | 5 -- app/controllers/topics_controller.rb | 15 ++++ app/models/incoming_link.rb | 71 +++++++-------- .../20140801052028_fix_incoming_links.rb | 21 +++++ .../application_controller_spec.rb | 20 ----- spec/controllers/topics_controller_spec.rb | 18 ++++ spec/fabricators/incoming_link_fabricator.rb | 9 -- spec/models/incoming_link_spec.rb | 90 +++++++++---------- 8 files changed, 129 insertions(+), 120 deletions(-) create mode 100644 db/migrate/20140801052028_fix_incoming_links.rb delete mode 100644 spec/fabricators/incoming_link_fabricator.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1babd3edb..3f06794b6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -36,7 +36,6 @@ class ApplicationController < ActionController::Base before_filter :disable_customization before_filter :block_if_readonly_mode before_filter :authorize_mini_profiler - before_filter :store_incoming_links before_filter :preload_json before_filter :check_xhr before_filter :redirect_to_login_if_required @@ -313,10 +312,6 @@ class ApplicationController < ActionController::Base Rack::MiniProfiler.authorize_request end - def store_incoming_links - IncomingLink.add(request, current_user) unless request.xhr? - end - def check_xhr # bypass xhr check on PUT / POST / DELETE provided api key is there, otherwise calling api is annoying return if !request.get? && api_key_valid? diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 290b8c82e..5a214a11d 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -31,6 +31,9 @@ class TopicsController < ApplicationController skip_before_filter :check_xhr, only: [:show, :feed] def show + + flash["referer"] ||= request.referer + # We'd like to migrate the wordpress feed to another url. This keeps up backwards compatibility with # existing installs. return wordpress if params[:best].present? @@ -380,6 +383,18 @@ class TopicsController < ApplicationController user_id = (current_user.id if current_user) track_visit = should_track_visit_to_topic? + Scheduler::Defer.later "Track Link" do + IncomingLink.add( + referer: request.referer || flash[:referer], + host: request.host, + current_user: current_user, + topic_id: @topic_view.topic.id, + post_number: params[:post_number], + username: request['u'], + ip_address: request.remote_ip + ) + end unless request.xhr? + Scheduler::Defer.later "Track Visit" do View.create_for_parent(Topic, topic_id, ip, user_id) if track_visit diff --git a/app/models/incoming_link.rb b/app/models/incoming_link.rb index 45d6513c6..f1080e2a1 100644 --- a/app/models/incoming_link.rb +++ b/app/models/incoming_link.rb @@ -2,38 +2,50 @@ class IncomingLink < ActiveRecord::Base belongs_to :topic belongs_to :user - validates :url, presence: true validate :referer_valid + validate :post_id, presence: true before_validation :extract_domain - before_validation :extract_topic_and_post after_create :update_link_counts - def self.add(request,current_user=nil) - user_id, host, referer = nil + attr_accessor :url - if request['u'] - u = User.select(:id).find_by(username_lower: request["u"].downcase) + def self.add(opts) + user_id, host, referer = nil + current_user = opts[:current_user] + + if username = opts[:username] + u = User.select(:id).find_by(username_lower: username.downcase) user_id = u.id if u end - if request.referer.present? + if opts[:referer].present? begin - host = URI.parse(request.referer).host - referer = request.referer[0..999] + host = URI.parse(opts[:referer]).host + referer = opts[:referer][0..999] rescue URI::InvalidURIError # bad uri, skip end end - if host != request.host && (user_id || referer) + if host != opts[:host] && (user_id || referer) + + post_id = opts[:post_id] + post_id ||= Post.where(topic_id: opts[:topic_id], + post_number: opts[:post_number] || 1) + .pluck(:id).first + cid = current_user ? (current_user.id) : (nil) + + unless cid && cid == user_id - IncomingLink.create(url: request.url, - referer: referer, + + IncomingLink.create(referer: referer, user_id: user_id, + post_id: post_id, current_user_id: cid, - ip_address: request.remote_ip) + ip_address: opts[:ip_address]) if post_id + end end @@ -49,35 +61,14 @@ class IncomingLink < ActiveRecord::Base end end - # Internal: If link is internal and points to topic/post, extract their IDs. - def extract_topic_and_post - if url.present? - parsed = URI.parse(url) - - begin - # TODO achieve same thing with no exception - params = Rails.application.routes.recognize_path(parsed.path) - if self.topic_id = params[:topic_id] - self.post_number = params[:post_number] || 1 - end - rescue ActionController::RoutingError - # If we can't route to the url, that's OK. Don't save those two fields. - end - end - end - # Internal: Update appropriate link counts. def update_link_counts - if topic_id.present? - exec_sql("UPDATE topics - SET incoming_link_count = incoming_link_count + 1 - WHERE id = ?", topic_id) - if post_number.present? - exec_sql("UPDATE posts - SET incoming_link_count = incoming_link_count + 1 - WHERE topic_id = ? and post_number = ?", topic_id, post_number) - end - end + exec_sql("UPDATE topics + SET incoming_link_count = incoming_link_count + 1 + WHERE id = (SELECT topic_id FROM posts where id = ?)", post_id) + exec_sql("UPDATE posts + SET incoming_link_count = incoming_link_count + 1 + WHERE id = ?", post_id) end protected diff --git a/db/migrate/20140801052028_fix_incoming_links.rb b/db/migrate/20140801052028_fix_incoming_links.rb new file mode 100644 index 000000000..c00ac021a --- /dev/null +++ b/db/migrate/20140801052028_fix_incoming_links.rb @@ -0,0 +1,21 @@ +class FixIncomingLinks < ActiveRecord::Migration + def up + execute "DROP INDEX incoming_index" + add_column :incoming_links, :post_id, :integer + remove_column :incoming_links, :updated_at + remove_column :incoming_links, :url + + execute "UPDATE incoming_links l SET post_id = ( + SELECT p.id FROM posts p WHERE p.topic_id = l.topic_id AND p.post_number = l.post_number + )" + + execute "DELETE FROM incoming_links WHERE post_id IS NULL" + change_column :incoming_links, :post_id, :integer, null: false + + add_index :incoming_links, :post_id + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 48c5551a5..c927de399 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -24,13 +24,6 @@ describe TopicsController do lambda { get :show, {id: topic.id} }.should_not raise_error end - it "stores an incoming link when there is an off-site referer" do - lambda { - set_referer("http://google.com/search") - get :show, {id: topic.id} - }.should change(IncomingLink, :count).by(1) - end - describe "has_escaped_fragment?" do render_views @@ -92,19 +85,6 @@ describe TopicsController do end - describe 'after inserting an incoming link' do - - it 'sets last link correctly' do - set_referer("http://google.com/search") - get :show, {topic_id: topic.id} - - last_link = IncomingLink.last - last_link.topic_id.should == topic.id - last_link.post_number.should == 1 - end - - end - describe 'set_locale' do it 'sets the one the user prefers' do SiteSetting.stubs(:allow_user_locale).returns(true) diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 897c008f3..3b20a8adf 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -590,6 +590,24 @@ describe TopicsController do lambda { xhr :get, :show, topic_id: topic.id, slug: topic.slug }.should change(View, :count).by(1) end + it 'records incoming links' do + user = Fabricate(:user) + get :show, topic_id: topic.id, slug: topic.slug, u: user.username + + IncomingLink.count.should == 1 + end + + it 'records redirects' do + @request.env['HTTP_REFERER'] = 'http://twitter.com' + get :show, { id: topic.id } + + @request.env['HTTP_REFERER'] = nil + get :show, topic_id: topic.id, slug: topic.slug + + link = IncomingLink.first + link.referer.should == 'http://twitter.com' + end + it 'tracks a visit for all html requests' do current_user = log_in(:coding_horror) TopicUser.expects(:track_visit!).with(topic.id, current_user.id) diff --git a/spec/fabricators/incoming_link_fabricator.rb b/spec/fabricators/incoming_link_fabricator.rb deleted file mode 100644 index baec9e3d4..000000000 --- a/spec/fabricators/incoming_link_fabricator.rb +++ /dev/null @@ -1,9 +0,0 @@ -Fabricator(:incoming_link) do - url 'http://localhost:3000/t/pinball/76/6' - referer 'https://twitter.com/evil_trout' -end - -Fabricator(:incoming_link_not_topic, from: :incoming_link) do - url 'http://localhost:3000/made-up-url' - referer 'https://twitter.com/evil_trout' -end diff --git a/spec/models/incoming_link_spec.rb b/spec/models/incoming_link_spec.rb index fb9af72f1..255eb5ebd 100644 --- a/spec/models/incoming_link_spec.rb +++ b/spec/models/incoming_link_spec.rb @@ -3,95 +3,93 @@ require 'spec_helper' describe IncomingLink do it { should belong_to :topic } - it { should validate_presence_of :url } let(:post) { Fabricate(:post) } - let(:topic) { post.topic } let :incoming_link do - IncomingLink.create(url: "/t/slug/#{topic.id}/#{post.post_number}", - referer: "http://twitter.com") + IncomingLink.add(host: "a.com", referer: "http://twitter.com", post_id: post.id, ip_address: '1.1.1.1') end describe 'local topic link' do - it 'should validate properly' do - Fabricate.build(:incoming_link).should be_valid - end - describe 'tracking link counts' do it "increases the incoming link counts" do - incoming_link - lambda { post.reload }.should change(post, :incoming_link_count).by(1) - lambda { topic.reload }.should change(topic, :incoming_link_count).by(1) + link = incoming_link + + link.domain.should == "twitter.com" + link.post_id.should == post.id + + post.reload + post.incoming_link_count.should == 1 + + topic.reload + topic.incoming_link_count.should == 1 end end - describe 'saving local link' do - it 'has correct info set' do - incoming_link.domain.should == "twitter.com" - incoming_link.topic_id.should == topic.id - incoming_link.post_number.should == post.post_number - end - - end end describe 'add' do - class TestRequest