diff --git a/app/finders/releases_finder.rb b/app/finders/releases_finder.rb index 78240e0a0502108fd956bff4c3404d020559d5c5..16d3f46e1afba4aefcbb85a1451a9f225ba63872 100644 --- a/app/finders/releases_finder.rb +++ b/app/finders/releases_finder.rb @@ -6,7 +6,7 @@ class ReleasesFinder attr_reader :parent, :current_user, :params def initialize(parent, current_user = nil, params = {}) - @parent = parent + @parent = Array.wrap(parent) @current_user = current_user @params = params @@ -15,7 +15,7 @@ def initialize(parent, current_user = nil, params = {}) end def execute(preload: true) - return Release.none if projects.empty? + return Release.none if authorized_projects.empty? releases = get_releases releases = by_tag(releases) @@ -26,16 +26,17 @@ def execute(preload: true) private def get_releases - Release.where(project_id: projects).where.not(tag: nil) # rubocop: disable CodeReuse/ActiveRecord + Release.where(project_id: authorized_projects).where.not(tag: nil) # rubocop: disable CodeReuse/ActiveRecord end - def projects - strong_memoize(:projects) do - if parent.is_a?(Project) - Ability.allowed?(current_user, :read_release, parent) ? [parent] : [] - end - end + def authorized_projects + # Preload policy for all projects to avoid N+1 queries + projects = Project.id_in(parent.map(&:id)).include_project_feature + Preloaders::ProjectPolicyPreloader.new(projects, current_user).execute + + projects.select { |project| authorized?(project) } end + strong_memoize_attr :authorized_projects # rubocop: disable CodeReuse/ActiveRecord def by_tag(releases) @@ -48,4 +49,8 @@ def by_tag(releases) def order_releases(releases) releases.sort_by_attribute("#{params[:order_by]}_#{params[:sort]}") end + + def authorized?(project) + Ability.allowed?(current_user, :read_release, project) + end end diff --git a/app/graphql/resolvers/releases_resolver.rb b/app/graphql/resolvers/releases_resolver.rb index 06f4ca2065cbd9ae91fba8e4f27433588f064870..8c9235c2b5fb18c0c6107c818080ca82fa8f1a23 100644 --- a/app/graphql/resolvers/releases_resolver.rb +++ b/app/graphql/resolvers/releases_resolver.rb @@ -8,8 +8,6 @@ class ReleasesResolver < BaseResolver required: false, default_value: :released_at_desc, description: 'Sort releases by given criteria.' - alias_method :project, :object - # This resolver has a custom singular resolver def self.single Resolvers::ReleaseResolver @@ -23,11 +21,24 @@ def self.single }.freeze def resolve(sort:) - ReleasesFinder.new( - project, - current_user, - SORT_TO_PARAMS_MAP[sort] - ).execute + BatchLoader::GraphQL.for(project).batch do |projects, loader| + releases = ReleasesFinder.new( + projects, + current_user, + SORT_TO_PARAMS_MAP[sort] + ).execute + + # group_by will not cause N+1 queries here because ReleasesFinder preloads projects + releases.group_by(&:project).each do |project, versions| + loader.call(project, versions) + end + end + end + + private + + def project + object.respond_to?(:project) ? object.project : object end end end diff --git a/app/graphql/types/ci/catalog/resource_type.rb b/app/graphql/types/ci/catalog/resource_type.rb index b5947826fa16821e7de890f8084968df471cbaa6..e4566aac9aad696972a9fbbb48b82ff8fa936e56 100644 --- a/app/graphql/types/ci/catalog/resource_type.rb +++ b/app/graphql/types/ci/catalog/resource_type.rb @@ -20,6 +20,11 @@ class ResourceType < BaseObject field :icon, GraphQL::Types::String, null: true, description: 'Icon for the catalog resource.', method: :avatar_path, alpha: { milestone: '15.11' } + + field :versions, Types::ReleaseType.connection_type, null: true, + description: 'Versions of the catalog resource.', + resolver: Resolvers::ReleasesResolver, + alpha: { milestone: '16.1' } end # rubocop: enable Graphql/AuthorizeTypes end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 2b633e37fc35119ce3f907e5e36bc1139dd3615e..bee2f8c3640bfa5406bf555341b3f0458824658e 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -12345,6 +12345,28 @@ Represents the total number of issues and their weights for a particular day. | <a id="cicatalogresourceid"></a>`id` **{warning-solid}** | [`ID!`](#id) | **Introduced** in 15.11. This feature is an Experiment. It can be changed or removed at any time. ID of the catalog resource. | | <a id="cicatalogresourcename"></a>`name` **{warning-solid}** | [`String`](#string) | **Introduced** in 15.11. This feature is an Experiment. It can be changed or removed at any time. Name of the catalog resource. | +#### Fields with arguments + +##### `CiCatalogResource.versions` + +Versions of the catalog resource. + +WARNING: +**Introduced** in 16.1. +This feature is an Experiment. It can be changed or removed at any time. + +Returns [`ReleaseConnection`](#releaseconnection). + +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="cicatalogresourceversionssort"></a>`sort` | [`ReleaseSort`](#releasesort) | Sort releases by given criteria. | + ### `CiConfig` #### Fields diff --git a/ee/spec/requests/api/graphql/ci/catalog/resources_spec.rb b/ee/spec/requests/api/graphql/ci/catalog/resources_spec.rb index 95b2090be247caf0489353e6e93a3bc6a557c6dd..0016a9fdd019adc39c20c539d4ab28f920efbd3e 100644 --- a/ee/spec/requests/api/graphql/ci/catalog/resources_spec.rb +++ b/ee/spec/requests/api/graphql/ci/catalog/resources_spec.rb @@ -5,12 +5,11 @@ RSpec.describe 'Query.ciCatalogResources', feature_category: :pipeline_composition do include GraphqlHelpers - let_it_be(:namespace) { create(:group) } - let_it_be(:project_2) { create(:project, namespace: namespace) } - let_it_be(:resource_2) { create(:catalog_resource, project: project_2) } let_it_be(:user) { create(:user) } + let_it_be(:namespace) { create(:group) } + let_it_be(:project2) { create(:project, namespace: namespace) } - let_it_be(:project_1) do + let_it_be(:project1) do create( :project, :with_avatar, name: 'Component Repository', @@ -19,22 +18,36 @@ ) end - let_it_be(:resource_1) { create(:catalog_resource, project: project_1) } + let_it_be(:resource1) { create(:catalog_resource, project: project1) } let(:query) do - %( + <<~GQL query { - ciCatalogResources(projectPath: "#{project_2.full_path}") { - count - + ciCatalogResources(projectPath: "#{project1.full_path}") { nodes { - name - description - icon + #{all_graphql_fields_for('CiCatalogResource', max_depth: 1)} } } } - ) + GQL + end + + subject(:post_query) { post_graphql(query, current_user: user) } + + shared_examples 'avoids N+1 queries' do + it do + ctx = { current_user: user } + + control_count = ActiveRecord::QueryRecorder.new do + run_with_clean_state(query, context: ctx) + end + + create(:catalog_resource, project: project2) + + expect do + run_with_clean_state(query, context: ctx) + end.not_to exceed_query_limit(control_count) + end end context 'when the CI Namespace Catalog feature is available' do @@ -42,41 +55,203 @@ stub_licensed_features(ci_namespace_catalog: true) end - it 'returns all resources visible to the current user in the namespace' do - namespace.add_developer(user) + context 'when the current user has permission to read the namespace catalog' do + before do + namespace.add_developer(user) + end + + it 'returns the resource with the expected data' do + post_query + + expect(graphql_data_at(:ciCatalogResources, :nodes)).to contain_exactly( + a_graphql_entity_for(resource1, :name, :description, icon: project1.avatar_path) + ) + end - post_graphql(query, current_user: user) + context 'when there are two resources visible to the current user in the namespace' do + it 'returns both resources' do + resource2 = create(:catalog_resource, project: project2) - resources_data = graphql_data['ciCatalogResources'] - expect(resources_data['count']).to be(2) - expect(resources_data['nodes'].count).to be(2) + post_query - resource_1_data = resources_data['nodes'].first - expect(resource_1_data['name']).to eq('Component Repository') - expect(resource_1_data['description']).to eq('A simple component') - expect(resource_1_data['icon']).to eq(project_1.avatar_path) + expect(graphql_data_at(:ciCatalogResources, :nodes)).to contain_exactly( + a_graphql_entity_for(resource1), + a_graphql_entity_for(resource2) + ) + end + + it_behaves_like 'avoids N+1 queries' + end end context 'when the current user does not have permission to read the namespace catalog' do - it 'returns an empty array' do + it 'returns an empty response' do namespace.add_guest(user) - post_graphql(query, current_user: user) + post_query - resources_data = graphql_data['ciCatalogResources'] - expect(resources_data).to be_nil + expect(graphql_data_at(:ciCatalogResources)).to be_nil end end end context 'when the CI Namespace Catalog feature is not available' do - it 'returns an empty array' do + it 'returns an empty response' do namespace.add_developer(user) - post_graphql(query, current_user: user) + post_query + + expect(graphql_data_at(:ciCatalogResources)).to be_nil + end + end - resources_data = graphql_data['ciCatalogResources'] - expect(resources_data).to be_nil + describe 'versions' do + before do + stub_licensed_features(ci_namespace_catalog: true) + namespace.add_developer(user) + end + + let(:params) { '' } + + let(:query) do + <<~GQL + query { + ciCatalogResources(projectPath: "#{project1.full_path}") { + nodes { + id + versions#{params} { + nodes { + id + tagName + releasedAt + author { + id + name + webUrl + } + } + } + } + } + } + GQL + end + + context 'when the resource has versions' do + let_it_be(:resource1_author) { create(:user, name: 'resource1_author') } + let_it_be(:resource2_author) { create(:user, name: 'resource2_author') } + + let_it_be(:resource1_version1) do + create(:release, project: project1, released_at: '2023-01-01T00:00:00Z', author: resource1_author) + end + + let_it_be(:resource1_version2) do + create(:release, project: project1, released_at: '2023-02-01T00:00:00Z', author: resource1_author) + end + + let_it_be(:resource2_version1) do + create(:release, project: project2, released_at: '2023-03-01T00:00:00Z', author: resource2_author) + end + + let_it_be(:resource2_version2) do + create(:release, project: project2, released_at: '2023-04-01T00:00:00Z', author: resource2_author) + end + + it 'returns the resource with the versions data' do + post_query + + expect(graphql_data_at(:ciCatalogResources, :nodes)).to contain_exactly( + a_graphql_entity_for(resource1) + ) + + expect(graphql_data_at(:ciCatalogResources, :nodes, 0, :versions, :nodes)).to contain_exactly( + a_graphql_entity_for( + resource1_version1, + tagName: resource1_version1.tag, + releasedAt: resource1_version1.released_at, + author: a_graphql_entity_for(resource1_author, :name) + ), + a_graphql_entity_for( + resource1_version2, + tagName: resource1_version2.tag, + releasedAt: resource1_version2.released_at, + author: a_graphql_entity_for(resource1_author, :name) + ) + ) + end + + it 'returns the versions by released_at in descending order by default' do + post_query + + version_ids = graphql_data_at(:ciCatalogResources, :nodes, 0, :versions, :nodes).pluck('id') + + expect(version_ids).to eq([resource1_version2.to_global_id.to_s, resource1_version1.to_global_id.to_s]) + end + + context 'when sort parameter RELEASED_AT_ASC is provided' do + let(:params) { '(sort: RELEASED_AT_ASC)' } + + it 'returns the versions by released_at in ascending order' do + post_query + + version_ids = graphql_data_at(:ciCatalogResources, :nodes, 0, :versions, :nodes).pluck('id') + + expect(version_ids).to eq([resource1_version1.to_global_id.to_s, resource1_version2.to_global_id.to_s]) + end + end + + context 'when there are two resources visible to the current user in the namespace' do + it 'returns both resources with the versions data' do + resource2 = create(:catalog_resource, project: project2) + + post_query + + expect(graphql_data_at(:ciCatalogResources, :nodes)).to contain_exactly( + a_graphql_entity_for(resource1), + a_graphql_entity_for(resource2) + ) + + expect([ + graphql_data_at(:ciCatalogResources, :nodes, 0, :versions, :nodes), + graphql_data_at(:ciCatalogResources, :nodes, 1, :versions, :nodes) + ].flatten).to contain_exactly( + a_graphql_entity_for(resource1_version1, author: a_graphql_entity_for(resource1_author)), + a_graphql_entity_for(resource1_version2, author: a_graphql_entity_for(resource1_author)), + a_graphql_entity_for(resource2_version1, author: a_graphql_entity_for(resource2_author)), + a_graphql_entity_for(resource2_version2, author: a_graphql_entity_for(resource2_author)) + ) + end + + it_behaves_like 'avoids N+1 queries' + + context 'when obtaining the latest version of the resource' do + let(:params) { '(first: 1)' } + + it 'returns the latest versions of both resources' do + create(:catalog_resource, project: project2) + + post_query + + expect([ + graphql_data_at(:ciCatalogResources, :nodes, 0, :versions, :nodes), + graphql_data_at(:ciCatalogResources, :nodes, 1, :versions, :nodes) + ].flatten).to contain_exactly( + a_graphql_entity_for(resource1_version2, author: a_graphql_entity_for(resource1_author)), + a_graphql_entity_for(resource2_version2, author: a_graphql_entity_for(resource2_author)) + ) + end + end + end + end + + context 'when the resource does not have a version' do + it 'returns the resource without the version data' do + post_query + + expect(graphql_data_at(:ciCatalogResources, :nodes)).to contain_exactly( + a_graphql_entity_for(resource1, versions: nil) + ) + end end end end diff --git a/spec/finders/releases_finder_spec.rb b/spec/finders/releases_finder_spec.rb index 858a0e566f65dbb464eef09e90ca73c6e94327fc..a3418f08b7dad0c96c9ea6b133d39268823cf742 100644 --- a/spec/finders/releases_finder_spec.rb +++ b/spec/finders/releases_finder_spec.rb @@ -2,20 +2,15 @@ require 'spec_helper' -RSpec.describe ReleasesFinder do - let(:user) { create(:user) } - let(:group) { create :group } - let(:project) { create(:project, :repository, group: group) } +RSpec.describe ReleasesFinder, feature_category: :release_orchestration do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create :group } + let_it_be(:project) { create(:project, :repository, group: group) } let(:params) { {} } let(:args) { {} } let(:repository) { project.repository } - let(:v1_0_0) { create(:release, project: project, tag: 'v1.0.0') } - let(:v1_1_0) { create(:release, project: project, tag: 'v1.1.0') } - - before do - v1_0_0.update_attribute(:released_at, 2.days.ago) - v1_1_0.update_attribute(:released_at, 1.day.ago) - end + let_it_be(:v1_0_0) { create(:release, project: project, tag: 'v1.0.0') } + let_it_be(:v1_1_0) { create(:release, project: project, tag: 'v1.1.0') } shared_examples_for 'when the user is not part of the project' do it 'returns no releases' do @@ -67,21 +62,20 @@ context 'when the user is a project guest' do before do project.add_guest(user) + + v1_0_0.update!(released_at: 2.days.ago, created_at: 1.day.ago) + v1_1_0.update!(released_at: 1.day.ago, created_at: 2.days.ago) end - it 'sorts by release date' do + it 'returns the releases' do is_expected.to be_present expect(subject.size).to eq(2) - expect(subject).to eq([v1_1_0, v1_0_0]) + expect(subject).to match_array([v1_1_0, v1_0_0]) end context 'with sorting parameters' do - before do - v1_1_0.update_attribute(:created_at, 3.days.ago) - end - - context 'by default is released_at in descending order' do - it { is_expected.to eq([v1_1_0, v1_0_0]) } + it 'sorted by released_at in descending order by default' do + is_expected.to eq([v1_1_0, v1_0_0]) end context 'released_at in ascending order' do @@ -107,4 +101,73 @@ it_behaves_like 'when a tag parameter is passed' end end + + describe 'when parent is an array of projects' do + let_it_be(:project2) { create(:project, :repository, group: group) } + let_it_be(:v2_0_0) { create(:release, project: project2, tag: 'v2.0.0') } + let_it_be(:v2_1_0) { create(:release, project: project2, tag: 'v2.1.0') } + + subject { described_class.new([project, project2], user, params).execute(**args) } + + context 'when the user is not part of any project' do + it_behaves_like 'when the user is not part of the project' + end + + context 'when the user is only part of one project' do + before do + project.add_guest(user) + end + + it 'returns the releases of only the authorized project' do + is_expected.to be_present + expect(subject.size).to eq(2) + expect(subject).to match_array([v1_1_0, v1_0_0]) + end + end + + context 'when the user is a guest on all projects' do + before do + project.add_guest(user) + project2.add_guest(user) + + v1_0_0.update!(released_at: 4.days.ago, created_at: 1.day.ago) + v1_1_0.update!(released_at: 3.days.ago, created_at: 2.days.ago) + v2_0_0.update!(released_at: 2.days.ago, created_at: 3.days.ago) + v2_1_0.update!(released_at: 1.day.ago, created_at: 4.days.ago) + end + + it 'returns the releases of all projects' do + is_expected.to be_present + expect(subject.size).to eq(4) + expect(subject).to match_array([v2_1_0, v2_0_0, v1_1_0, v1_0_0]) + end + + it_behaves_like 'preload' + it_behaves_like 'when a tag parameter is passed' + + context 'with sorting parameters' do + it 'sorted by released_at in descending order by default' do + is_expected.to eq([v2_1_0, v2_0_0, v1_1_0, v1_0_0]) + end + + context 'released_at in ascending order' do + let(:params) { { sort: 'asc' } } + + it { is_expected.to eq([v1_0_0, v1_1_0, v2_0_0, v2_1_0]) } + end + + context 'order by created_at in descending order' do + let(:params) { { order_by: 'created_at' } } + + it { is_expected.to eq([v1_0_0, v1_1_0, v2_0_0, v2_1_0]) } + end + + context 'order by created_at in ascending order' do + let(:params) { { order_by: 'created_at', sort: 'asc' } } + + it { is_expected.to eq([v2_1_0, v2_0_0, v1_1_0, v1_0_0]) } + end + end + end + end end diff --git a/spec/graphql/resolvers/releases_resolver_spec.rb b/spec/graphql/resolvers/releases_resolver_spec.rb index 58f6257c946da0be5d15b9a348b06c72f41fecba..1e9608ab78082aa14bde251326fe9f9004240c33 100644 --- a/spec/graphql/resolvers/releases_resolver_spec.rb +++ b/spec/graphql/resolvers/releases_resolver_spec.rb @@ -54,7 +54,9 @@ private def resolve_releases - context = { current_user: current_user } - resolve(described_class, obj: project, args: args, ctx: context, arg_style: :internal) + batch_sync do + context = { current_user: current_user } + resolve(described_class, obj: project, args: args, ctx: context, arg_style: :internal) + end end end diff --git a/spec/graphql/types/ci/catalog/resource_type_spec.rb b/spec/graphql/types/ci/catalog/resource_type_spec.rb index d0bb45a4f1d432f99e2efae09c953fccb78c7f28..894522283cd5cf06764c8bbddb8672dcef497577 100644 --- a/spec/graphql/types/ci/catalog/resource_type_spec.rb +++ b/spec/graphql/types/ci/catalog/resource_type_spec.rb @@ -11,6 +11,7 @@ name description icon + versions ] expect(described_class).to have_graphql_fields(*expected_fields) diff --git a/spec/models/ci/catalog/resource_spec.rb b/spec/models/ci/catalog/resource_spec.rb index dfb7b311d96316120ec471899acbb0654b19eeaa..cccd164d4a80731c83772caa0212ec0960cc679d 100644 --- a/spec/models/ci/catalog/resource_spec.rb +++ b/spec/models/ci/catalog/resource_spec.rb @@ -10,13 +10,9 @@ let_it_be(:resource_2) { create(:catalog_resource, project: project_2) } let_it_be(:resource_3) { create(:catalog_resource, project: project_3) } - let_it_be(:releases) do - [ - create(:release, project: project, released_at: Time.zone.now - 2.days), - create(:release, project: project, released_at: Time.zone.now - 1.day), - create(:release, project: project, released_at: Time.zone.now) - ] - end + let_it_be(:release1) { create(:release, project: project, released_at: Time.zone.now - 2.days) } + let_it_be(:release2) { create(:release, project: project, released_at: Time.zone.now - 1.day) } + let_it_be(:release3) { create(:release, project: project, released_at: Time.zone.now) } it { is_expected.to belong_to(:project) } @@ -58,13 +54,13 @@ describe '#versions' do it 'returns releases ordered by released date descending' do - expect(resource.versions).to eq(releases.reverse) + expect(resource.versions).to eq([release3, release2, release1]) end end describe '#latest_version' do it 'returns the latest release' do - expect(resource.latest_version).to eq(releases.last) + expect(resource.latest_version).to eq(release3) end end end diff --git a/spec/support/shared_examples/graphql/resolvers/releases_resolvers_shared_examples.rb b/spec/support/shared_examples/graphql/resolvers/releases_resolvers_shared_examples.rb index 0e09a9d9e6699df0d2f748bf7117632f4fc06234..a1fa263c524a9c0acc35e9ed63ea024ae233d08a 100644 --- a/spec/support/shared_examples/graphql/resolvers/releases_resolvers_shared_examples.rb +++ b/spec/support/shared_examples/graphql/resolvers/releases_resolvers_shared_examples.rb @@ -4,8 +4,8 @@ context 'when the user does not have access to the project' do let(:current_user) { public_user } - it 'returns an empty array' do - expect(resolve_releases).to be_empty + it 'returns an empty response' do + expect(resolve_releases).to be_blank end end