Merge branch '342881-limit-build-infos-for-single-package-graphql-queries' into 'master'

GraphQL: Limit the amount of ids loaded when using package build infos

See merge request gitlab-org/gitlab!75672

app/finders/packages/build_infos_finder.rb

+# frozen_string_literal: true
+
+module Packages
+  class BuildInfosFinder
+    MAX_PAGE_SIZE = 100
+
+    def initialize(package, params)
+      @package = package
+      @params = params
+    end
+
+    def execute
+      build_infos = @package.build_infos.without_empty_pipelines
+      build_infos = apply_order(build_infos)
+      build_infos = apply_limit(build_infos)
+      apply_cursor(build_infos)
+    end
+
+    private
+
+    def apply_order(build_infos)
+      order_direction = :desc
+      order_direction = :asc if last
+
+      build_infos.order_by_pipeline_id(order_direction)
+    end
+
+    def apply_limit(build_infos)
+      limit = [first, last, max_page_size, MAX_PAGE_SIZE].compact.min
+      limit += 1 if support_next_page
+      build_infos.limit(limit)
+    end
+
+    def apply_cursor(build_infos)
+      if before
+        build_infos.with_pipeline_id_greater_than(before)
+      elsif after
+        build_infos.with_pipeline_id_less_than(after)
+      else
+        build_infos
+      end
+    end
+
+    def first
+      @params[:first]
+    end
+
+    def last
+      @params[:last]
+    end
+
+    def max_page_size
+      @params[:max_page_size]
+    end
+
+    def before
+      @params[:before]
+    end
+
+    def after
+      @params[:after]
+    end
+
+    def support_next_page
+      @params[:support_next_page]
+    end
+  end
+end

app/graphql/resolvers/package_pipelines_resolver.rb

+# frozen_string_literal: true
+
+module Resolvers
+  class PackagePipelinesResolver < BaseResolver
+    include Gitlab::Graphql::Authorize::AuthorizeResource
+
+    type Types::Ci::PipelineType.connection_type, null: true
+    extension Gitlab::Graphql::Extensions::ExternallyPaginatedArrayExtension
+
+    authorizes_object!
+    authorize :read_pipeline
+
+    alias_method :package, :object
+
+    def resolve(first: nil, last: nil, after: nil, before: nil, lookahead:)
+      finder = ::Packages::BuildInfosFinder.new(
+        package,
+        first: first,
+        last: last,
+        after: decode_cursor(after),
+        before: decode_cursor(before),
+        max_page_size: context.schema.default_max_page_size,
+        support_next_page: lookahead.selects?(:page_info)
+      )
+
+      build_infos = finder.execute
+
+      # this .pluck_pipeline_ids can load max 101 pipelines ids
+      ::Ci::Pipeline.id_in(build_infos.pluck_pipeline_ids)
+    end
+
+    # we manage the pagination manually, so opt out of the connection field extension
+    def self.field_options
+      super.merge(
+        connection: false,
+        extras: [:lookahead]
+      )
+    end
+
+    private
+
+    def decode_cursor(encoded)
+      return unless encoded
+
+      decoded = Gitlab::Json.parse(context.schema.cursor_encoder.decode(encoded, nonce: true))
+      id_from_cursor(decoded)
+    rescue JSON::ParserError
+      raise Gitlab::Graphql::Errors::ArgumentError, "Please provide a valid cursor"
+    end
+
+    def id_from_cursor(cursor)
+      cursor&.fetch('id')
+    rescue KeyError
+      raise Gitlab::Graphql::Errors::ArgumentError, "Please provide a valid cursor"
+    end
+  end
+end

app/graphql/types/packages/package_details_type.rb

@@ -14,6 +14,13 @@ module Types
 
       field :dependency_links, Types::Packages::PackageDependencyLinkType.connection_type, null: true, description: 'Dependency link.'
 
+      # this is an override of Types::Packages::PackageType.pipelines
+      # in order to use a custom resolver: Resolvers::PackagePipelinesResolver
+      field :pipelines,
+            resolver: Resolvers::PackagePipelinesResolver,
+            description: 'Pipelines that built the package.',
+            deprecated: { reason: 'Due to scalability concerns, this field is going to be removed', milestone: '14.6' }
+
       def versions
         object.versions
       end

app/models/packages/build_info.rb

@@ -3,4 +3,10 @@
 class Packages::BuildInfo < ApplicationRecord
   belongs_to :package, inverse_of: :build_infos
   belongs_to :pipeline, class_name: 'Ci::Pipeline'
