Merge remote-tracking branch 'public/project-find-with-namespace-performance'

CHANGELOG

@@ -5,6 +5,7 @@ v 8.1.0 (unreleased)
   - Allow removing of project without confirmation when JavaScript is disabled (Stan Hu)
   - Support filtering by "Any" milestone or issue and fix "No Milestone" and "No Label" filters (Stan Hu)
   - Improved performance of the trending projects page
+  - Improved performance of finding projects by their namespace
   - Fix bug where transferring a project would result in stale commit links (Stan Hu)
   - Include full path of source and target branch names in New Merge Request page (Stan Hu)
   - Add user preference to view activities as default dashboard (Stan Hu)

app/models/concerns/case_sensitivity.rb

+# Concern for querying columns with specific case sensitivity handling.
+module CaseSensitivity
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    # Queries the given columns regardless of the casing used.
+    #
+    # Unlike other ActiveRecord methods this method only operates on a Hash.
+    def iwhere(params)
+      criteria   = self
+      cast_lower = Gitlab::Database.postgresql?
+
+      params.each do |key, value|
+        column = ActiveRecord::Base.connection.quote_table_name(key)
+
+        if cast_lower
+          condition = "LOWER(#{column}) = LOWER(:value)"
+        else
+          condition = "#{column} = :value"
+        end
+
+        criteria = criteria.where(condition, value: value)
+      end
+
+      criteria
+    end
+  end
+end

app/models/project.rb

@@ -40,6 +40,7 @@ class Project < ActiveRecord::Base
   include Referable
   include Sortable
   include AfterCommitQueue
+  include CaseSensitivity
 
   extend Gitlab::ConfigHelper
   extend Enumerize
@@ -235,13 +236,18 @@ class Project < ActiveRecord::Base
     end
 
     def find_with_namespace(id)
-      return nil unless id.include?('/')
-
-      id = id.split('/')
-      namespace = Namespace.by_path(id.first)
-      return nil unless namespace
-
-      where(namespace_id: namespace.id).where("LOWER(projects.path) = :path", path: id.second.downcase).first
+      namespace_path, project_path = id.split('/')
+
+      return nil if !namespace_path || !project_path
+
+      # Use of unscoped ensures we're not secretly adding any ORDER BYs, which
+      # have a negative impact on performance (and aren't needed for this
+      # query).
+      unscoped.
+        joins(:namespace).
+        iwhere('namespaces.path' => namespace_path).
+        iwhere('projects.path' => project_path).
+        take
     end
 
     def visibility_levels

db/migrate/20151007120511_namespaces_projects_path_lower_indexes.rb

+class NamespacesProjectsPathLowerIndexes < ActiveRecord::Migration
+  disable_ddl_transaction!
+
+  def up
+    return unless Gitlab::Database.postgresql?
+
+    execute 'CREATE INDEX CONCURRENTLY index_on_namespaces_lower_path ON namespaces (LOWER(path));'
+    execute 'CREATE INDEX CONCURRENTLY index_on_projects_lower_path ON projects (LOWER(path));'
+  end
+
+  def down
+    return unless Gitlab::Database.postgresql?
+
+    remove_index :namespaces, name: :index_on_namespaces_lower_path
+    remove_index :projects, name: :index_on_projects_lower_path
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20151005162154) do
+ActiveRecord::Schema.define(version: 20151007120511) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

lib/gitlab/database.rb

+module Gitlab
+  module Database
+    def self.mysql?
+      ActiveRecord::Base.connection.adapter_name.downcase == 'mysql'
+    end
+
+    def self.postgresql?
+      ActiveRecord::Base.connection.adapter_name.downcase == 'postgresql'
+    end
+  end
+end

lib/tasks/gitlab/setup.rake

@@ -16,6 +16,7 @@ namespace :gitlab do
 
     Rake::Task["db:setup"].invoke
     Rake::Task["add_limits_mysql"].invoke
+    Rake::Task["setup_postgresql"].invoke
     Rake::Task["db:seed_fu"].invoke
   rescue Gitlab::TaskAbortedByUserError
     puts "Quitting...".red

lib/tasks/migrate/setup_postgresql.rake

+require Rails.root.join('db/migrate/20151007120511_namespaces_projects_path_lower_indexes')
+
+desc 'GitLab | Sets up PostgreSQL'
+task setup_postgresql: :environment do
+  NamespacesProjectsPathLowerIndexes.new.up
+end

spec/benchmarks/models/project_spec.rb

@@ -30,4 +30,21 @@ describe Project, benchmark: true do
       it { is_expected.to iterate_per_second(iterations) }
     end
   end
+
+  describe '.find_with_namespace' do
+    let(:group)   { create(:group, name: 'sisinmaru') }
+    let(:project) { create(:project, name: 'maru', namespace: group) }
+
+    describe 'using a capitalized namespace' do
+      benchmark_subject { described_class.find_with_namespace('sisinmaru/MARU') }
+
+      it { is_expected.to iterate_per_second(600) }
+    end
+
+    describe 'using a lowercased namespace' do
+      benchmark_subject { described_class.find_with_namespace('sisinmaru/maru') }
+
+      it { is_expected.to iterate_per_second(600) }
+    end
+  end
 end

spec/lib/gitlab/database_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Database do
+  # These are just simple smoke tests to check if the methods work (regardless
+  # of what they may return).
+  describe '.mysql?' do
+    subject { described_class.mysql? }
+
+    it { is_expected.to satisfy { |val| val == true || val == false } }
+  end
+
+  describe '.postgresql?' do
+    subject { described_class.postgresql? }
+
+    it { is_expected.to satisfy { |val| val == true || val == false } }
+  end
+end

