From c0eeae4ffaad3b15014b132812a43c497c6eb07b Mon Sep 17 00:00:00 2001
From: Leaminn Ma <lma@gitlab.com>
Date: Tue, 23 May 2023 20:14:40 +0000
Subject: [PATCH] Add Graphql field to CiCatalogResource
Added Graphql field to CiCatalogResource.
Updated the ReleasesFinder so that can it take multiple
projects as input. Also updated ReleasesResolver with
batch loading so that releases for multiple projects
can be called in one query.
---
app/finders/releases_finder.rb | 23 +-
app/graphql/resolvers/releases_resolver.rb | 25 +-
app/graphql/types/ci/catalog/resource_type.rb | 5 +
doc/api/graphql/reference/index.md | 22 ++
.../api/graphql/ci/catalog/resources_spec.rb | 237 +++++++++++++++---
spec/finders/releases_finder_spec.rb | 101 ++++++--
.../resolvers/releases_resolver_spec.rb | 6 +-
.../types/ci/catalog/resource_type_spec.rb | 1 +
spec/models/ci/catalog/resource_spec.rb | 14 +-
.../releases_resolvers_shared_examples.rb | 4 +-
10 files changed, 359 insertions(+), 79 deletions(-)
diff --git a/app/finders/releases_finder.rb b/app/finders/releases_finder.rb
index 78240e0a0502..16d3f46e1afb 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 06f4ca2065cb..8c9235c2b5fb 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 b5947826fa16..e4566aac9aad 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 2b633e37fc35..bee2f8c3640b 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 95b2090be247..0016a9fdd019 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 858a0e566f65..a3418f08b7da 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 58f6257c946d..1e9608ab7808 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 d0bb45a4f1d4..894522283cd5 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 dfb7b311d963..cccd164d4a80 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 0e09a9d9e669..a1fa263c524a 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
--
GitLab