+
+  scope :pluck_pipeline_ids, -> { pluck(:pipeline_id) }
+  scope :without_empty_pipelines, -> { where.not(pipeline_id: nil) }
+  scope :order_by_pipeline_id, -> (direction) { order(pipeline_id: direction) }
+  scope :with_pipeline_id_less_than, -> (pipeline_id) { where("pipeline_id < ?", pipeline_id) }
+  scope :with_pipeline_id_greater_than, -> (pipeline_id) { where("pipeline_id > ?", pipeline_id) }
 end

db/migrate/20211202135508_add_index_on_packages_build_infos_package_id_pipeline_id.rb

+# frozen_string_literal: true
+
+class AddIndexOnPackagesBuildInfosPackageIdPipelineId < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_packages_build_infos_package_id_pipeline_id'
+  OLD_INDEX_NAME = 'idx_packages_build_infos_on_package_id'
+
+  def up
+    add_concurrent_index :packages_build_infos, [:package_id, :pipeline_id], name: INDEX_NAME
+    remove_concurrent_index_by_name :packages_build_infos, OLD_INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index :packages_build_infos, :package_id, name: OLD_INDEX_NAME
+    remove_concurrent_index :packages_build_infos, [:package_id, :pipeline_id], name: INDEX_NAME
+  end
+end

db/schema_migrations/20211202135508

+b565abbbb43f04ba4a6b77154ecb24b30328ac6d964f4be9fc5f9d05144606f0
\ No newline at end of file

db/structure.sql

@@ -25158,8 +25158,6 @@ CREATE UNIQUE INDEX idx_on_external_status_checks_project_id_external_url ON ext
 
 CREATE UNIQUE INDEX idx_on_external_status_checks_project_id_name ON external_status_checks USING btree (project_id, name);
 
-CREATE INDEX idx_packages_build_infos_on_package_id ON packages_build_infos USING btree (package_id);
-
 CREATE INDEX idx_packages_debian_group_component_files_on_architecture_id ON packages_debian_group_component_files USING btree (architecture_id);
 
 CREATE INDEX idx_packages_debian_project_component_files_on_architecture_id ON packages_debian_project_component_files USING btree (architecture_id);
@@ -26972,6 +26970,8 @@ CREATE UNIQUE INDEX index_ops_strategies_user_lists_on_strategy_id_and_user_list
 
 CREATE INDEX index_packages_build_infos_on_pipeline_id ON packages_build_infos USING btree (pipeline_id);
 
+CREATE INDEX index_packages_build_infos_package_id_pipeline_id ON packages_build_infos USING btree (package_id, pipeline_id);
+
 CREATE UNIQUE INDEX index_packages_composer_cache_namespace_and_sha ON packages_composer_cache_files USING btree (namespace_id, file_sha256);
 
 CREATE UNIQUE INDEX index_packages_composer_metadata_on_package_id_and_target_sha ON packages_composer_metadata USING btree (package_id, target_sha);

spec/finders/packages/build_infos_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Packages::BuildInfosFinder do
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:package) { create(:package) }
+  let_it_be(:build_infos) { create_list(:package_build_info, 5, :with_pipeline, package: package) }
+  let_it_be(:build_info_with_empty_pipeline) { create(:package_build_info, package: package) }
+
+  let(:finder) { described_class.new(package, params) }
+  let(:params) do
+    {
+      first: first,
+      last: last,
+      after: after,
+      before: before,
+      max_page_size: max_page_size,
+      support_next_page: support_next_page
+    }
+  end
+
+  describe '#execute' do
+    subject { finder.execute }
+
+    where(:first, :last, :after_index, :before_index, :max_page_size, :support_next_page, :expected_build_infos_indexes) do
+      # F   L     AI    BI    MPS   SNP
+      nil | nil | nil | nil | nil | false | [4, 3, 2, 1, 0]
+      nil | nil | nil | nil | 10  | false | [4, 3, 2, 1, 0]
+      nil | nil | nil | nil | 2   | false | [4, 3]
+      2   | nil | nil | nil | nil | false | [4, 3]
+      2   | nil | nil | nil | nil | true  | [4, 3, 2]
+      2   | nil | 3   | nil | nil | false | [2, 1]
+      2   | nil | 3   | nil | nil | true  | [2, 1, 0]
+      3   | nil | 4   | nil | 2   | false | [3, 2]
+      3   | nil | 4   | nil | 2   | true  | [3, 2, 1]
+      nil | 2   | nil | nil | nil | false | [0, 1]
+      nil | 2   | nil | nil | nil | true  | [0, 1, 2]
+      nil | 2   | nil | 1   | nil | false | [2, 3]
+      nil | 2   | nil | 1   | nil | true  | [2, 3, 4]
+      nil | 3   | nil | 0   | 2   | false | [1, 2]
+      nil | 3   | nil | 0   | 2   | true  | [1, 2, 3]
+    end
+
+    with_them do
+      let(:expected_build_infos) do
+        expected_build_infos_indexes.map do |idx|
+          build_infos[idx]
+        end
+      end
+
+      let(:after) do
+        build_infos[after_index].pipeline_id if after_index
+      end
+
+      let(:before) do
+        build_infos[before_index].pipeline_id if before_index
+      end
+
+      it { is_expected.to eq(expected_build_infos) }
+    end
+  end
+end

