From 58b9df24e628a143fee76e966f962c7b7209f4b2 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Sat, 5 Sep 2020 03:08:31 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- .../user_group_notification_settings_finder.rb | 6 ++- .../resolvers/namespace_projects_resolver.rb | 22 +++++++-- .../types/projects/namespace_project_sort_enum.rb | 12 +++++ app/models/project.rb | 7 ++- ...otificationsettings_page_nonexistent_parent.yml | 5 ++ .../similarity-sorting-of-projects-graphql-api.yml | 5 ++ doc/api/graphql/reference/gitlab_schema.graphql | 30 ++++++++++++ doc/api/graphql/reference/gitlab_schema.json | 57 ++++++++++++++++++++++ .../pagination/keyset/conditions/base_condition.rb | 5 +- lib/gitlab/graphql/pagination/keyset/order_info.rb | 21 ++++---- ...user_group_notification_settings_finder_spec.rb | 37 ++++++++++++++ .../resolvers/namespace_projects_resolver_spec.rb | 51 +++++++++++++++++-- .../graphql/pagination/keyset/connection_spec.rb | 25 ++++++---- .../graphql/pagination/keyset/order_info_spec.rb | 12 +++++ .../pagination/keyset/query_builder_spec.rb | 37 ++++++++++++++ .../api/graphql/namespace/projects_spec.rb | 39 +++++++++++++++ .../sorted_paginated_query_shared_examples.rb | 3 ++ 17 files changed, 345 insertions(+), 29 deletions(-) create mode 100644 app/graphql/types/projects/namespace_project_sort_enum.rb create mode 100644 changelogs/unreleased/fix_notificationsettings_page_nonexistent_parent.yml create mode 100644 changelogs/unreleased/similarity-sorting-of-projects-graphql-api.yml diff --git a/app/finders/user_group_notification_settings_finder.rb b/app/finders/user_group_notification_settings_finder.rb index a29cf409692..a6f6769116f 100644 --- a/app/finders/user_group_notification_settings_finder.rb +++ b/app/finders/user_group_notification_settings_finder.rb @@ -7,7 +7,9 @@ class UserGroupNotificationSettingsFinder end def execute - groups_with_ancestors = Gitlab::ObjectHierarchy.new(groups).base_and_ancestors + # rubocop: disable CodeReuse/ActiveRecord + groups_with_ancestors = Gitlab::ObjectHierarchy.new(Group.where(id: groups.select(:id))).base_and_ancestors + # rubocop: enable CodeReuse/ActiveRecord @loaded_groups_with_ancestors = groups_with_ancestors.index_by(&:id) @loaded_notification_settings = user.notification_settings_for_groups(groups_with_ancestors).preload_source_route.index_by(&:source_id) @@ -27,6 +29,8 @@ class UserGroupNotificationSettingsFinder parent_setting = loaded_notification_settings[group.parent_id] + return user.notification_settings.build(source: group) unless parent_setting + if should_copy?(parent_setting) user.notification_settings.build(source: group) do |ns| ns.assign_attributes(parent_setting.slice(*NotificationSetting.allowed_fields)) diff --git a/app/graphql/resolvers/namespace_projects_resolver.rb b/app/graphql/resolvers/namespace_projects_resolver.rb index e841132eea7..c221cb9aed6 100644 --- a/app/graphql/resolvers/namespace_projects_resolver.rb +++ b/app/graphql/resolvers/namespace_projects_resolver.rb @@ -7,19 +7,33 @@ module Resolvers default_value: false, description: 'Include also subgroup projects' + argument :search, GraphQL::STRING_TYPE, + required: false, + default_value: nil, + description: 'Search project with most similar names or paths' + + argument :sort, Types::Projects::NamespaceProjectSortEnum, + required: false, + default_value: nil, + description: 'Sort projects by this criteria' + type Types::ProjectType, null: true - def resolve(include_subgroups:) + def resolve(include_subgroups:, sort:, search:) # The namespace could have been loaded in batch by `BatchLoader`. # At this point we need the `id` or the `full_path` of the namespace # to query for projects, so make sure it's loaded and not `nil` before continuing. namespace = object.respond_to?(:sync) ? object.sync : object return Project.none if namespace.nil? - if include_subgroups - namespace.all_projects.with_route + query = include_subgroups ? namespace.all_projects.with_route : namespace.projects.with_route + + return query unless search.present? + + if sort == :similarity + query.sorted_by_similarity_desc(search, include_in_select: true).merge(Project.search(search)) else - namespace.projects.with_route + query.merge(Project.search(search)) end end diff --git a/app/graphql/types/projects/namespace_project_sort_enum.rb b/app/graphql/types/projects/namespace_project_sort_enum.rb new file mode 100644 index 00000000000..1e13deb6508 --- /dev/null +++ b/app/graphql/types/projects/namespace_project_sort_enum.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Types + module Projects + class NamespaceProjectSortEnum < BaseEnum + graphql_name 'NamespaceProjectSort' + description 'Values for sorting projects' + + value 'SIMILARITY', 'Most similar to the search query', value: :similarity + end + end +end diff --git a/app/models/project.rb b/app/models/project.rb index a5317d14dd8..8e781ffe55d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -461,14 +461,17 @@ class Project < ApplicationRecord # Sometimes queries (e.g. using CTEs) require explicit disambiguation with table name scope :projects_order_id_desc, -> { reorder(self.arel_table['id'].desc) } - scope :sorted_by_similarity_desc, -> (search) do + scope :sorted_by_similarity_desc, -> (search, include_in_select: false) do order_expression = Gitlab::Database::SimilarityScore.build_expression(search: search, rules: [ { column: arel_table["path"], multiplier: 1 }, { column: arel_table["name"], multiplier: 0.7 }, { column: arel_table["description"], multiplier: 0.2 } ]) - reorder(order_expression.desc, arel_table['id'].desc) + query = reorder(order_expression.desc, arel_table['id'].desc) + + query = query.select(*query.arel.projections, order_expression.as('similarity')) if include_in_select + query end scope :with_packages, -> { joins(:packages) } diff --git a/changelogs/unreleased/fix_notificationsettings_page_nonexistent_parent.yml b/changelogs/unreleased/fix_notificationsettings_page_nonexistent_parent.yml new file mode 100644 index 00000000000..c1466cca5f5 --- /dev/null +++ b/changelogs/unreleased/fix_notificationsettings_page_nonexistent_parent.yml @@ -0,0 +1,5 @@ +--- +title: NotificationsController - Handle mising parent notificationsetting +merge_request: 41612 +author: +type: fixed diff --git a/changelogs/unreleased/similarity-sorting-of-projects-graphql-api.yml b/changelogs/unreleased/similarity-sorting-of-projects-graphql-api.yml new file mode 100644 index 00000000000..1c7779b4512 --- /dev/null +++ b/changelogs/unreleased/similarity-sorting-of-projects-graphql-api.yml @@ -0,0 +1,5 @@ +--- +title: Add similarity sorting for projects for GraphQL API +merge_request: 38916 +author: +type: added diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index c49ff1a5609..ca3c71761a2 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -6776,6 +6776,16 @@ type Group { Returns the last _n_ elements from the list. """ last: Int + + """ + Search project with most similar names or paths + """ + search: String = null + + """ + Sort projects by this criteria + """ + sort: NamespaceProjectSort = null ): ProjectConnection! """ @@ -10366,6 +10376,16 @@ type Namespace { Returns the last _n_ elements from the list. """ last: Int + + """ + Search project with most similar names or paths + """ + search: String = null + + """ + Sort projects by this criteria + """ + sort: NamespaceProjectSort = null ): ProjectConnection! """ @@ -10464,6 +10484,16 @@ type NamespaceIncreaseStorageTemporarilyPayload { namespace: Namespace } +""" +Values for sorting projects +""" +enum NamespaceProjectSort { + """ + Most similar to the search query + """ + SIMILARITY +} + input NegatedBoardIssueInput { """ Filter by assignee username diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index e7beae1b01a..fd545d4cbaa 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -18629,6 +18629,26 @@ }, "defaultValue": "false" }, + { + "name": "search", + "description": "Search project with most similar names or paths", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": "null" + }, + { + "name": "sort", + "description": "Sort projects by this criteria", + "type": { + "kind": "ENUM", + "name": "NamespaceProjectSort", + "ofType": null + }, + "defaultValue": "null" + }, { "name": "hasVulnerabilities", "description": "Returns only the projects which have vulnerabilities", @@ -30963,6 +30983,26 @@ }, "defaultValue": "false" }, + { + "name": "search", + "description": "Search project with most similar names or paths", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": "null" + }, + { + "name": "sort", + "description": "Sort projects by this criteria", + "type": { + "kind": "ENUM", + "name": "NamespaceProjectSort", + "ofType": null + }, + "defaultValue": "null" + }, { "name": "hasVulnerabilities", "description": "Returns only the projects which have vulnerabilities", @@ -31318,6 +31358,23 @@ "enumValues": null, "possibleTypes": null }, + { + "kind": "ENUM", + "name": "NamespaceProjectSort", + "description": "Values for sorting projects", + "fields": null, + "inputFields": null, + "interfaces": null, + "enumValues": [ + { + "name": "SIMILARITY", + "description": "Most similar to the search query", + "isDeprecated": false, + "deprecationReason": null + } + ], + "possibleTypes": null + }, { "kind": "INPUT_OBJECT", "name": "NegatedBoardIssueInput", diff --git a/lib/gitlab/graphql/pagination/keyset/conditions/base_condition.rb b/lib/gitlab/graphql/pagination/keyset/conditions/base_condition.rb index 25b68db2233..bd785880b57 100644 --- a/lib/gitlab/graphql/pagination/keyset/conditions/base_condition.rb +++ b/lib/gitlab/graphql/pagination/keyset/conditions/base_condition.rb @@ -29,7 +29,10 @@ module Gitlab def table_condition(order_info, value, operator) if order_info.named_function target = order_info.named_function - value = value&.downcase if target.respond_to?(:name) && target&.name&.downcase == 'lower' + + if target.try(:name)&.casecmp('lower') == 0 + value = value&.downcase + end else target = arel_table[order_info.attribute_name] end diff --git a/lib/gitlab/graphql/pagination/keyset/order_info.rb b/lib/gitlab/graphql/pagination/keyset/order_info.rb index df103a73209..f54695ddb9a 100644 --- a/lib/gitlab/graphql/pagination/keyset/order_info.rb +++ b/lib/gitlab/graphql/pagination/keyset/order_info.rb @@ -90,21 +90,24 @@ module Gitlab end def extract_attribute_values(order_value) - named = nil - name = if ordering_by_lower?(order_value) - named = order_value.expr - named.expressions[0].name.to_s - else - order_value.expr.name - end - - [name, order_value.direction, named] + if ordering_by_lower?(order_value) + [order_value.expr.expressions[0].name.to_s, order_value.direction, order_value.expr] + elsif ordering_by_similarity?(order_value) + ['similarity', order_value.direction, order_value.expr] + else + [order_value.expr.name, order_value.direction, nil] + end end # determine if ordering using LOWER, eg. "ORDER BY LOWER(boards.name)" def ordering_by_lower?(order_value) order_value.expr.is_a?(Arel::Nodes::NamedFunction) && order_value.expr&.name&.downcase == 'lower' end + + # determine if ordering using SIMILARITY scoring based on Gitlab::Database::SimilarityScore + def ordering_by_similarity?(order_value) + order_value.to_sql.match?(/SIMILARITY\(.+\*/) + end end end end diff --git a/spec/finders/user_group_notification_settings_finder_spec.rb b/spec/finders/user_group_notification_settings_finder_spec.rb index 6f391621999..453da691866 100644 --- a/spec/finders/user_group_notification_settings_finder_spec.rb +++ b/spec/finders/user_group_notification_settings_finder_spec.rb @@ -71,6 +71,43 @@ RSpec.describe UserGroupNotificationSettingsFinder do end end + context 'when the group has parent_id set but that does not belong to any group' do + let_it_be(:group) { create(:group) } + let_it_be(:groups) { [group] } + + before do + # Let's set a parent_id for a group that definitely doesn't exist + group.update_columns(parent_id: 19283746) + end + + it 'returns a default Global notification setting' do + expect(subject.count).to eq(1) + expect(attributes(&:level)).to eq(['global']) + expect(attributes(&:notification_email)).to eq([nil]) + end + end + + context 'when the group has a private parent' do + let_it_be(:ancestor) { create(:group, :private) } + let_it_be(:group) { create(:group, :private, parent: ancestor) } + let_it_be(:ancestor_email) { create(:email, :confirmed, email: 'ancestor@example.com', user: user) } + let_it_be(:groups) { [group] } + + before do + group.add_reporter(user) + # Adding the user creates a NotificationSetting, so we remove it here + user.notification_settings.where(source: group).delete_all + + create(:notification_setting, user: user, source: ancestor, level: 'participating', notification_email: ancestor_email.email) + end + + it 'still inherits the notification settings' do + expect(subject.count).to eq(1) + expect(attributes(&:level)).to eq(['participating']) + expect(attributes(&:notification_email)).to eq([ancestor_email.email]) + end + end + it 'does not cause an N+1', :aggregate_failures do parent = create(:group) child = create(:group, parent: parent) diff --git a/spec/graphql/resolvers/namespace_projects_resolver_spec.rb b/spec/graphql/resolvers/namespace_projects_resolver_spec.rb index 699269b47e0..4ad8f99219f 100644 --- a/spec/graphql/resolvers/namespace_projects_resolver_spec.rb +++ b/spec/graphql/resolvers/namespace_projects_resolver_spec.rb @@ -27,7 +27,7 @@ RSpec.describe Resolvers::NamespaceProjectsResolver do end it 'finds all projects including the subgroups' do - expect(resolve_projects(include_subgroups: true)).to contain_exactly(project1, project2, nested_project) + expect(resolve_projects(include_subgroups: true, sort: nil, search: nil)).to contain_exactly(project1, project2, nested_project) end context 'with an user namespace' do @@ -38,7 +38,52 @@ RSpec.describe Resolvers::NamespaceProjectsResolver do end it 'finds all projects including the subgroups' do - expect(resolve_projects(include_subgroups: true)).to contain_exactly(project1, project2) + expect(resolve_projects(include_subgroups: true, sort: nil, search: nil)).to contain_exactly(project1, project2) + end + end + end + + context 'search and similarity sorting' do + let(:project_1) { create(:project, name: 'Project', path: 'project', namespace: namespace) } + let(:project_2) { create(:project, name: 'Test Project', path: 'test-project', namespace: namespace) } + let(:project_3) { create(:project, name: 'Test', path: 'test', namespace: namespace) } + + before do + project_1.add_developer(current_user) + project_2.add_developer(current_user) + project_3.add_developer(current_user) + end + + it 'returns projects ordered by similarity to the search input' do + projects = resolve_projects(include_subgroups: true, sort: :similarity, search: 'test') + + project_names = projects.map { |proj| proj['name'] } + expect(project_names.first).to eq('Test') + expect(project_names.second).to eq('Test Project') + end + + it 'filters out result that do not match the search input' do + projects = resolve_projects(include_subgroups: true, sort: :similarity, search: 'test') + + project_names = projects.map { |proj| proj['name'] } + expect(project_names).not_to include('Project') + end + + context 'when `search` parameter is not given' do + it 'returns projects not ordered by similarity' do + projects = resolve_projects(include_subgroups: true, sort: :similarity, search: nil) + + project_names = projects.map { |proj| proj['name'] } + expect(project_names.first).not_to eq('Test') + end + end + + context 'when only search term is given' do + it 'filters out result that do not match the search input, but does not sort them' do + projects = resolve_projects(include_subgroups: true, sort: :nil, search: 'test') + + project_names = projects.map { |proj| proj['name'] } + expect(project_names).to contain_exactly('Test', 'Test Project') end end end @@ -63,7 +108,7 @@ RSpec.describe Resolvers::NamespaceProjectsResolver do expect(field.to_graphql.complexity.call({}, { include_subgroups: true }, 1)).to eq 24 end - def resolve_projects(args = { include_subgroups: false }, context = { current_user: current_user }) + def resolve_projects(args = { include_subgroups: false, sort: nil, search: nil }, context = { current_user: current_user }) resolve(described_class, obj: namespace, args: args, ctx: context) end end diff --git a/spec/lib/gitlab/graphql/pagination/keyset/connection_spec.rb b/spec/lib/gitlab/graphql/pagination/keyset/connection_spec.rb index 09d7e084172..c8f368b15fc 100644 --- a/spec/lib/gitlab/graphql/pagination/keyset/connection_spec.rb +++ b/spec/lib/gitlab/graphql/pagination/keyset/connection_spec.rb @@ -262,6 +262,22 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::Connection do end end + context 'when ordering by similarity' do + let!(:project1) { create(:project, name: 'test') } + let!(:project2) { create(:project, name: 'testing') } + let!(:project3) { create(:project, name: 'tests') } + let!(:project4) { create(:project, name: 'testing stuff') } + let!(:project5) { create(:project, name: 'test') } + + let(:nodes) do + Project.sorted_by_similarity_desc('test', include_in_select: true) + end + + let(:descending_nodes) { nodes.to_a } + + it_behaves_like 'nodes are in descending order' + end + context 'when an invalid cursor is provided' do let(:arguments) { { before: Base64Bp.urlsafe_encode64('invalidcursor', padding: false) } } @@ -358,15 +374,6 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::Connection do end end - context 'when before and last does not request all remaining nodes' do - let(:arguments) { { before: encoded_cursor(project_list.last), last: 2 } } - - it 'has a previous and a next' do - expect(subject.has_previous_page).to be_truthy - expect(subject.has_next_page).to be_truthy - end - end - context 'when before and last does request all remaining nodes' do let(:arguments) { { before: encoded_cursor(project_list[1]), last: 3 } } diff --git a/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb b/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb index 9f310f30253..444c10074a0 100644 --- a/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb +++ b/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb @@ -51,6 +51,18 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::OrderInfo do expect(order_list.last.operator_for(:after)).to eq '>' end end + + context 'when ordering by SIMILARITY' do + let(:relation) { Project.sorted_by_similarity_desc('test', include_in_select: true) } + + it 'assigns the right attribute name, named function, and direction' do + expect(order_list.count).to eq 2 + expect(order_list.first.attribute_name).to eq 'similarity' + expect(order_list.first.named_function).to be_kind_of(Arel::Nodes::Addition) + expect(order_list.first.named_function.to_sql).to include 'SIMILARITY(' + expect(order_list.first.sort_direction).to eq :desc + end + end end describe '#validate_ordering' do diff --git a/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb b/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb index 31c02fd43e8..c7e7db4d535 100644 --- a/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb +++ b/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb @@ -131,5 +131,42 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::QueryBuilder do end end end + + context 'when sorting using SIMILARITY' do + let(:relation) { Project.sorted_by_similarity_desc('test', include_in_select: true) } + let(:arel_table) { Project.arel_table } + let(:decoded_cursor) { { 'similarity' => 0.5, 'id' => 100 } } + let(:similarity_sql) do + [ + '(SIMILARITY(COALESCE("projects"."path", \'\'), \'test\') * CAST(\'1\' AS numeric))', + '(SIMILARITY(COALESCE("projects"."name", \'\'), \'test\') * CAST(\'0.7\' AS numeric))', + '(SIMILARITY(COALESCE("projects"."description", \'\'), \'test\') * CAST(\'0.2\' AS numeric))' + ].join(' + ') + end + + context 'when no values are nil' do + context 'when :after' do + it 'generates the correct condition' do + conditions = builder.conditions.gsub(/\s+/, ' ') + + expect(conditions).to include "(#{similarity_sql} < 0.5)" + expect(conditions).to include '"projects"."id" < 100' + expect(conditions).to include "OR (#{similarity_sql} IS NULL)" + end + end + + context 'when :before' do + let(:before_or_after) { :before } + + it 'generates the correct condition' do + conditions = builder.conditions.gsub(/\s+/, ' ') + + expect(conditions).to include "(#{similarity_sql} > 0.5)" + expect(conditions).to include '"projects"."id" > 100' + expect(conditions).to include "OR ( #{similarity_sql} = 0.5" + end + end + end + end end end diff --git a/spec/requests/api/graphql/namespace/projects_spec.rb b/spec/requests/api/graphql/namespace/projects_spec.rb index 0b634e6b689..03160719389 100644 --- a/spec/requests/api/graphql/namespace/projects_spec.rb +++ b/spec/requests/api/graphql/namespace/projects_spec.rb @@ -78,4 +78,43 @@ RSpec.describe 'getting projects' do it_behaves_like 'a graphql namespace' end + + describe 'sorting and pagination' do + let(:data_path) { [:namespace, :projects] } + + def pagination_query(params, page_info) + graphql_query_for( + 'namespace', + { 'fullPath' => subject.full_path }, + <<~QUERY + projects(includeSubgroups: #{include_subgroups}, search: "#{search}", #{params}) { + #{page_info} edges { + node { + #{all_graphql_fields_for('Project')} + } + } + } + QUERY + ) + end + + def pagination_results_data(data) + data.map { |project| project.dig('node', 'name') } + end + + context 'when sorting by similarity' do + let!(:project_1) { create(:project, name: 'Project', path: 'project', namespace: subject) } + let!(:project_2) { create(:project, name: 'Test Project', path: 'test-project', namespace: subject) } + let!(:project_3) { create(:project, name: 'Test', path: 'test', namespace: subject) } + let!(:project_4) { create(:project, name: 'Test Project Other', path: 'other-test-project', namespace: subject) } + let(:search) { 'test' } + let(:current_user) { user } + + it_behaves_like 'sorted paginated query' do + let(:sort_param) { 'SIMILARITY' } + let(:first_param) { 2 } + let(:expected_results) { [project_3.name, project_2.name, project_4.name] } + end + end + end end diff --git a/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb b/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb index 2ef71d275a2..7627a7b4d59 100644 --- a/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb +++ b/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb @@ -84,6 +84,9 @@ RSpec.shared_examples 'sorted paginated query' do cursored_query = pagination_query([sort_argument, "after: \"#{end_cursor}\""].compact.join(','), page_info) post_graphql(cursored_query, current_user: current_user) + + expect(response).to have_gitlab_http_status(:ok) + response_data = graphql_dig_at(Gitlab::Json.parse(response.body), :data, *data_path, :edges) expect(pagination_results_data(response_data)).to eq expected_results.drop(first_param) -- cgit v1.2.3