From a0f73de8cc5caa559e6c9909bebac78460e0f417 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro <noreply@pedro.pombei.ro> Date: Tue, 6 Sep 2022 15:41:19 +0000 Subject: [PATCH] GraphQL: Add resolver to runner projects Changelog: added --- app/finders/projects_finder.rb | 4 +- .../ci/runner_owner_project_resolver.rb | 15 ++- .../resolvers/ci/runner_projects_resolver.rb | 63 +++++++++++ .../concerns/project_search_arguments.rb | 36 ++++++ app/graphql/resolvers/projects_resolver.rb | 32 +----- app/graphql/types/ci/runner_type.rb | 20 +--- doc/api/graphql/reference/index.md | 31 +++++- ee/app/graphql/ee/types/ci/runner_type.rb | 4 - .../projects_resolver.rb | 2 +- .../ci/runner_projects_resolver_spec.rb | 69 ++++++++++++ spec/requests/api/graphql/ci/runner_spec.rb | 103 ++++++++++++++++-- 11 files changed, 307 insertions(+), 72 deletions(-) create mode 100644 app/graphql/resolvers/ci/runner_projects_resolver.rb create mode 100644 app/graphql/resolvers/concerns/project_search_arguments.rb create mode 100644 spec/graphql/resolvers/ci/runner_projects_resolver_spec.rb diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 6b8dcd61d2940..6bfe730ebc969 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -119,9 +119,9 @@ def collection_without_user # This is an optimization - surprisingly PostgreSQL does not optimize # for this. # - # If the default visiblity level and desired visiblity level filter cancels + # If the default visibility level and desired visibility level filter cancels # each other out, don't use the SQL clause for visibility level in - # `Project.public_or_visible_to_user`. In fact, this then becames equivalent + # `Project.public_or_visible_to_user`. In fact, this then becomes equivalent # to just authorized projects for the user. # # E.g. diff --git a/app/graphql/resolvers/ci/runner_owner_project_resolver.rb b/app/graphql/resolvers/ci/runner_owner_project_resolver.rb index 14b5f8f90eb19..da8fab9361938 100644 --- a/app/graphql/resolvers/ci/runner_owner_project_resolver.rb +++ b/app/graphql/resolvers/ci/runner_owner_project_resolver.rb @@ -9,7 +9,7 @@ class RunnerOwnerProjectResolver < BaseResolver alias_method :runner, :object - def resolve_with_lookahead(**args) + def resolve_with_lookahead(**_args) resolve_owner end @@ -19,6 +19,8 @@ def preloads } end + private + def filtered_preloads selection = lookahead @@ -27,8 +29,6 @@ def filtered_preloads end end - private - def resolve_owner return unless runner.project_type? @@ -48,14 +48,13 @@ def resolve_owner .transform_values { |runner_projects| runner_projects.first.project_id } project_ids = owner_project_id_by_runner_id.values.uniq - all_preloads = unconditional_includes + filtered_preloads - owner_relation = Project.all - owner_relation = owner_relation.preload(*all_preloads) if all_preloads.any? - projects = owner_relation.where(id: project_ids).index_by(&:id) + projects = Project.where(id: project_ids) + Preloaders::ProjectPolicyPreloader.new(projects, current_user).execute + projects_by_id = projects.index_by(&:id) runner_ids.each do |runner_id| owner_project_id = owner_project_id_by_runner_id[runner_id] - loader.call(runner_id, projects[owner_project_id]) + loader.call(runner_id, projects_by_id[owner_project_id]) end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/graphql/resolvers/ci/runner_projects_resolver.rb b/app/graphql/resolvers/ci/runner_projects_resolver.rb new file mode 100644 index 0000000000000..ca3b4ebb79755 --- /dev/null +++ b/app/graphql/resolvers/ci/runner_projects_resolver.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Resolvers + module Ci + class RunnerProjectsResolver < BaseResolver + include Gitlab::Graphql::Authorize::AuthorizeResource + include LooksAhead + include ProjectSearchArguments + + type Types::ProjectType.connection_type, null: true + authorize :read_runner + authorizes_object! + + alias_method :runner, :object + + argument :sort, GraphQL::Types::String, + required: false, + default_value: 'id_asc', # TODO: Remove in %16.0 and move :sort to ProjectSearchArguments, see https://gitlab.com/gitlab-org/gitlab/-/issues/372117 + deprecated: { + reason: 'Default sort order will change in 16.0. ' \ + 'Specify `"id_asc"` if query results\' order is important', + milestone: '15.4' + }, + description: "Sort order of results. Format: '<field_name>_<sort_direction>', " \ + "for example: 'id_desc' or 'name_asc'" + + def resolve_with_lookahead(**args) + return unless runner.project_type? + + # rubocop:disable CodeReuse/ActiveRecord + BatchLoader::GraphQL.for(runner.id).batch(key: :runner_projects) do |runner_ids, loader| + plucked_runner_and_project_ids = ::Ci::RunnerProject + .select(:runner_id, :project_id) + .where(runner_id: runner_ids) + .pluck(:runner_id, :project_id) + + project_ids = plucked_runner_and_project_ids.collect { |_runner_id, project_id| project_id }.uniq + projects = ProjectsFinder + .new(current_user: current_user, + params: project_finder_params(args), + project_ids_relation: project_ids) + .execute + Preloaders::ProjectPolicyPreloader.new(projects, current_user).execute + projects_by_id = projects.index_by(&:id) + + # In plucked_runner_and_project_ids, first() represents the runner ID, and second() the project ID, + # so let's group the project IDs by runner ID + runner_project_ids_by_runner_id = + plucked_runner_and_project_ids + .group_by(&:first) + .transform_values { |values| values.map(&:second).filter_map { |project_id| projects_by_id[project_id] } } + + runner_ids.each do |runner_id| + runner_projects = runner_project_ids_by_runner_id[runner_id] || [] + + loader.call(runner_id, runner_projects) + end + end + # rubocop:enable CodeReuse/ActiveRecord + end + end + end +end diff --git a/app/graphql/resolvers/concerns/project_search_arguments.rb b/app/graphql/resolvers/concerns/project_search_arguments.rb new file mode 100644 index 0000000000000..7e03963f412c0 --- /dev/null +++ b/app/graphql/resolvers/concerns/project_search_arguments.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module ProjectSearchArguments + extend ActiveSupport::Concern + + included do + argument :membership, GraphQL::Types::Boolean, + required: false, + description: 'Return only projects that the current user is a member of.' + + argument :search, GraphQL::Types::String, + required: false, + description: 'Search query, which can be for the project name, a path, or a description.' + + argument :search_namespaces, GraphQL::Types::Boolean, + required: false, + description: 'Include namespace in project search.' + + argument :topics, type: [GraphQL::Types::String], + required: false, + description: 'Filter projects by topics.' + end + + private + + def project_finder_params(params) + { + without_deleted: true, + non_public: params[:membership], + search: params[:search], + search_namespaces: params[:search_namespaces], + sort: params[:sort], + topic: params[:topics] + }.compact + end +end diff --git a/app/graphql/resolvers/projects_resolver.rb b/app/graphql/resolvers/projects_resolver.rb index facf8ffe36f20..4d1e1b867da88 100644 --- a/app/graphql/resolvers/projects_resolver.rb +++ b/app/graphql/resolvers/projects_resolver.rb @@ -2,31 +2,18 @@ module Resolvers class ProjectsResolver < BaseResolver - type Types::ProjectType, null: true - - argument :membership, GraphQL::Types::Boolean, - required: false, - description: 'Limit projects that the current user is a member of.' + include ProjectSearchArguments - argument :search, GraphQL::Types::String, - required: false, - description: 'Search query for project name, path, or description.' + type Types::ProjectType, null: true argument :ids, [GraphQL::Types::ID], required: false, description: 'Filter projects by IDs.' - argument :search_namespaces, GraphQL::Types::Boolean, - required: false, - description: 'Include namespace in project search.' - argument :sort, GraphQL::Types::String, required: false, - description: 'Sort order of results.' - - argument :topics, type: [GraphQL::Types::String], - required: false, - description: 'Filters projects by topics.' + description: "Sort order of results. Format: '<field_name>_<sort_direction>', " \ + "for example: 'id_desc' or 'name_asc'" def resolve(**args) ProjectsFinder @@ -36,17 +23,6 @@ def resolve(**args) private - def project_finder_params(params) - { - without_deleted: true, - non_public: params[:membership], - search: params[:search], - search_namespaces: params[:search_namespaces], - sort: params[:sort], - topic: params[:topics] - }.compact - end - def parse_gids(gids) gids&.map { |gid| GitlabSchema.parse_gid(gid, expected_type: ::Project).model_id } end diff --git a/app/graphql/types/ci/runner_type.rb b/app/graphql/types/ci/runner_type.rb index 0afb61d2b6473..ee0ded60d82f7 100644 --- a/app/graphql/types/ci/runner_type.rb +++ b/app/graphql/types/ci/runner_type.rb @@ -63,8 +63,11 @@ class RunnerType < BaseObject description: 'Indicates the runner is paused and not available to run jobs.' field :project_count, GraphQL::Types::Int, null: true, description: 'Number of projects that the runner is associated with.' - field :projects, ::Types::ProjectType.connection_type, null: true, - description: 'Projects the runner is associated with. For project runners only.' + field :projects, + ::Types::ProjectType.connection_type, + null: true, + resolver: ::Resolvers::Ci::RunnerProjectsResolver, + description: 'Find projects the runner is associated with. For project runners only.' field :revision, GraphQL::Types::String, null: true, description: 'Revision of the runner.' field :run_untagged, GraphQL::Types::Boolean, null: false, @@ -131,12 +134,6 @@ def groups batched_owners(::Ci::RunnerNamespace, Group, :runner_groups, :namespace_id) end - def projects - return unless runner.project_type? - - batched_owners(::Ci::RunnerProject, Project, :runner_projects, :project_id) - end - private def can_admin_runners? @@ -159,19 +156,12 @@ def batched_owners(runner_assoc_type, assoc_type, key, column_name) owner_ids = runner_owner_ids_by_runner_id.values.flatten.uniq owners = assoc_type.where(id: owner_ids).index_by(&:id) - # Preload projects namespaces to avoid N+1 queries when checking the `read_project` policy for each - preload_projects_namespaces(owners.values) if assoc_type == Project - runner_ids.each do |runner_id| loader.call(runner_id, runner_owner_ids_by_runner_id[runner_id]&.map { |owner_id| owners[owner_id] } || []) end end end # rubocop: enable CodeReuse/ActiveRecord - - def preload_projects_namespaces(_projects) - # overridden in EE - end end end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index d7e3ae81ad052..0b6591136ec93 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -317,11 +317,11 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="queryprojectsids"></a>`ids` | [`[ID!]`](#id) | Filter projects by IDs. | -| <a id="queryprojectsmembership"></a>`membership` | [`Boolean`](#boolean) | Limit projects that the current user is a member of. | -| <a id="queryprojectssearch"></a>`search` | [`String`](#string) | Search query for project name, path, or description. | +| <a id="queryprojectsmembership"></a>`membership` | [`Boolean`](#boolean) | Return only projects that the current user is a member of. | +| <a id="queryprojectssearch"></a>`search` | [`String`](#string) | Search query, which can be for the project name, a path, or a description. | | <a id="queryprojectssearchnamespaces"></a>`searchNamespaces` | [`Boolean`](#boolean) | Include namespace in project search. | -| <a id="queryprojectssort"></a>`sort` | [`String`](#string) | Sort order of results. | -| <a id="queryprojectstopics"></a>`topics` | [`[String!]`](#string) | Filters projects by topics. | +| <a id="queryprojectssort"></a>`sort` | [`String`](#string) | Sort order of results. Format: '<field_name>_<sort_direction>', for example: 'id_desc' or 'name_asc'. | +| <a id="queryprojectstopics"></a>`topics` | [`[String!]`](#string) | Filter projects by topics. | ### `Query.queryComplexity` @@ -10360,7 +10360,6 @@ CI/CD variables for a project. | <a id="cirunnerplatformname"></a>`platformName` | [`String`](#string) | Platform provided by the runner. | | <a id="cirunnerprivateprojectsminutescostfactor"></a>`privateProjectsMinutesCostFactor` | [`Float`](#float) | Private projects' "minutes cost factor" associated with the runner (GitLab.com only). | | <a id="cirunnerprojectcount"></a>`projectCount` | [`Int`](#int) | Number of projects that the runner is associated with. | -| <a id="cirunnerprojects"></a>`projects` | [`ProjectConnection`](#projectconnection) | Projects the runner is associated with. For project runners only. (see [Connections](#connections)) | | <a id="cirunnerpublicprojectsminutescostfactor"></a>`publicProjectsMinutesCostFactor` | [`Float`](#float) | Public projects' "minutes cost factor" associated with the runner (GitLab.com only). | | <a id="cirunnerrevision"></a>`revision` | [`String`](#string) | Revision of the runner. | | <a id="cirunnerrununtagged"></a>`runUntagged` | [`Boolean!`](#boolean) | Indicates the runner is able to run untagged jobs. | @@ -10390,6 +10389,26 @@ four standard [pagination arguments](#connection-pagination-arguments): | ---- | ---- | ----------- | | <a id="cirunnerjobsstatuses"></a>`statuses` | [`[CiJobStatus!]`](#cijobstatus) | Filter jobs by status. | +##### `CiRunner.projects` + +Find projects the runner is associated with. For project runners only. + +Returns [`ProjectConnection`](#projectconnection). + +This field returns a [connection](#connections). It accepts the +four standard [pagination arguments](#connection-pagination-arguments): +`before: String`, `after: String`, `first: Int`, `last: Int`. + +###### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="cirunnerprojectsmembership"></a>`membership` | [`Boolean`](#boolean) | Return only projects that the current user is a member of. | +| <a id="cirunnerprojectssearch"></a>`search` | [`String`](#string) | Search query, which can be for the project name, a path, or a description. | +| <a id="cirunnerprojectssearchnamespaces"></a>`searchNamespaces` | [`Boolean`](#boolean) | Include namespace in project search. | +| <a id="cirunnerprojectssort"></a>`sort` **{warning-solid}** | [`String`](#string) | **Deprecated** in 15.4. Default sort order will change in 16.0. Specify `"id_asc"` if query results' order is important. | +| <a id="cirunnerprojectstopics"></a>`topics` | [`[String!]`](#string) | Filter projects by topics. | + ##### `CiRunner.status` Status of the runner. @@ -13295,7 +13314,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | -| <a id="instancesecuritydashboardprojectssearch"></a>`search` | [`String`](#string) | Search query for project name, path, or description. | +| <a id="instancesecuritydashboardprojectssearch"></a>`search` | [`String`](#string) | Search query, which can be for the project name, a path, or a description. | ##### `InstanceSecurityDashboard.vulnerabilitySeveritiesCount` diff --git a/ee/app/graphql/ee/types/ci/runner_type.rb b/ee/app/graphql/ee/types/ci/runner_type.rb index cdda46d69984b..256aef69d7dd7 100644 --- a/ee/app/graphql/ee/types/ci/runner_type.rb +++ b/ee/app/graphql/ee/types/ci/runner_type.rb @@ -37,10 +37,6 @@ def upgrade_status def upgrade_status_available? License.feature_available?(:runner_upgrade_management) || current_user&.has_paid_namespace? end - - def preload_projects_namespaces(projects) - ActiveRecord::Associations::Preloader.new.preload(projects, :namespace) # rubocop:disable CodeReuse/ActiveRecord - end end end end diff --git a/ee/app/graphql/resolvers/instance_security_dashboard/projects_resolver.rb b/ee/app/graphql/resolvers/instance_security_dashboard/projects_resolver.rb index 94b1da07b1cb4..cea791076d994 100644 --- a/ee/app/graphql/resolvers/instance_security_dashboard/projects_resolver.rb +++ b/ee/app/graphql/resolvers/instance_security_dashboard/projects_resolver.rb @@ -7,7 +7,7 @@ class ProjectsResolver < BaseResolver argument :search, GraphQL::Types::String, required: false, - description: 'Search query for project name, path, or description.' + description: 'Search query, which can be for the project name, a path, or a description.' alias_method :dashboard, :object diff --git a/spec/graphql/resolvers/ci/runner_projects_resolver_spec.rb b/spec/graphql/resolvers/ci/runner_projects_resolver_spec.rb new file mode 100644 index 0000000000000..952c7337d65f2 --- /dev/null +++ b/spec/graphql/resolvers/ci/runner_projects_resolver_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Resolvers::Ci::RunnerProjectsResolver do + include GraphqlHelpers + + let_it_be(:project1) { create(:project, description: 'Project1.1') } + let_it_be(:project2) { create(:project, description: 'Project1.2') } + let_it_be(:project3) { create(:project, description: 'Project2.1') } + let_it_be(:runner) { create(:ci_runner, :project, projects: [project1, project2, project3]) } + + let(:args) { {} } + + subject { resolve_projects(args) } + + describe '#resolve' do + context 'with authorized user', :enable_admin_mode do + let(:current_user) { create(:user, :admin) } + + context 'with search argument' do + let(:args) { { search: 'Project1.' } } + + it 'returns a lazy value with projects containing the specified prefix' do + expect(subject).to be_a(GraphQL::Execution::Lazy) + expect(subject.value).to contain_exactly(project1, project2) + end + end + + context 'with supported arguments' do + let(:args) { { membership: true, search_namespaces: true, topics: %w[xyz] } } + + it 'creates ProjectsFinder with expected arguments' do + expect(ProjectsFinder).to receive(:new).with( + a_hash_including( + params: a_hash_including( + non_public: true, + search_namespaces: true, + topic: %w[xyz] + ) + ) + ).and_call_original + + expect(subject).to be_a(GraphQL::Execution::Lazy) + subject.value + end + end + + context 'without arguments' do + it 'returns a lazy value with all projects' do + expect(subject).to be_a(GraphQL::Execution::Lazy) + expect(subject.value).to contain_exactly(project1, project2, project3) + end + end + end + + context 'with unauthorized user' do + let(:current_user) { create(:user) } + + it { is_expected.to be_nil } + end + end + + private + + def resolve_projects(args = {}, context = { current_user: current_user }) + resolve(described_class, obj: runner, args: args, ctx: context) + end +end diff --git a/spec/requests/api/graphql/ci/runner_spec.rb b/spec/requests/api/graphql/ci/runner_spec.rb index 8ed84c25bf0c3..8bd002d533a18 100644 --- a/spec/requests/api/graphql/ci/runner_spec.rb +++ b/spec/requests/api/graphql/ci/runner_spec.rb @@ -54,7 +54,8 @@ executor_type: :shell) end - let_it_be(:active_project_runner) { create(:ci_runner, :project) } + let_it_be(:project1) { create(:project) } + let_it_be(:active_project_runner) { create(:ci_runner, :project, projects: [project1]) } shared_examples 'runner details fetch' do let(:query) do @@ -223,7 +224,6 @@ end describe 'ownerProject' do - let_it_be(:project1) { create(:project) } let_it_be(:project2) { create(:project) } let_it_be(:runner1) { create(:ci_runner, :project, projects: [project2, project1]) } let_it_be(:runner2) { create(:ci_runner, :project, projects: [project1, project2]) } @@ -337,7 +337,6 @@ end describe 'for multiple runners' do - let_it_be(:project1) { create(:project, :test_repo) } let_it_be(:project2) { create(:project, :test_repo) } let_it_be(:project_runner1) { create(:ci_runner, :project, projects: [project1, project2], description: 'Runner 1') } let_it_be(:project_runner2) { create(:ci_runner, :project, projects: [], description: 'Runner 2') } @@ -508,8 +507,8 @@ def runner_query(runner) <<~QUERY { instance_runner1: #{runner_query(active_instance_runner)} - project_runner1: #{runner_query(active_project_runner)} group_runner1: #{runner_query(active_group_runner)} + project_runner1: #{runner_query(active_project_runner)} } QUERY end @@ -529,12 +528,13 @@ def runner_query(runner) it 'does not execute more queries per runner', :aggregate_failures do # warm-up license cache and so on: - post_graphql(double_query, current_user: user) + personal_access_token = create(:personal_access_token, user: user) + args = { current_user: user, token: { personal_access_token: personal_access_token } } + post_graphql(double_query, **args) - control = ActiveRecord::QueryRecorder.new { post_graphql(single_query, current_user: user) } + control = ActiveRecord::QueryRecorder.new { post_graphql(single_query, **args) } - expect { post_graphql(double_query, current_user: user) } - .not_to exceed_query_limit(control) + expect { post_graphql(double_query, **args) }.not_to exceed_query_limit(control) expect(graphql_data.count).to eq 6 expect(graphql_data).to match( @@ -564,4 +564,91 @@ def runner_query(runner) )) end end + + describe 'sorting and pagination' do + let(:query) do + <<~GQL + query($id: CiRunnerID!, $projectSearchTerm: String, $n: Int, $cursor: String) { + runner(id: $id) { + #{fields} + } + } + GQL + end + + before do + post_graphql(query, current_user: user, variables: variables) + end + + context 'with project search term' do + let_it_be(:project1) { create(:project, description: 'abc') } + let_it_be(:project2) { create(:project, description: 'def') } + let_it_be(:project_runner) do + create(:ci_runner, :project, projects: [project1, project2]) + end + + let(:variables) { { id: project_runner.to_global_id.to_s, n: n, project_search_term: search_term } } + + let(:fields) do + <<~QUERY + projects(search: $projectSearchTerm, first: $n, after: $cursor) { + count + nodes { + id + } + pageInfo { + hasPreviousPage + startCursor + endCursor + hasNextPage + } + } + QUERY + end + + let(:projects_data) { graphql_data_at('runner', 'projects') } + + context 'set to empty string' do + let(:search_term) { '' } + + context 'with n = 1' do + let(:n) { 1 } + + it_behaves_like 'a working graphql query' + + it 'returns paged result' do + expect(projects_data).not_to be_nil + expect(projects_data['count']).to eq 2 + expect(projects_data['pageInfo']['hasNextPage']).to eq true + end + end + + context 'with n = 2' do + let(:n) { 2 } + + it 'returns non-paged result' do + expect(projects_data).not_to be_nil + expect(projects_data['count']).to eq 2 + expect(projects_data['pageInfo']['hasNextPage']).to eq false + end + end + end + + context 'set to partial match' do + let(:search_term) { 'def' } + + context 'with n = 1' do + let(:n) { 1 } + + it_behaves_like 'a working graphql query' + + it 'returns paged result with no additional pages' do + expect(projects_data).not_to be_nil + expect(projects_data['count']).to eq 1 + expect(projects_data['pageInfo']['hasNextPage']).to eq false + end + end + end + end + end end -- GitLab