spec/graphql/resolvers/package_pipelines_resolver_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Resolvers::PackagePipelinesResolver do
+  include GraphqlHelpers
+
+  let_it_be_with_reload(:package) { create(:package) }
+  let_it_be(:pipelines) { create_list(:ci_pipeline, 3, project: package.project) }
+
+  let(:user) { package.project.owner }
+  let(:args) { {} }
+
+  describe '#resolve' do
+    subject { resolve(described_class, obj: package, args: args, ctx: { current_user: user }) }
+
+    before do
+      package.pipelines = pipelines
+      package.save!
+    end
+
+    it { is_expected.to contain_exactly(*pipelines) }
+
+    context 'with invalid after' do
+      let(:args) { { first: 1, after: 'not_json_string' } }
+
+      it 'raises argument error' do
+        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+      end
+    end
+
+    context 'with invalid after key' do
+      let(:args) { { first: 1, after: encode_cursor(foo: 3) } }
+
+      it 'raises argument error' do
+        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+      end
+    end
+
+    context 'with invalid before' do
+      let(:args) { { last: 1, before: 'not_json_string' } }
+
+      it 'raises argument error' do
+        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+      end
+    end
+
+    context 'with invalid before key' do
+      let(:args) { { last: 1, before: encode_cursor(foo: 3) } }
+
+      it 'raises argument error' do
+        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+      end
+    end
+
+    context 'field options' do
+      let(:field) do
+        field_options = described_class.field_options.merge(
+          owner: resolver_parent,
+          name: 'dummy_field'
+        )
+        ::Types::BaseField.new(**field_options)
+      end
+
+      it 'sets them properly' do
+        expect(field).not_to be_connection
+        expect(field.extras).to match_array([:lookahead])
+      end
+    end
+
+    context 'with unauthorized user' do
+      let_it_be(:user) { create(:user) }
+
+      it { is_expected.to be_nil }
+    end
+
+    def encode_cursor(json)
+      GitlabSchema.cursor_encoder.encode(
+        Gitlab::Json.dump(json),
+        nonce: true
+      )
+    end
+  end
+end

spec/graphql/types/packages/package_details_type_spec.rb

@@ -10,4 +10,13 @@ RSpec.describe GitlabSchema.types['PackageDetailsType'] do
 
     expect(described_class).to include_graphql_fields(*expected_fields)
   end
+
+  it 'overrides the pipelines field' do
+    field = described_class.fields['pipelines']
+
+    expect(field).to have_graphql_type(Types::Ci::PipelineType.connection_type)
+    expect(field).to have_graphql_extension(Gitlab::Graphql::Extensions::ExternallyPaginatedArrayExtension)
+    expect(field).to have_graphql_resolver(Resolvers::PackagePipelinesResolver)
+    expect(field).not_to be_connection
+  end
 end

spec/models/packages/build_info_spec.rb

@@ -6,4 +6,46 @@ RSpec.describe Packages::BuildInfo, type: :model do
     it { is_expected.to belong_to(:package) }
     it { is_expected.to belong_to(:pipeline) }
   end
