From b6bf95e74183fbfb5fee73441247d80f52e2081a Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 13 May 2013 18:04:03 +1000 Subject: [PATCH] speed up startup (avoid loading some gems on startup) correct group permission leaks add Discourse.cache for richer caching support --- Gemfile | 29 +++++----- Gemfile.lock | 1 + Guardfile | 4 ++ app/controllers/application_controller.rb | 2 +- app/controllers/search_controller.rb | 2 +- app/models/category.rb | 10 ++++ app/models/category_list.rb | 15 ++---- app/models/site.rb | 23 +++++--- app/models/user.rb | 3 +- app/serializers/site_serializer.rb | 2 +- lib/cache.rb | 55 +++++++++++++++++++ lib/diff_engine.rb | 1 + lib/discourse.rb | 7 ++- lib/guardian.rb | 4 +- lib/search.rb | 50 ++++++++++------- lib/tasks/autospec.rake | 7 +++ spec/components/cache_spec.rb | 63 ++++++++++++++++++++++ spec/components/category_list_spec.rb | 8 +-- spec/components/search_spec.rb | 16 ++++-- spec/controllers/search_controller_spec.rb | 8 ++- spec/spec_helper.rb | 6 +++ 21 files changed, 250 insertions(+), 66 deletions(-) create mode 100644 lib/cache.rb create mode 100644 spec/components/cache_spec.rb diff --git a/Gemfile b/Gemfile index e56f22086..353764973 100644 --- a/Gemfile +++ b/Gemfile @@ -63,7 +63,7 @@ gem 'sinatra', require: nil gem 'slim' # required for sidekiq-web gem 'therubyracer', require: 'v8' gem 'thin' -gem 'diffy' +gem 'diffy', require: false # Gem that enables support for plugins. It is required. gem 'discourse_plugin', path: 'vendor/gems/discourse_plugin' @@ -86,26 +86,27 @@ group :assets do end group :test do - gem 'fakeweb', '~> 1.3.0' - gem 'minitest' + gem 'fakeweb', '~> 1.3.0', require: false + gem 'minitest', require: false end group :test, :development do gem 'jshint_on_rails' - gem 'guard-jshint-on-rails' - gem 'certified' - gem 'fabrication' - gem 'guard-jasmine' - gem 'guard-rspec' - gem 'guard-spork' + gem 'listen', require: false + gem 'guard-jshint-on-rails', require: false + gem 'certified', require: false + gem 'fabrication', require: false + gem 'guard-jasmine', require: false + gem 'guard-rspec', require: false + gem 'guard-spork', require: false gem 'jasminerice' gem 'mocha', require: false - gem 'rb-fsevent' - gem 'rb-inotify', '~> 0.9', require: RUBY_PLATFORM.include?('linux') && 'rb-inotify' - gem 'rspec-rails' - gem 'shoulda' + gem 'rb-fsevent', require: false + gem 'rb-inotify', '~> 0.9', require: false + gem 'rspec-rails', require: false + gem 'shoulda', require: false gem 'simplecov', require: false - gem 'terminal-notifier-guard', require: RUBY_PLATFORM.include?('darwin') && 'terminal-notifier-guard' + gem 'terminal-notifier-guard', require: false end group :development do diff --git a/Gemfile.lock b/Gemfile.lock index 740bce767..b2291f355 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -482,6 +482,7 @@ DEPENDENCIES jquery-rails jshint_on_rails librarian (>= 0.0.25) + listen lru_redux message_bus! minitest diff --git a/Guardfile b/Guardfile index 428033848..74368bc63 100644 --- a/Guardfile +++ b/Guardfile @@ -1,3 +1,6 @@ +require 'rb-inotify' if RUBY_PLATFORM.include?('linux') +require 'terminal-notifier-guard' if RUBY_PLATFORM.include?('darwin') + phantom_path = File.expand_path('~/phantomjs/bin/phantomjs') phantom_path = nil unless File.exists?(phantom_path) @@ -27,6 +30,7 @@ guard 'jshint-on-rails', config_path: 'config/jshint.yml' do end unless ENV["USING_AUTOSPEC"] + puts "Sam strongly recommends you Run: `bundle exec rake autospec` in favor of guard for specs, set USING_AUTOSPEC in .rvmrc to disable from Guard" guard :spork, wait: 120 do watch('config/application.rb') diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e22227523..0dda1353a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -99,7 +99,7 @@ class ApplicationController < ActionController::Base guardian.current_user.sync_notification_channel_position end - store_preloaded("site", Site.cached_json) + store_preloaded("site", Site.cached_json(current_user)) if current_user.present? store_preloaded("currentUser", MultiJson.dump(CurrentUserSerializer.new(current_user, root: false))) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 4cdd4d2b6..266507070 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -3,7 +3,7 @@ require_dependency 'search' class SearchController < ApplicationController def query - search_result = Search.query(params[:term], current_user, params[:type_filter], SiteSetting.min_search_term_length) + search_result = Search.query(params[:term], guardian, params[:type_filter], SiteSetting.min_search_term_length) render_json_dump(search_result.as_json) end diff --git a/app/models/category.rb b/app/models/category.rb index d4d37930c..6ce6d3afc 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -29,6 +29,16 @@ class Category < ActiveRecord::Base scope :latest, ->{ order('topic_count desc') } + scope :secured, lambda { |guardian| + ids = nil + ids = guardian.secure_category_ids if guardian + if ids.present? + where("categories.secure ='f' or categories.id = :cats ", cats: ids) + else + where("categories.secure ='f'") + end + } + delegate :post_template, to: 'self.class' # Internal: Update category stats: # of topics in past year, month, week for diff --git a/app/models/category_list.rb b/app/models/category_list.rb index cca2b731e..9625281ea 100644 --- a/app/models/category_list.rb +++ b/app/models/category_list.rb @@ -3,19 +3,14 @@ class CategoryList attr_accessor :categories, :topic_users, :uncategorized - def initialize(current_user) + def initialize(guardian) @categories = Category .includes(featured_topics: [:category]) .includes(:featured_users) .where('topics.visible' => true) + .secured(guardian) .order('categories.topics_week desc, categories.topics_month desc, categories.topics_year desc') - allowed_ids = current_user ? current_user.secure_category_ids : nil - if allowed_ids.present? - @categories = @categories.where("categories.secure = 'f' OR categories.id in (?)", allowed_ids) - else - @categories = @categories.where("categories.secure = 'f'") - end @categories = @categories.to_a @@ -56,7 +51,7 @@ class CategoryList @categories.insert(insert_at || @categories.size, uncategorized) end - unless Guardian.new(current_user).can_create?(Category) + unless guardian.can_create?(Category) # Remove categories with no featured topics unless we have the ability to edit one @categories.delete_if { |c| c.featured_topics.blank? } else @@ -69,7 +64,7 @@ class CategoryList end # Get forum topic user records if appropriate - if current_user.present? + if guardian.current_user topics = [] @categories.each { |c| topics << c.featured_topics } topics << @uncategorized @@ -77,7 +72,7 @@ class CategoryList topics.flatten! if topics.present? topics.compact! if topics.present? - topic_lookup = TopicUser.lookup_for(current_user, topics) + topic_lookup = TopicUser.lookup_for(guardian.current_user, topics) # Attach some data for serialization to each topic topics.each { |ft| ft.user_data = topic_lookup[ft.id] } diff --git a/app/models/site.rb b/app/models/site.rb index 692f5b2e8..e94871d24 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -5,6 +5,10 @@ require_dependency 'trust_level' class Site include ActiveModel::Serialization + def initialize(guardian) + @guardian = guardian + end + def site_setting SiteSetting end @@ -22,27 +26,32 @@ class Site end def categories - Category.latest.includes(:topic_only_relative_url) + Category + .secured(@guardian) + .latest + .includes(:topic_only_relative_url) end def archetypes Archetype.list.reject { |t| t.id == Archetype.private_message } end - def self.cache_key - "site_json" + def cache_key + k ="site_json_cats_" + k << @guardian.secure_category_ids.join("_") if @guardian end - def self.cached_json + def self.cached_json(guardian) # Sam: bumping this way down, SiteSerializer will serialize post actions as well, # On my local this was not being flushed as post actions types changed, it turn this # broke local. - Rails.cache.fetch(Site.cache_key, expires_in: 1.minute) do - MultiJson.dump(SiteSerializer.new(Site.new, root: false)) + site = Site.new(guardian) + Discourse.cache.fetch(site.cache_key, family: "site", expires_in: 1.minute) do + MultiJson.dump(SiteSerializer.new(site, root: false)) end end def self.invalidate_cache - Rails.cache.delete(Site.cache_key) + Discourse.cache.delete_by_family("site") end end diff --git a/app/models/user.rb b/app/models/user.rb index 13abe4fbc..d798d9793 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -565,9 +565,10 @@ class User < ActiveRecord::Base def secure_category_ids cats = self.staff? ? Category.select(:id).where(secure: true) : secure_categories.select('categories.id') - cats.map{|c| c.id} + cats.map{|c| c.id}.sort end + protected def cook diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 119025eb6..7fde96ffe 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -4,7 +4,7 @@ class SiteSerializer < ApplicationSerializer :notification_types, :post_types - has_many :categories, embed: :objects + has_many :categories, serializer: BasicCategorySerializer, embed: :objects has_many :post_action_types, embed: :objects has_many :trust_levels, embed: :objects has_many :archetypes, embed: :objects, serializer: ArchetypeSerializer diff --git a/lib/cache.rb b/lib/cache.rb new file mode 100644 index 000000000..1ed4b2ac3 --- /dev/null +++ b/lib/cache.rb @@ -0,0 +1,55 @@ +# Standard Rails.cache is lacking support for this interface, possibly yank all in from redis:cache and start using this instead +# + +class Cache + def initialize(redis=nil) + @redis = redis + end + + def redis + @redis || $redis + end + + def fetch(key, options={}) + result = redis.get key + if result.nil? + if expiry = options[:expires_in] + if block_given? + result = yield + redis.setex(key, expiry, result) + end + else + if block_given? + result = yield + redis.set(key, result) + end + end + end + + if family = family_key(options[:family]) + redis.sadd(family, key) + end + + result + end + + def delete(key) + redis.del(key) + end + + def delete_by_family(key) + k = family_key(key) + redis.smembers(k).each do |member| + delete(member) + end + redis.del(k) + end + + private + + def family_key(name) + if name + "FAMILY_#{name}" + end + end +end diff --git a/lib/diff_engine.rb b/lib/diff_engine.rb index 213e6da28..bbcd5e573 100644 --- a/lib/diff_engine.rb +++ b/lib/diff_engine.rb @@ -1,3 +1,4 @@ +require 'diffy' # This class is used to generate diffs, it will be consumed by the UI on # on the client the displays diffs. # diff --git a/lib/discourse.rb b/lib/discourse.rb index 54a0d8623..d621cb12e 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -1,3 +1,5 @@ +require 'cache' + module Discourse # When they try to do something they should be logged in for @@ -12,6 +14,9 @@ module Discourse # When something they want is not found class NotFound < Exception; end + def self.cache + @cache ||= Cache.new + end # Get the current base URL for the current site def self.current_hostname @@ -20,7 +25,7 @@ module Discourse def self.base_uri default_value="" if !ActionController::Base.config.relative_url_root.blank? - return ActionController::Base.config.relative_url_root + return ActionController::Base.config.relative_url_root else return default_value end diff --git a/lib/guardian.rb b/lib/guardian.rb index d18a2efef..6e2ec95d6 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -364,7 +364,7 @@ class Guardian return true unless category.secure return false unless @user - @user.secure_category_ids.include?(category.id) + secure_category_ids.include?(category.id) end def can_vote?(post, opts={}) @@ -405,6 +405,6 @@ class Guardian end def secure_category_ids - @user ? @user.secure_category_ids : [] + @secure_category_ids ||= @user ? @user.secure_category_ids : [] end end diff --git a/lib/search.rb b/lib/search.rb index b68fdc5a5..908f4fd38 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -24,7 +24,7 @@ module Search " end - def self.post_query(current_user, type, args) + def self.post_query(guardian, type, args) builder = SqlBuilder.new < '#{Archetype.private_message}' SQL + add_allowed_categories(builder, guardian) + + builder.exec(args) + end + + def self.add_allowed_categories(builder, guardian) allowed_categories = nil - allowed_categories = current_user.secure_category_ids if current_user + allowed_categories = guardian.secure_category_ids if allowed_categories.present? builder.where("(c.id IS NULL OR c.secure = 'f' OR c.id in (:category_ids))", category_ids: allowed_categories) else builder.where("(c.id IS NULL OR c.secure = 'f')") end - - builder.exec(args) end - def self.category_query_sql - "SELECT 'category' AS type, + def self.category_query(guardian, args) + builder = SqlBuilder.new < 0 - tmp = post_query(current_user, :post, args.merge(limit: expected_topics * 3)) + tmp = post_query(guardian, :post, args.merge(limit: expected_topics * 3)) topic_ids = Set.new db_result.map{|r| r["id"]} diff --git a/lib/tasks/autospec.rake b/lib/tasks/autospec.rake index 2f5932219..c4ca2008c 100644 --- a/lib/tasks/autospec.rake +++ b/lib/tasks/autospec.rake @@ -4,6 +4,13 @@ desc "Run all specs automatically as needed" task "autospec" => :environment do + + if RUBY_PLATFORM.include?('linux') + require 'rb-inotify' + end + + require 'listen' + puts "If file watching is not working you can force polling with: bundle exec rake autospec p l=3" require 'autospec/runner' diff --git a/spec/components/cache_spec.rb b/spec/components/cache_spec.rb new file mode 100644 index 000000000..011a73464 --- /dev/null +++ b/spec/components/cache_spec.rb @@ -0,0 +1,63 @@ +require 'spec_helper' +require 'cache' + +describe Cache do + + let :cache do + Cache.new + end + + it "can delete by family" do + cache.fetch("key2", family: "my_family") do + "test" + end + + cache.fetch("key", expires_in: 1.minute, family: "my_family") do + "test" + end + + cache.delete_by_family("my_family") + cache.fetch("key").should be_nil + cache.fetch("key2").should be_nil + + end + + it "can delete correctly" do + r = cache.fetch("key", expires_in: 1.minute) do + "test" + end + + cache.delete("key") + cache.fetch("key").should be_nil + end + + it "can store with expiry correctly" do + $redis.expects(:get).with("key").returns nil + $redis.expects(:setex).with("key", 60 , "bob") + + r = cache.fetch("key", expires_in: 1.minute) do + "bob" + end + r.should == "bob" + end + + it "can store and fetch correctly" do + $redis.expects(:get).with("key").returns nil + $redis.expects(:set).with("key", "bob") + + r = cache.fetch "key" do + "bob" + end + r.should == "bob" + end + + it "can fetch existing correctly" do + + $redis.expects(:get).with("key").returns "bill" + + r = cache.fetch "key" do + "bob" + end + r.should == "bill" + end +end diff --git a/spec/components/category_list_spec.rb b/spec/components/category_list_spec.rb index dbea089c5..b3903a630 100644 --- a/spec/components/category_list_spec.rb +++ b/spec/components/category_list_spec.rb @@ -4,7 +4,7 @@ require 'category_list' describe CategoryList do let(:user) { Fabricate(:user) } - let(:category_list) { CategoryList.new(user) } + let(:category_list) { CategoryList.new(Guardian.new user) } context "with no categories" do @@ -39,9 +39,9 @@ describe CategoryList do cat.allow(Group[:admins]) cat.save - CategoryList.new(admin).categories.count.should == 1 - CategoryList.new(user).categories.count.should == 0 - CategoryList.new(nil).categories.count.should == 0 + CategoryList.new(Guardian.new admin).categories.count.should == 1 + CategoryList.new(Guardian.new user).categories.count.should == 0 + CategoryList.new(Guardian.new nil).categories.count.should == 0 end end diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 481dd20c7..0e6284036 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -164,12 +164,20 @@ describe Search do context 'categories' do let!(:category) { Fabricate(:category) } - let(:result) { first_of_type(Search.query('amazing', nil), 'category') } + def result + first_of_type(Search.query('amazing', nil), 'category') + end it 'returns the correct result' do - result.should be_present - result['title'].should == category.name - result['url'].should == "/category/#{category.slug}" + r = result + r.should be_present + r['title'].should == category.name + r['url'].should == "/category/#{category.slug}" + + category.deny(:all) + category.save + + result.should_not be_present end end diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index 89df481e0..ae6bdfc87 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -3,12 +3,16 @@ require 'spec_helper' describe SearchController do it 'performs the query' do - Search.expects(:query).with('test', nil, nil, 3) + m = Guardian.new(nil) + Guardian.stubs(:new).returns(m) + Search.expects(:query).with('test', m, nil, 3) xhr :get, :query, term: 'test' end it 'performs the query with a filter' do - Search.expects(:query).with('test', nil, 'topic', 3) + m = Guardian.new(nil) + Guardian.stubs(:new).returns(m) + Search.expects(:query).with('test', m, 'topic', 3) xhr :get, :query, term: 'test', type_filter: 'topic' end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5557dabea..bcaa409dc 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -33,10 +33,16 @@ Spork.prefork do # Loading more in this block will cause your tests to run faster. However, # if you change any configuration or code from libraries loaded here, you'll # need to restart spork for it take effect. + require 'fabrication' + require 'mocha/api' + require 'fakeweb' + require 'certified' + ENV["RAILS_ENV"] ||= 'test' require File.expand_path("../../config/environment", __FILE__) require 'rspec/rails' require 'rspec/autorun' + require 'shoulda' # Requires supporting ruby files with custom matchers and macros, etc, # in spec/support/ and its subdirectories.