Merge branch 'ce-to-ee-2018-12-04' into 'master'

CE upstream - 2018-12-04 10:23 UTC

See merge request gitlab-org/gitlab-ee!8690

app/assets/javascripts/notes/components/noteable_discussion.vue

@@ -75,12 +75,14 @@ export default {
     },
   },
   data() {
+    const { diff_discussion: isDiffDiscussion, resolved } = this.discussion;
+
     return {
       isReplying: false,
       isResolving: false,
       isUnresolving: false,
       resolveAsThread: true,
-      isRepliesToggledByUser: false,
+      isRepliesCollapsed: Boolean(!isDiffDiscussion && resolved),
     };
   },
   computed: {
@@ -160,15 +162,6 @@ export default {
 
       return expanded || this.alwaysExpanded || isResolvedNonDiffDiscussion;
     },
-    isRepliesCollapsed() {
-      const { discussion, isRepliesToggledByUser } = this;
-      const { resolved, notes } = discussion;
-      const hasReplies = notes.length > 1;
-
-      return (
-        (!discussion.diff_discussion && resolved && hasReplies && !isRepliesToggledByUser) || false
-      );
-    },
     actionText() {
       const commitId = this.discussion.commit_id ? truncateSha(this.discussion.commit_id) : '';
       const linkStart = `<a href="${_.escape(this.discussion.discussion_path)}">`;
@@ -244,7 +237,7 @@ export default {
       this.toggleDiscussion({ discussionId: this.discussion.id });
     },
     toggleReplies() {
-      this.isRepliesToggledByUser = !this.isRepliesToggledByUser;
+      this.isRepliesCollapsed = !this.isRepliesCollapsed;
     },
     showReplyForm() {
       this.isReplying = true;

app/controllers/concerns/uploads_actions.rb

 # frozen_string_literal: true
 
 module UploadsActions
-  extend ActiveSupport::Concern
-
   include Gitlab::Utils::StrongMemoize
   include SendFileUpload
 
   UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
 
-  included do
-    prepend_before_action :set_html_format, only: :show
-  end
-
   def create
     link_to_file = UploadService.new(model, params[:file], uploader_class).execute
 
@@ -61,13 +55,6 @@ module UploadsActions
 
   private
 
-  # Explicitly set the format.
-  # Otherwise rails 5 will set it from a file extension.
-  # See https://github.com/rails/rails/commit/84e8accd6fb83031e4c27e44925d7596655285f7#diff-2b8f2fbb113b55ca8e16001c393da8f1
-  def set_html_format
-    request.format = :html
-  end
-
   def uploader_class
     raise NotImplementedError
   end

app/controllers/projects/artifacts_controller.rb

@@ -9,7 +9,6 @@ class Projects::ArtifactsController < Projects::ApplicationController
   before_action :authorize_read_build!
   before_action :authorize_update_build!, only: [:keep]
   before_action :extract_ref_name_and_path
-  before_action :set_request_format, only: [:file]
   before_action :validate_artifacts!, except: [:download]
   before_action :entry, only: [:file]
 
@@ -110,12 +109,4 @@ class Projects::ArtifactsController < Projects::ApplicationController
 
     render_404 unless @entry.exists?
   end
-
-  def set_request_format
-    request.format = :html if set_request_format?
-  end
-
-  def set_request_format?
-    request.format != :json
-  end
 end

app/controllers/projects/blob_controller.rb

@@ -9,7 +9,6 @@ class Projects::BlobController < Projects::ApplicationController
   include ActionView::Helpers::SanitizeHelper
   prepend_before_action :authenticate_user!, only: [:edit]
 
-  before_action :set_request_format, only: [:edit, :show, :update, :destroy]
   before_action :require_non_empty_project, except: [:new, :create]
   before_action :authorize_download_code!
 
@@ -242,18 +241,6 @@ class Projects::BlobController < Projects::ApplicationController
       .last_for_path(@repository, @ref, @path).sha
   end
 
-  # In Rails 4.2 if params[:format] is empty, Rails set it to :html
-  # But since Rails 5.0 the framework now looks for an extension.
-  # E.g. for `blob/master/CHANGELOG.md` in Rails 4 the format would be `:html`, but in Rails 5 on it'd be `:md`
-  # This before_action explicitly sets the `:html` format for all requests unless `:format` is set by a client e.g. by JS for XHR requests.
-  def set_request_format
-    request.format = :html if set_request_format?
-  end
-
-  def set_request_format?
-    params[:id].present? && params[:format].blank? && request.format != "json"
-  end
-
   def show_html
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last

app/controllers/projects/commits_controller.rb

@@ -12,7 +12,6 @@ class Projects::CommitsController < Projects::ApplicationController
   before_action :assign_ref_vars, except: :commits_root
   before_action :authorize_download_code!
   before_action :set_commits, except: :commits_root
-  before_action :set_request_format, only: :show
 
   def commits_root
     redirect_to project_commits_path(@project, @project.default_branch)
@@ -71,19 +70,6 @@ class Projects::CommitsController < Projects::ApplicationController
     @commits = set_commits_for_rendering(@commits)
   end
 
-  # Rails 5 sets request.format from the extension.
-  # Explicitly set to :html.
-  def set_request_format
-    request.format = :html if set_request_format?
-  end
-
-  # Rails 5 sets request.format from extension.
-  # In this case if the ref ends with `.atom`, it's expected to be the html response,
-  # not the atom one. So explicitly set request.format as :html to act like rails4.
-  def set_request_format?
-    request.format.to_s == "text/html" || @commits.ref.ends_with?("atom")
-  end
-
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42330')
   end

changelogs/unreleased/ab-approximate-counts.yml

+---
+title: Approximate counting strategy with TABLESAMPLE.
+merge_request: 22650
+author:
+type: performance

changelogs/unreleased/auto-devops-support-for-group-security-dashboard.yml

----
-title: Auto DevOps support for Group Security Dashboard
-merge_request: 23165
-author:
-type: fixed

changelogs/unreleased/document-raw-snippet-api.yml

+---
+title: Fix lack of documentation on how to fetch a snippet's content using API
+merge_request: 23448
+author: Colin Leroy
+type: other

changelogs/unreleased/winh-collapse-discussions.yml

+---
+title: Fix collapsing discussion replies
+merge_request: 23462
+author:
+type: fixed

config/initializers/action_dispatch_http_mime_negotiation.rb

+# Starting with Rails 5, Rails tries to determine the request format based on
+# the extension of the full URL path if no explicit `format` param or `Accept`
+# header is provided, like when simply browsing to a page in your browser.
+#
+# This is undesireable in GitLab, because many of our paths will end in a ref or
+# blob name that can end with any extension, while these pages should still be
+# presented as HTML unless otherwise specified.
+
+# We override `format_from_path_extension` to disable this behavior.
+
+module ActionDispatch
+  module Http
+    module MimeNegotiation
+      def format_from_path_extension
+        nil
+      end
+    end
+  end
+end

config/routes/wiki.rb

@@ -6,7 +6,7 @@ scope(controller: :wikis) do
     post '/', to: 'wikis#create'
   end
 
-  scope(path: 'wikis/*id', as: :wiki, format: false, defaults: { format: :html }) do
+  scope(path: 'wikis/*id', as: :wiki, format: false) do
     get :edit
     get :history
     post :preview_markdown

doc/api/snippets.md

@@ -37,13 +37,13 @@ Parameters:
 | ---------          | ----    | -------- | -----------                   |
 | `id`               | Integer | yes      | The ID of a snippet           |
 
-``` bash
+```bash
 curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/snippets/1
 ```
 
 Example response:
 
-``` json
+```json
 {
   "id": 1,
   "title": "test",
@@ -65,6 +65,30 @@ Example response:
 }
 ```
 
+## Single snippet contents
+
+Get a single snippet's raw contents.
+
+```
+GET /snippets/:id/raw
+```
+
+Parameters:
+
+| Attribute          | Type    | Required | Description                   |
+| ---------          | ----    | -------- | -----------                   |
+| `id`               | Integer | yes      | The ID of a snippet           |
+
+```bash
+curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/snippets/1/raw
+```
+
+Example response:
+
+```
+Hello World snippet
+```
+
 ## Create new snippet
 
 Creates a new snippet. The user must have permission to create new snippets.
@@ -84,7 +108,7 @@ Parameters:
 | `visibility`       | String  | no       | The snippet's visibility     |
 
 
-``` bash
+```bash
 curl --request POST \
      --data '{"title": "This is a snippet", "content": "Hello world", "description": "Hello World snippet", "file_name": "test.txt", "visibility": "internal" }' \
      --header 'Content-Type: application/json' \
@@ -94,7 +118,7 @@ curl --request POST \
 
 Example response:
 
-``` json
+```json
 {
   "id": 1,
   "title": "This is a snippet",
@@ -136,7 +160,7 @@ Parameters:
 | `visibility`       | String  | no       | The snippet's visibility     |
 
 
-``` bash
+```bash
 curl --request PUT \
      --data '{"title": "foo", "content": "bar"}' \
      --header 'Content-Type: application/json' \
@@ -146,7 +170,7 @@ curl --request PUT \
 
 Example response:
 
-``` json
+```json
 {
   "id": 1,
   "title": "test",
@@ -201,13 +225,13 @@ GET /snippets/public
 | `per_page` | Integer | no       | number of snippets to return per page |
 | `page`     | Integer | no       | the page to retrieve                  |
 
-``` bash
+```bash
 curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/snippets/public?per_page=2&page=1
 ```
 
 Example response:
 
-``` json
+```json
 [
     {
         "author": {

doc/development/gotchas.md

@@ -92,7 +92,7 @@ describe API::Labels do
 end
 ```
 
-## Avoid using `allow_any_instance_of` in RSpec
+## Avoid using `expect_any_instance_of` or `allow_any_instance_of` in RSpec
 
 ### Why
 
@@ -102,7 +102,7 @@ end
   error like this:
 
     ```
-    1.1) Failure/Error: allow_any_instance_of(ApplicationSetting).to receive_messages(messages)
+    1.1) Failure/Error: expect_any_instance_of(ApplicationSetting).to receive_messages(messages)
          Using `any_instance` to stub a method (elasticsearch_indexing) that has been defined on a prepended module (EE::ApplicationSetting) is not supported.
     ```
 
@@ -112,7 +112,7 @@ Instead of writing:
 
 ```ruby
 # Don't do this:
-allow_any_instance_of(Project).to receive(:add_import_job)
+expect_any_instance_of(Project).to receive(:add_import_job)
 ```
 
 We could write:

doc/topics/autodevops/index.md

@@ -666,8 +666,6 @@ also be customized, and you can easily use a [custom buildpack](#custom-buildpac
 | `REVIEW_DISABLED`            | From GitLab 11.0, this variable can be used to disable the `review` and the manual `review:stop` job. If the variable is present, these jobs will not be created. |
 | `DAST_DISABLED`              | From GitLab 11.0, this variable can be used to disable the `dast` job. If the variable is present, the job will not be created. |
 | `PERFORMANCE_DISABLED`       | From GitLab 11.0, this variable can be used to disable the `performance` job. If the variable is present, the job will not be created. |
-| `OLD_REPORTS_DISABLED`       | From GitLab 11.5, this variable can be used to disable the `sast` job. If the variable is present, the job will not be created. |
-| `NEW_REPORTS_DISABLED`       | From GitLab 11.5, this variable can be used to disable the `sast_dashboard` job. If the variable is present, the job will not be created. |
 
 TIP: **Tip:**
 Set up the replica variables using a

lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml

@@ -19,15 +19,6 @@
 # * review: REVIEW_DISABLED
 # * stop_review: REVIEW_DISABLED
 #
-# The sast and sast_dashboard jobs are executed to guarantee full compatibility
-# with the group security dashboard and the security reports with old runners.
-# If you use only runners with version 11.5 or above, you can disable the sast
-# job by setting the OLD_REPORTS_DISABLED environment variable. If you use only
-# runners with version below 11.5, you can disable the sast_dashboard job by
-# setting the NEW_REPORTS_DISABLED environment variable.
-# The sast_dashboard job will be removed in the future, when the sast job will
-# use the new reports syntax.
-#
 # In order to deploy, you must have a Kubernetes cluster configured either
 # via a project integration, or via group/project variables.
 # AUTO_DEVOPS_DOMAIN must also be set as a variable at the group or project
@@ -182,29 +173,6 @@ sast:
   except:
     variables:
       - $SAST_DISABLED
-      - $OLD_REPORTS_DISABLED
-
-sast_dashboard:
-  stage: test
-  image: docker:stable
-  allow_failure: true
-  services:
-    - docker:stable-dind
-  script:
-    - setup_docker
-    - sast
-  artifacts:
-    reports:
-      sast: gl-sast-report.json
-  only:
-    refs:
-      - branches
-    variables:
-      - $GITLAB_FEATURES =~ /\bsast\b/
-  except:
-    variables:
-      - $SAST_DISABLED
-      - $NEW_REPORTS_DISABLED
 
 dependency_scanning:
   stage: test

lib/gitlab/database/count.rb

 # frozen_string_literal: true
 
 # For large tables, PostgreSQL can take a long time to count rows due to MVCC.
-# We can optimize this by using the reltuples count as described in https://wiki.postgresql.org/wiki/Slow_Counting.
+# We can optimize this by using various strategies for approximate counting.
+#
+# For example, we can use the reltuples count as described in https://wiki.postgresql.org/wiki/Slow_Counting.
+#
+# However, since statistics are not always up to date, we also implement a table sampling strategy
+# that performs an exact count but only on a sample of the table. See TablesampleCountStrategy.
 module Gitlab
   module Database
     module Count
@@ -20,68 +25,30 @@ module Gitlab
         end
 
       # Takes in an array of models and returns a Hash for the approximate
-      # counts for them.  If the model's table has not been vacuumed or
-      # analyzed recently, simply run the Model.count to get the data.
+      # counts for them.
+      #
+      # Various count strategies can be specified that are executed in
+      # sequence until all tables have an approximate count attached
+      # or we run out of strategies.
+      #
+      # Note that not all strategies are available on all supported RDBMS.
       #
       # @param [Array]
       # @return [Hash] of Model -> count mapping
-      def self.approximate_counts(models)
-        table_to_model_map = models.each_with_object({}) do |model, hash|
-          hash[model.table_name] = model
-        end
-
-        table_names = table_to_model_map.keys
-        counts_by_table_name = Gitlab::Database.postgresql? ? reltuples_from_recently_updated(table_names) : {}
+      def self.approximate_counts(models, strategies: [TablesampleCountStrategy, ReltuplesCountStrategy, ExactCountStrategy])
+        strategies.each_with_object({}) do |strategy, counts_by_model|
+          if strategy.enabled?
+            models_with_missing_counts = models - counts_by_model.keys
 
-        # Convert table -> count to Model -> count
-        counts_by_model = counts_by_table_name.each_with_object({}) do |pair, hash|
-          model = table_to_model_map[pair.first]
-          hash[model] = pair.second
-        end
+            break if models_with_missing_counts.empty?
 
-        missing_tables = table_names - counts_by_table_name.keys
+            counts = strategy.new(models_with_missing_counts).count
 
-        missing_tables.each do |table|
-          model = table_to_model_map[table]
-          counts_by_model[model] = model.count
+            counts.each do |model, count|
+              counts_by_model[model] = count
+            end
+          end
         end
-
-        counts_by_model
-      end
-
-      # Returns a hash of the table names that have recently updated tuples.
-      #
-      # @param [Array] table names
-      # @returns [Hash] Table name to count mapping (e.g. { 'projects' => 5, 'users' => 100 })
-      def self.reltuples_from_recently_updated(table_names)
-        query = postgresql_estimate_query(table_names)
-        rows = []
-
-        # Querying tuple stats only works on the primary. Due to load
-        # balancing, we need to ensure this query hits the load balancer.  The
-        # easiest way to do this is to start a transaction.
-        ActiveRecord::Base.transaction do
-          rows = ActiveRecord::Base.connection.select_all(query)
-        end
-
-        rows.each_with_object({}) { |row, data| data[row['table_name']] = row['estimate'].to_i }
-      rescue *CONNECTION_ERRORS
-        {}
-      end
-
-      # Generates the PostgreSQL query to return the tuples for tables
-      # that have been vacuumed or analyzed in the last hour.
-      #
-      # @param [Array] table names
-      # @returns [Hash] Table name to count mapping (e.g. { 'projects' => 5, 'users' => 100 })
-      def self.postgresql_estimate_query(table_names)
-        time = "to_timestamp(#{1.hour.ago.to_i})"
-        <<~SQL
-          SELECT pg_class.relname AS table_name, reltuples::bigint AS estimate FROM pg_class
-          LEFT JOIN pg_stat_user_tables ON pg_class.relname = pg_stat_user_tables.relname
-          WHERE pg_class.relname IN (#{table_names.map { |table| "'#{table}'" }.join(',')})
-          AND (last_vacuum > #{time} OR last_autovacuum > #{time} OR last_analyze > #{time} OR last_autoanalyze > #{time})
-        SQL
       end
     end
   end

lib/gitlab/database/count/exact_count_strategy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module Count
+      # This strategy performs an exact count on the model.
+      #
+      # This is guaranteed to be accurate, however it also scans the
+      # whole table. Hence, there are no guarantees with respect
+      # to runtime.
+      #
+      # Note that for very large tables, this may even timeout.
+      class ExactCountStrategy
+        attr_reader :models
+        def initialize(models)
+          @models = models
+        end
+
+        def count
+          models.each_with_object({}) do |model, data|
+            data[model] = model.count
+          end
+        end
+
+        def self.enabled?
+          true
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/count/reltuples_count_strategy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module Count
+      class PgClass < ActiveRecord::Base
+        self.table_name = 'pg_class'
+      end
+
+      # This strategy counts based on PostgreSQL's statistics in pg_stat_user_tables.
+      #
+      # Specifically, it relies on the column reltuples in said table. An additional
+      # check is performed to make sure statistics were updated within the last hour.
+      #
+      # Otherwise, this strategy skips tables with outdated statistics.
+      #
+      # There are no guarantees with respect to the accuracy of this strategy. Runtime
+      # however is guaranteed to be "fast", because it only looks up statistics.
+      class ReltuplesCountStrategy
+        attr_reader :models
+        def initialize(models)
+          @models = models
+        end
+
+        # Returns a hash of the table names that have recently updated tuples.
+        #
+        # @returns [Hash] Table name to count mapping (e.g. { 'projects' => 5, 'users' => 100 })
+        def count
+          size_estimates
+        rescue *CONNECTION_ERRORS
+          {}
+        end
+
+        def self.enabled?
+          Gitlab::Database.postgresql?
+        end
+
+        private
+
+        def table_names
+          models.map(&:table_name)
+        end
+
+        def size_estimates(check_statistics: true)
+          table_to_model = models.each_with_object({}) { |model, h| h[model.table_name] = model }
+
+          # Querying tuple stats only works on the primary. Due to load balancing, the
+          # easiest way to do this is to start a transaction.
+          ActiveRecord::Base.transaction do
+            get_statistics(table_names, check_statistics: check_statistics).each_with_object({}) do |row, data|
+              model = table_to_model[row.table_name]
+              data[model] = row.estimate
+            end
+          end
+        end
+
+        # Generates the PostgreSQL query to return the tuples for tables
+        # that have been vacuumed or analyzed in the last hour.
+        #
+        # @param [Array] table names
+        # @returns [Hash] Table name to count mapping (e.g. { 'projects' => 5, 'users' => 100 })
+        def get_statistics(table_names, check_statistics: true)
+          time = 1.hour.ago
+
+          query = PgClass.joins("LEFT JOIN pg_stat_user_tables USING (relname)")
+            .where(relname: table_names)
+            .select('pg_class.relname AS table_name, reltuples::bigint AS estimate')
+
+          if check_statistics
+            query = query.where('last_vacuum > ? OR last_autovacuum > ? OR last_analyze > ? OR last_autoanalyze > ?',
+                                time, time, time, time)
+          end
+
+          query
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/count/tablesample_count_strategy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module Count
+      # A tablesample count executes in two phases:
+      # * Estimate table sizes based on reltuples.
+      # * Based on the estimate:
+      #   * If the table is considered 'small', execute an exact relation count.
+      #   * Otherwise, count on a sample of the table using TABLESAMPLE.
+      #
+      # The size of the sample is chosen in a way that we always roughly scan
+      # the same amount of rows (see TABLESAMPLE_ROW_TARGET).
+      #
+      # There are no guarantees with respect to the accuracy of the result or runtime.
+      class TablesampleCountStrategy < ReltuplesCountStrategy
+        EXACT_COUNT_THRESHOLD = 10_000
+        TABLESAMPLE_ROW_TARGET = 10_000
+
+        def count
+          estimates = size_estimates(check_statistics: false)
+
+          models.each_with_object({}) do |model, count_by_model|
+            count = perform_count(model, estimates[model])
+            count_by_model[model] = count if count
+          end
+        rescue *CONNECTION_ERRORS
+          {}
+        end
+
+        def self.enabled?
+          Gitlab::Database.postgresql? && Feature.enabled?(:tablesample_counts)
+        end
+
+        private
+
+        def perform_count(model, estimate)
+          # If we estimate 0, we may not have statistics at all. Don't use them.
+          return nil unless estimate && estimate > 0
+
+          if estimate < EXACT_COUNT_THRESHOLD
+            # The table is considered small, the assumption here is that
+            # the exact count will be fast anyways.
+            model.count
+          else
+            # The table is considered large, let's only count on a sample.
+            tablesample_count(model, estimate)
+          end
+        end
+
+        def tablesample_count(model, estimate)
+          portion = (TABLESAMPLE_ROW_TARGET.to_f / estimate).round(4)
+          inverse = 1 / portion
+          query = <<~SQL
+            SELECT (COUNT(*)*#{inverse})::integer AS count
+            FROM #{model.table_name} TABLESAMPLE SYSTEM (#{portion * 100})
+          SQL
+
+          rows = ActiveRecord::Base.connection.select_all(query)
+
+          Integer(rows.first['count'])
+        end
+      end
+    end
+  end
+end

spec/controllers/projects/blob_controller_spec.rb

@@ -35,6 +35,11 @@ describe Projects::BlobController do
         let(:id) { 'binary-encoding/encoding/binary-1.bin' }
         it { is_expected.to respond_with(:success) }
       end
+
+      context "Markdown file" do
+        let(:id) { 'master/README.md' }
+        it { is_expected.to respond_with(:success) }
+      end
     end
 
     context 'with file path and JSON format' do

spec/javascripts/notes/components/noteable_discussion_spec.js

@@ -6,7 +6,6 @@ import { noteableDataMock, discussionMock, notesDataMock } from '../mock_data';
 import mockDiffFile from '../../diffs/mock_data/diff_file';
 
 const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json';
-const diffDiscussionFixture = 'merge_requests/diff_discussion.json';
 
 describe('noteable_discussion component', () => {
   const Component = Vue.extend(noteableDiscussion);
@@ -79,51 +78,6 @@ describe('noteable_discussion component', () => {
     });
   });
 
-  describe('computed', () => {
-    describe('isRepliesCollapsed', () => {
-      it('should return false for diff discussions', done => {
-        const diffDiscussion = getJSONFixture(diffDiscussionFixture)[0];
-        vm.$store.dispatch('setInitialNotes', [diffDiscussion]);
-
-        Vue.nextTick()
-          .then(() => {
-            expect(vm.isRepliesCollapsed).toEqual(false);
-            expect(vm.$el.querySelector('.js-toggle-replies')).not.toBeNull();
-            expect(vm.$el.querySelector('.discussion-reply-holder')).not.toBeNull();
-          })
-          .then(done)
-          .catch(done.fail);
-      });
-
-      it('should return false if discussion does not have a reply', () => {
-        const discussion = { ...discussionMock, resolved: true };
-        discussion.notes = discussion.notes.slice(0, 1);
-        const noRepliesVm = new Component({
-          store,
-          propsData: { discussion },
-        }).$mount();
-
-        expect(noRepliesVm.isRepliesCollapsed).toEqual(false);
-        expect(noRepliesVm.$el.querySelector('.js-toggle-replies')).toBeNull();
-        expect(vm.$el.querySelector('.discussion-reply-holder')).not.toBeNull();
-        noRepliesVm.$destroy();
-      });
-
-      it('should return true for resolved non-diff discussion which has replies', () => {
-        const discussion = { ...discussionMock, resolved: true };
-        const resolvedDiscussionVm = new Component({
-          store,
-          propsData: { discussion },
-        }).$mount();
-
-        expect(resolvedDiscussionVm.isRepliesCollapsed).toEqual(true);
-        expect(resolvedDiscussionVm.$el.querySelector('.js-toggle-replies')).not.toBeNull();
-        expect(vm.$el.querySelector('.discussion-reply-holder')).not.toBeNull();
-        resolvedDiscussionVm.$destroy();
-      });
-    });
-  });
-
   describe('methods', () => {
     describe('jumpToNextDiscussion', () => {
       it('expands next unresolved discussion', done => {

spec/lib/gitlab/database/count/exact_count_strategy_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Database::Count::ExactCountStrategy do
+  before do
+    create_list(:project, 3)
+    create(:identity)
+  end
+
+  let(:models) { [Project, Identity] }
+
+  subject { described_class.new(models).count }
+
+  describe '#count' do
+    it 'counts all models' do
+      expect(models).to all(receive(:count).and_call_original)
+
+      expect(subject).to eq({ Project => 3, Identity => 1 })
+    end
+  end
+
+  describe '.enabled?' do
+    it 'is enabled for PostgreSQL' do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+
+      expect(described_class.enabled?).to be_truthy
+    end
+
+    it 'is enabled for MySQL' do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
+
+      expect(described_class.enabled?).to be_truthy
+    end
+  end
+end

spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Database::Count::ReltuplesCountStrategy do
+  before do
+    create_list(:project, 3)
+    create(:identity)
+  end
+
+  let(:models) { [Project, Identity] }
+  subject { described_class.new(models).count }
+
+  describe '#count', :postgresql do
+    context 'when reltuples is up to date' do
+      before do
+        ActiveRecord::Base.connection.execute('ANALYZE projects')
+        ActiveRecord::Base.connection.execute('ANALYZE identities')
+      end
+
+      it 'uses statistics to do the count' do
+        models.each { |model| expect(model).not_to receive(:count) }
+
+        expect(subject).to eq({ Project => 3, Identity => 1 })
+      end
+    end
+
+    context 'insufficient permissions' do
+      it 'returns an empty hash' do
+        allow(ActiveRecord::Base).to receive(:transaction).and_raise(PG::InsufficientPrivilege)
+
+        expect(subject).to eq({})
+      end
+    end
+  end
+
+  describe '.enabled?' do
+    it 'is enabled for PostgreSQL' do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+
+      expect(described_class.enabled?).to be_truthy
+    end
+
+    it 'is disabled for MySQL' do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
+
+      expect(described_class.enabled?).to be_falsey
+    end
+  end
+end

spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Database::Count::TablesampleCountStrategy do
+  before do
+    create_list(:project, 3)
+    create(:identity)
+  end
+
+  let(:models) { [Project, Identity] }
+  let(:strategy) { described_class.new(models) }
+
+  subject { strategy.count }
+
+  describe '#count', :postgresql do
+    let(:estimates) { { Project => threshold + 1, Identity => threshold - 1 } }
+    let(:threshold) { Gitlab::Database::Count::TablesampleCountStrategy::EXACT_COUNT_THRESHOLD }
+
+    before do
+      allow(strategy).to receive(:size_estimates).with(check_statistics: false).and_return(estimates)
+    end
+
+    context 'for tables with an estimated small size' do
+      it 'performs an exact count' do
+        expect(Identity).to receive(:count).and_call_original
+
+        expect(subject).to include({ Identity => 1 })
+      end
+    end
+
+    context 'for tables with an estimated large size' do
+      it 'performs a tablesample count' do
+        expect(Project).not_to receive(:count)
+
+        result = subject
+        expect(result[Project]).to eq(3)
+      end
+    end
+
+    context 'insufficient permissions' do
+      it 'returns an empty hash' do
+        allow(strategy).to receive(:size_estimates).and_raise(PG::InsufficientPrivilege)
+
+        expect(subject).to eq({})
+      end
+    end
+  end
+
+  describe '.enabled?' do
+    before do
+      stub_feature_flags(tablesample_counts: true)
+    end
+
+    it 'is enabled for PostgreSQL' do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+
+      expect(described_class.enabled?).to be_truthy
+    end
+
+    it 'is disabled for MySQL' do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
+
+      expect(described_class.enabled?).to be_falsey
+    end
+  end
+end

spec/lib/gitlab/database/count_spec.rb

@@ -8,63 +8,51 @@ describe Gitlab::Database::Count do
 
   let(:models) { [Project, Identity] }
 
-  describe '.approximate_counts' do
-    context 'with MySQL' do
-      context 'when reltuples have not been updated' do
-        it 'counts all models the normal way' do
-          expect(Gitlab::Database).to receive(:postgresql?).and_return(false)
+  context '.approximate_counts' do
+    context 'selecting strategies' do
+      let(:strategies) { [double('s1', enabled?: true), double('s2', enabled?: false)] }
 
-          expect(Project).to receive(:count).and_call_original
-          expect(Identity).to receive(:count).and_call_original
+      it 'uses only enabled strategies' do
+        expect(strategies[0]).to receive(:new).and_return(double('strategy1', count: {}))
+        expect(strategies[1]).not_to receive(:new)
 
-          expect(described_class.approximate_counts(models)).to eq({ Project => 3, Identity => 1 })
-        end
+        described_class.approximate_counts(models, strategies: strategies)
       end
     end
 
-    context 'with PostgreSQL', :postgresql do
-      describe 'when reltuples have not been updated' do
-        it 'counts all models the normal way' do
-          expect(described_class).to receive(:reltuples_from_recently_updated).with(%w(projects identities)).and_return({})
+    context 'fallbacks' do
+      subject { described_class.approximate_counts(models, strategies: strategies) }
 
-          expect(Project).to receive(:count).and_call_original
-          expect(Identity).to receive(:count).and_call_original
-          expect(described_class.approximate_counts(models)).to eq({ Project => 3, Identity => 1 })
-        end
+      let(:strategies) do
+        [
+          double('s1', enabled?: true, new: first_strategy),
+          double('s2', enabled?: true, new: second_strategy)
+        ]
       end
 
-      describe 'no permission' do
-        it 'falls back to standard query' do
-          allow(described_class).to receive(:postgresql_estimate_query).and_raise(PG::InsufficientPrivilege)
+      let(:first_strategy) { double('first strategy', count: {}) }
+      let(:second_strategy) { double('second strategy', count: {}) }
 
-          expect(Project).to receive(:count).and_call_original
-          expect(Identity).to receive(:count).and_call_original
-          expect(described_class.approximate_counts(models)).to eq({ Project => 3, Identity => 1 })
-        end
+      it 'gets results from first strategy' do
+        expect(strategies[0]).to receive(:new).with(models).and_return(first_strategy)
+        expect(first_strategy).to receive(:count)
+
+        subject
       end
 
-      describe 'when some reltuples have been updated' do
-        it 'counts projects in the fast way' do
-          expect(described_class).to receive(:reltuples_from_recently_updated).with(%w(projects identities)).and_return({ 'projects' => 3 })
+      it 'gets more results from second strategy if some counts are missing' do
+        expect(first_strategy).to receive(:count).and_return({ Project => 3 })
+        expect(strategies[1]).to receive(:new).with([Identity]).and_return(second_strategy)
+        expect(second_strategy).to receive(:count).and_return({ Identity => 1 })
 
-          expect(Project).not_to receive(:count).and_call_original
-          expect(Identity).to receive(:count).and_call_original
-          expect(described_class.approximate_counts(models)).to eq({ Project => 3, Identity => 1 })
-        end
+        expect(subject).to eq({ Project => 3, Identity => 1 })
       end
 
-      describe 'when all reltuples have been updated' do
-        before do
-          ActiveRecord::Base.connection.execute('ANALYZE projects')
-          ActiveRecord::Base.connection.execute('ANALYZE identities')
-        end
-
-        it 'counts models with the standard way' do
-          expect(Project).not_to receive(:count)
-          expect(Identity).not_to receive(:count)
+      it 'does not get more results as soon as all counts are present' do
+        expect(first_strategy).to receive(:count).and_return({ Project => 3, Identity => 1 })
+        expect(strategies[1]).not_to receive(:new)
 
-          expect(described_class.approximate_counts(models)).to eq({ Project => 3, Identity => 1 })
-        end
+        subject
       end
     end
   end

spec/routing/project_routing_spec.rb

@@ -36,36 +36,33 @@ describe 'project routing' do
   shared_examples 'RESTful project resources' do
     let(:actions) { [:index, :create, :new, :edit, :show, :update, :destroy] }
     let(:controller_path) { controller }
-    let(:id) { { id: '1' } }
-    let(:format) { {} } # response format, e.g. { format: :html }
-    let(:params) { { namespace_id: 'gitlab', project_id: 'gitlabhq' } }
 
     it 'to #index' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#index", params) if actions.include?(:index)
+      expect(get("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#index", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:index)
     end
 
     it 'to #create' do
-      expect(post("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#create", params) if actions.include?(:create)
+      expect(post("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#create", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:create)
     end
 
     it 'to #new' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}/new")).to route_to("projects/#{controller}#new", params) if actions.include?(:new)
+      expect(get("/gitlab/gitlabhq/#{controller_path}/new")).to route_to("projects/#{controller}#new", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:new)
     end
 
     it 'to #edit' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}/1/edit")).to route_to("projects/#{controller}#edit", params.merge(**id, **format)) if actions.include?(:edit)
+      expect(get("/gitlab/gitlabhq/#{controller_path}/1/edit")).to route_to("projects/#{controller}#edit", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:edit)
     end
 
     it 'to #show' do
-      expect(get("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#show", params.merge(**id, **format)) if actions.include?(:show)
+      expect(get("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#show", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:show)
     end
 
     it 'to #update' do
-      expect(put("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#update", params.merge(id)) if actions.include?(:update)
+      expect(put("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#update", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:update)
     end
 
     it 'to #destroy' do
-      expect(delete("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#destroy", params.merge(**id, **format)) if actions.include?(:destroy)
+      expect(delete("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#destroy", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:destroy)
     end
   end
 
@@ -154,13 +151,12 @@ describe 'project routing' do
     end
 
     it 'to #history' do
-      expect(get('/gitlab/gitlabhq/wikis/1/history')).to route_to('projects/wikis#history', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1', format: :html)
+      expect(get('/gitlab/gitlabhq/wikis/1/history')).to route_to('projects/wikis#history', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1')
     end
 
     it_behaves_like 'RESTful project resources' do
       let(:actions)    { [:create, :edit, :show, :destroy] }
       let(:controller) { 'wikis' }
-      let(:format) { { format: :html } }
     end
   end
 

spec/support/features/discussion_comments_shared_example.rb

@@ -142,6 +142,14 @@ shared_examples 'discussion comments' do |resource_name|
           find(comments_selector, match: :first)
         end
 
+        def submit_reply(text)
+          find("#{comments_selector} .js-vue-discussion-reply").click
+          find("#{comments_selector} .note-textarea").send_keys(text)
+
+          click_button "Comment"
+          wait_for_requests
+        end
+
         it 'clicking "Start discussion" will post a discussion' do
           new_comment = all(comments_selector).last
 
@@ -149,16 +157,29 @@ shared_examples 'discussion comments' do |resource_name|
           expect(new_comment).to have_selector '.discussion'
         end
 
+        if resource_name =~ /(issue|merge request)/
+          it 'can be replied to' do
+            submit_reply('some text')
+
+            expect(page).to have_css('.discussion-notes .note', count: 2)
+            expect(page).to have_content 'Collapse replies'
+          end
+
+          it 'can be collapsed' do
+            submit_reply('another text')
+
+            find('.js-collapse-replies').click
+            expect(page).to have_css('.discussion-notes .note', count: 1)
+            expect(page).to have_content '1 reply'
+          end
+        end
+
         if resource_name == 'merge request'
           let(:note_id) { find("#{comments_selector} .note:first-child", match: :first)['data-note-id'] }
           let(:reply_id) { find("#{comments_selector} .note:last-child", match: :first)['data-note-id'] }
 
           it 'shows resolved discussion when toggled' do
-            find("#{comments_selector} .js-vue-discussion-reply").click
-            find("#{comments_selector} .note-textarea").send_keys('a')
-
-            click_button "Comment"
-            wait_for_requests
+            submit_reply('a')
 
             click_button "Resolve discussion"
             wait_for_requests