+
+  context 'with some build infos' do
+    let_it_be(:package) { create(:package) }
+    let_it_be(:build_infos) { create_list(:package_build_info, 3, :with_pipeline, package: package) }
+    let_it_be(:build_info_with_no_pipeline) { create(:package_build_info) }
+
+    describe '.pluck_pipeline_ids' do
+      subject { package.build_infos.pluck_pipeline_ids.sort }
+
+      it { is_expected.to eq(build_infos.map(&:pipeline_id).sort) }
+    end
+
+    describe '.without_empty_pipelines' do
+      subject { package.build_infos.without_empty_pipelines }
+
+      it { is_expected.to contain_exactly(*build_infos) }
+    end
+
+    describe '.order_by_pipeline_id asc' do
+      subject { package.build_infos.order_by_pipeline_id(:asc) }
+
+      it { is_expected.to eq(build_infos) }
+    end
+
+    describe '.order_by_pipeline_id desc' do
+      subject { package.build_infos.order_by_pipeline_id(:desc) }
+
+      it { is_expected.to eq(build_infos.reverse) }
+    end
+
+    describe '.with_pipeline_id_less_than' do
+      subject { package.build_infos.with_pipeline_id_less_than(build_infos[1].pipeline_id) }
+
+      it { is_expected.to contain_exactly(build_infos[0]) }
+    end
+
+    describe '.with_pipeline_id_greater_than' do
+      subject { package.build_infos.with_pipeline_id_greater_than(build_infos[1].pipeline_id) }
+
+      it { is_expected.to contain_exactly(build_infos[2]) }
+    end
+  end
 end

spec/requests/api/graphql/packages/package_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 RSpec.describe 'package details' do
   include GraphqlHelpers
 
-  let_it_be(:project) { create(:project) }
+  let_it_be_with_reload(:project) { create(:project) }
   let_it_be(:composer_package) { create(:composer_package, project: project) }
   let_it_be(:composer_json) { { name: 'name', type: 'type', license: 'license', version: 1 } }
   let_it_be(:composer_metadatum) do
@@ -97,8 +97,23 @@ RSpec.describe 'package details' do
     end
   end
 
-  context 'when loading pipelines ordered by ID DESC' do
+  context 'with unauthorized user' do
+    let_it_be(:user) { create(:user) }
+
+    before do
+      project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+    end
+
+    it 'returns no packages' do
+      subject
+
+      expect(graphql_data_at(:package)).to be_nil
+    end
+  end
+
+  context 'pipelines field', :aggregate_failures do
     let(:pipelines) { create_list(:ci_pipeline, 6, project: project) }
+    let(:pipeline_gids) { pipelines.sort_by(&:id).map(&:to_gid).map(&:to_s).reverse }
 
     before do
       composer_package.pipelines = pipelines
@@ -111,6 +126,7 @@ RSpec.describe 'package details' do
         id
       }
       pageInfo {
+        startCursor
         endCursor
       }
       QUERY
@@ -119,20 +135,48 @@ RSpec.describe 'package details' do
       post_graphql(query, current_user: user)
     end
 
-    it 'loads the second page correctly' do
-      run_query({ first: 2 })
+    it 'loads the second page with pagination first correctly' do
+      run_query(first: 2)
+      pipeline_ids = graphql_data.dig('package', 'pipelines', 'nodes').pluck('id')
+
+      expect(pipeline_ids).to eq(pipeline_gids[0..1])
+
       cursor = graphql_data.dig('package', 'pipelines', 'pageInfo', 'endCursor')
 
-      run_query({ first: 2, after: cursor })
+      run_query(first: 2, after: cursor)
+
+      pipeline_ids = graphql_data.dig('package', 'pipelines', 'nodes').pluck('id')
+
+      expect(pipeline_ids).to eq(pipeline_gids[2..3])
+    end
+
+    it 'loads the second page with pagination last correctly' do
+      run_query(last: 2)
+      pipeline_ids = graphql_data.dig('package', 'pipelines', 'nodes').pluck('id')
 
-      expected_pipeline_ids = pipelines
-        .sort_by(&:id)
-        .reverse[2..3] # second page
-        .map { |pipeline| pipeline.to_gid.to_s }
+      expect(pipeline_ids).to eq(pipeline_gids[4..5])
+
+      cursor = graphql_data.dig('package', 'pipelines', 'pageInfo', 'startCursor')
+
+      run_query(last: 2, before: cursor)
 
       pipeline_ids = graphql_data.dig('package', 'pipelines', 'nodes').pluck('id')
 
-      expect(pipeline_ids).to eq(expected_pipeline_ids)
+      expect(pipeline_ids).to eq(pipeline_gids[2..3])
+    end
+
+    context 'with unauthorized user' do
+      let_it_be(:user) { create(:user) }
+
+      before do
+        project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'returns no packages' do
+        run_query(first: 2)
+
+        expect(graphql_data_at(:package)).to be_nil
+      end
     end
   end
 end