spec/models/concerns/case_sensitivity_spec.rb

+require 'spec_helper'
+
+describe CaseSensitivity do
+  describe '.iwhere' do
+    let(:connection) { ActiveRecord::Base.connection }
+    let(:model)      { Class.new { include CaseSensitivity } }
+
+    describe 'using PostgreSQL' do
+      before do
+        allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+        allow(Gitlab::Database).to receive(:mysql?).and_return(false)
+      end
+
+      describe 'with a single column/value pair' do
+        it 'returns the criteria for a column and a value' do
+          criteria = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:foo).
+            and_return('"foo"')
+
+          expect(model).to receive(:where).
+            with(%q{LOWER("foo") = LOWER(:value)}, value: 'bar').
+            and_return(criteria)
+
+          expect(model.iwhere(foo: 'bar')).to eq(criteria)
+        end
+
+        it 'returns the criteria for a column with a table, and a value' do
+          criteria = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:'foo.bar').
+            and_return('"foo"."bar"')
+
+          expect(model).to receive(:where).
+            with(%q{LOWER("foo"."bar") = LOWER(:value)}, value: 'bar').
+            and_return(criteria)
+
+          expect(model.iwhere(:'foo.bar' => 'bar')).to eq(criteria)
+        end
+      end
+
+      describe 'with multiple column/value pairs' do
+        it 'returns the criteria for a column and a value' do
+          initial = double(:criteria)
+          final   = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:foo).
+            and_return('"foo"')
+
+          expect(connection).to receive(:quote_table_name).
+            with(:bar).
+            and_return('"bar"')
+
+          expect(model).to receive(:where).
+            with(%q{LOWER("foo") = LOWER(:value)}, value: 'bar').
+            and_return(initial)
+
+          expect(initial).to receive(:where).
+            with(%q{LOWER("bar") = LOWER(:value)}, value: 'baz').
+            and_return(final)
+
+          got = model.iwhere(foo: 'bar', bar: 'baz')
+
+          expect(got).to eq(final)
+        end
+
+        it 'returns the criteria for a column with a table, and a value' do
+          initial = double(:criteria)
+          final   = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:'foo.bar').
+            and_return('"foo"."bar"')
+
+          expect(connection).to receive(:quote_table_name).
+            with(:'foo.baz').
+            and_return('"foo"."baz"')
+
+          expect(model).to receive(:where).
+            with(%q{LOWER("foo"."bar") = LOWER(:value)}, value: 'bar').
+            and_return(initial)
+
+          expect(initial).to receive(:where).
+            with(%q{LOWER("foo"."baz") = LOWER(:value)}, value: 'baz').
+            and_return(final)
+
+          got = model.iwhere(:'foo.bar' => 'bar',
+                             :'foo.baz' => 'baz')
+
+          expect(got).to eq(final)
+        end
+      end
+    end
+
+    describe 'using MySQL' do
+      before do
+        allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
+        allow(Gitlab::Database).to receive(:mysql?).and_return(true)
+      end
+
+      describe 'with a single column/value pair' do
+        it 'returns the criteria for a column and a value' do
+          criteria = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:foo).
+            and_return('`foo`')
+
+          expect(model).to receive(:where).
+            with(%q{`foo` = :value}, value: 'bar').
+            and_return(criteria)
+
+          expect(model.iwhere(foo: 'bar')).to eq(criteria)
+        end
+
+        it 'returns the criteria for a column with a table, and a value' do
+          criteria = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:'foo.bar').
+            and_return('`foo`.`bar`')
+
+          expect(model).to receive(:where).
+            with(%q{`foo`.`bar` = :value}, value: 'bar').
+            and_return(criteria)
+
+          expect(model.iwhere(:'foo.bar' => 'bar')).
+            to eq(criteria)
+        end
+      end
+
+      describe 'with multiple column/value pairs' do
+        it 'returns the criteria for a column and a value' do
+          initial = double(:criteria)
+          final   = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:foo).
+            and_return('`foo`')
+
+          expect(connection).to receive(:quote_table_name).
+            with(:bar).
+            and_return('`bar`')
+
+          expect(model).to receive(:where).
+            with(%q{`foo` = :value}, value: 'bar').
+            and_return(initial)
+
+          expect(initial).to receive(:where).
+            with(%q{`bar` = :value}, value: 'baz').
+            and_return(final)
+
+          got = model.iwhere(foo: 'bar', bar: 'baz')
+
+          expect(got).to eq(final)
+        end
+
+        it 'returns the criteria for a column with a table, and a value' do
+          initial = double(:criteria)
+          final   = double(:criteria)
+
+          expect(connection).to receive(:quote_table_name).
+            with(:'foo.bar').
+            and_return('`foo`.`bar`')
+
+          expect(connection).to receive(:quote_table_name).
+            with(:'foo.baz').
+            and_return('`foo`.`baz`')
+
+          expect(model).to receive(:where).
+            with(%q{`foo`.`bar` = :value}, value: 'bar').
+            and_return(initial)
+
+          expect(initial).to receive(:where).
+            with(%q{`foo`.`baz` = :value}, value: 'baz').
+            and_return(final)
+
+          got = model.iwhere(:'foo.bar' => 'bar',
+                             :'foo.baz' => 'baz')
+
+          expect(got).to eq(final)
+        end
+      end
+    end
+  end
+end