Commit e31c1bfa authored by Patrick Derichs's avatar Patrick Derichs

Add possibility to search for partial iids on EpicResolver

Also add specs and update GraphQL schema

Make EpicsFinder able to search for epics which
start with a partial iid

Constructs a combined where clause which includes all possible
combinations for the given iid start string which would take advantage
of the existing index on epics table.

Use LIKE query for iid search with specialized index

Add index for searching epics by iid and group_id

Remove obsolete scopes and specs

Change changelog entry
parent c30132ac
# frozen_string_literal: true
class AddIndexForGroupAndIidSearchToEpics < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
INDEX_NAME = 'index_epics_on_group_id_and_iid_varchar_pattern'
disable_ddl_transaction!
def up
disable_statement_timeout do
execute "CREATE INDEX CONCURRENTLY \"#{INDEX_NAME}\" ON epics (group_id, CAST(iid AS VARCHAR) varchar_pattern_ops);"
end
end
def down
disable_statement_timeout do
remove_concurrent_index_by_name :epics, INDEX_NAME
end
end
end
...@@ -1581,6 +1581,7 @@ ActiveRecord::Schema.define(version: 2020_02_27_165129) do ...@@ -1581,6 +1581,7 @@ ActiveRecord::Schema.define(version: 2020_02_27_165129) do
t.integer "start_date_sourcing_epic_id" t.integer "start_date_sourcing_epic_id"
t.integer "due_date_sourcing_epic_id" t.integer "due_date_sourcing_epic_id"
t.integer "health_status", limit: 2 t.integer "health_status", limit: 2
t.index "group_id, ((iid)::character varying) varchar_pattern_ops", name: "index_epics_on_group_id_and_iid_varchar_pattern"
t.index ["assignee_id"], name: "index_epics_on_assignee_id" t.index ["assignee_id"], name: "index_epics_on_assignee_id"
t.index ["author_id"], name: "index_epics_on_author_id" t.index ["author_id"], name: "index_epics_on_author_id"
t.index ["closed_by_id"], name: "index_epics_on_closed_by_id" t.index ["closed_by_id"], name: "index_epics_on_closed_by_id"
......
...@@ -1890,6 +1890,11 @@ type Epic implements Noteable { ...@@ -1890,6 +1890,11 @@ type Epic implements Noteable {
""" """
iid: ID iid: ID
"""
Filter epics by iid for autocomplete
"""
iidStartsWith: String
""" """
List of IIDs of epics, e.g., [1, 2] List of IIDs of epics, e.g., [1, 2]
""" """
...@@ -2941,6 +2946,11 @@ type Group { ...@@ -2941,6 +2946,11 @@ type Group {
""" """
iid: ID iid: ID
"""
Filter epics by iid for autocomplete
"""
iidStartsWith: String
""" """
List of IIDs of epics, e.g., [1, 2] List of IIDs of epics, e.g., [1, 2]
""" """
...@@ -3008,6 +3018,11 @@ type Group { ...@@ -3008,6 +3018,11 @@ type Group {
""" """
iid: ID iid: ID
"""
Filter epics by iid for autocomplete
"""
iidStartsWith: String
""" """
List of IIDs of epics, e.g., [1, 2] List of IIDs of epics, e.g., [1, 2]
""" """
......
...@@ -3513,6 +3513,16 @@ ...@@ -3513,6 +3513,16 @@
} }
}, },
"defaultValue": null "defaultValue": null
},
{
"name": "iidStartsWith",
"description": "Filter epics by iid for autocomplete",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": null
} }
], ],
"type": { "type": {
...@@ -3633,6 +3643,16 @@ ...@@ -3633,6 +3643,16 @@
}, },
"defaultValue": null "defaultValue": null
}, },
{
"name": "iidStartsWith",
"description": "Filter epics by iid for autocomplete",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": null
},
{ {
"name": "after", "name": "after",
"description": "Returns the elements in the list that come after the specified cursor.", "description": "Returns the elements in the list that come after the specified cursor.",
...@@ -4866,6 +4886,16 @@ ...@@ -4866,6 +4886,16 @@
}, },
"defaultValue": null "defaultValue": null
}, },
{
"name": "iidStartsWith",
"description": "Filter epics by iid for autocomplete",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": null
},
{ {
"name": "after", "name": "after",
"description": "Returns the elements in the list that come after the specified cursor.", "description": "Returns the elements in the list that come after the specified cursor.",
......
...@@ -18,10 +18,13 @@ ...@@ -18,10 +18,13 @@
# updated_before: datetime # updated_before: datetime
# include_ancestor_groups: boolean # include_ancestor_groups: boolean
# include_descendant_groups: boolean # include_descendant_groups: boolean
# starts_with_iid: string (containing a number)
class EpicsFinder < IssuableFinder class EpicsFinder < IssuableFinder
include TimeFrameFilter include TimeFrameFilter
IID_STARTS_WITH_PATTERN = %r{\A(\d)+\z}.freeze
def self.scalar_params def self.scalar_params
@scalar_params ||= %i[ @scalar_params ||= %i[
parent_id parent_id
...@@ -38,6 +41,10 @@ class EpicsFinder < IssuableFinder ...@@ -38,6 +41,10 @@ class EpicsFinder < IssuableFinder
@array_params ||= { label_name: [] } @array_params ||= { label_name: [] }
end end
def self.valid_iid_query?(query)
query.match?(IID_STARTS_WITH_PATTERN)
end
def klass def klass
Epic Epic
end end
...@@ -55,6 +62,7 @@ class EpicsFinder < IssuableFinder ...@@ -55,6 +62,7 @@ class EpicsFinder < IssuableFinder
items = by_label(items) items = by_label(items)
items = by_parent(items) items = by_parent(items)
items = by_iids(items) items = by_iids(items)
items = starts_with_iid(items)
sort(items) sort(items)
end end
...@@ -91,6 +99,15 @@ class EpicsFinder < IssuableFinder ...@@ -91,6 +99,15 @@ class EpicsFinder < IssuableFinder
private private
def starts_with_iid(items)
return items unless params[:iid_starts_with].present?
query = params[:iid_starts_with]
raise ArgumentError unless self.class.valid_iid_query?(query)
items.iid_starts_with(query)
end
def related_groups def related_groups
include_ancestors = params.fetch(:include_ancestor_groups, false) include_ancestors = params.fetch(:include_ancestor_groups, false)
include_descendants = params.fetch(:include_descendant_groups, true) include_descendants = params.fetch(:include_descendant_groups, true)
......
...@@ -32,6 +32,10 @@ module Resolvers ...@@ -32,6 +32,10 @@ module Resolvers
required: false, required: false,
description: 'Filter epics by labels' description: 'Filter epics by labels'
argument :iid_starts_with, GraphQL::STRING_TYPE,
required: false,
description: 'Filter epics by iid for autocomplete'
type Types::EpicType, null: true type Types::EpicType, null: true
def resolve(**args) def resolve(**args)
...@@ -41,6 +45,7 @@ module Resolvers ...@@ -41,6 +45,7 @@ module Resolvers
return [] unless epic_feature_enabled? return [] unless epic_feature_enabled?
validate_timeframe_params!(args) validate_timeframe_params!(args)
validate_iid_starts_with_query!(args)
find_epics(transform_args(args)) find_epics(transform_args(args))
end end
...@@ -81,6 +86,16 @@ module Resolvers ...@@ -81,6 +86,16 @@ module Resolvers
parent.group parent.group
end end
def validate_iid_starts_with_query!(args)
return unless args[:iid_starts_with].present?
iid_starts_with_query = args[:iid_starts_with]
unless EpicsFinder.valid_iid_query?(iid_starts_with_query)
raise Gitlab::Graphql::Errors::ArgumentError, "#{iid_starts_with_query} is not a valid iid search term"
end
end
# If we're querying for multiple iids and selecting issues, then ideally # If we're querying for multiple iids and selecting issues, then ideally
# we want to batch the epic and issue queries into one to reduce N+1 and memory. # we want to batch the epic and issue queries into one to reduce N+1 and memory.
# https://gitlab.com/gitlab-org/gitlab/issues/11841 # https://gitlab.com/gitlab-org/gitlab/issues/11841
......
...@@ -66,6 +66,7 @@ module EE ...@@ -66,6 +66,7 @@ module EE
scope :in_milestone, -> (milestone_id) { joins(:issues).where(issues: { milestone_id: milestone_id }) } scope :in_milestone, -> (milestone_id) { joins(:issues).where(issues: { milestone_id: milestone_id }) }
scope :in_issues, -> (issues) { joins(:epic_issues).where(epic_issues: { issue_id: issues }).distinct } scope :in_issues, -> (issues) { joins(:epic_issues).where(epic_issues: { issue_id: issues }).distinct }
scope :has_parent, -> { where.not(parent_id: nil) } scope :has_parent, -> { where.not(parent_id: nil) }
scope :iid_starts_with, -> (query) { where("CAST(iid AS VARCHAR) LIKE ?", "#{sanitize_sql_like(query)}%") }
scope :within_timeframe, -> (start_date, end_date) do scope :within_timeframe, -> (start_date, end_date) do
where('start_date is not NULL or end_date is not NULL') where('start_date is not NULL or end_date is not NULL')
......
---
title: Add possibility to search for epics by partial iids using GraphQL
merge_request: 24673
author:
type: added
...@@ -235,7 +235,62 @@ describe EpicsFinder do ...@@ -235,7 +235,62 @@ describe EpicsFinder do
expect(epics(params)).to contain_exactly(epic1) expect(epics(params)).to contain_exactly(epic1)
end end
end end
context 'when using iid starts with query' do
let!(:epic1) { create(:epic, :opened, group: group, iid: '11') }
let!(:epic2) { create(:epic, :opened, group: group, iid: '1112') }
let!(:epic3) { create(:epic, :closed, group: group, iid: '9978') }
let!(:epic4) { create(:epic, :closed, group: another_group, iid: '111') }
it 'returns the expected epics if just the first two numbers are given' do
params = { iid_starts_with: '11' }
expect(epics(params)).to contain_exactly(epic1, epic2)
end
it 'returns the expected epics if the exact id is given' do
params = { iid_starts_with: '1112' }
expect(epics(params)).to contain_exactly(epic2)
end
it 'is empty if the last number is given' do
params = { iid_starts_with: '8' }
expect(epics(params)).to be_empty
end
it 'fails if iid_starts_with contains a non-numeric string' do
expect { epics({ iid_starts_with: 'foo' }) }.to raise_error(ArgumentError)
end end
it 'fails if iid_starts_with contains a non-numeric string with line breaks' do
expect { epics({ iid_starts_with: "foo\n1" }) }.to raise_error(ArgumentError)
end
it 'fails if iid_starts_with contains a string which contains a negative number' do
expect { epics(iid_starts_with: '-1') }.to raise_error(ArgumentError)
end
end
end
end
end
describe '.valid_iid_query?' do
using RSpec::Parameterized::TableSyntax
where(:query, :expected_result) do
"foo" | false
"-1" | false
"1\nfoo" | false
"foo\n1" | false
"1" | true
end
with_them do
subject { described_class.valid_iid_query?(query) }
it { is_expected.to eq(expected_result) }
end end
end end
......
...@@ -194,6 +194,51 @@ describe Resolvers::EpicResolver do ...@@ -194,6 +194,51 @@ describe Resolvers::EpicResolver do
expect(resolve_epics).to contain_exactly(epic1, epic2, epic3, epic4) expect(resolve_epics).to contain_exactly(epic1, epic2, epic3, epic4)
end end
end end
context 'with partial iids' do
let!(:other_group) { create(:group, :private) }
let!(:epic3) { create(:epic, group: group, iid: '1122') }
let!(:epic4) { create(:epic, group: group, iid: '132') }
let!(:epic5) { create(:epic, group: group, iid: '62') }
let!(:epic6) { create(:epic, group: other_group, iid: '11999') }
it 'returns the expected epics if just the first number of iid is requested' do
epics = resolve_epics(iid_starts_with: '1')
expect(epics).to contain_exactly(epic3, epic4)
end
it 'returns the expected epics if first two numbers of iid are requested' do
epics = resolve_epics(iid_starts_with: '11')
expect(epics).to contain_exactly(epic3)
end
it 'returns the expected epics if last two numbers of iid are given' do
epics = resolve_epics(iid_starts_with: '32')
expect(epics).to be_empty
end
it 'returns the expected epics if exact number of iid is given' do
epics = resolve_epics(iid_starts_with: '62')
expect(epics).to contain_exactly(epic5)
end
it 'fails if iid_starts_with contains a non-numeric string' do
expect { resolve_epics(iid_starts_with: 'foo') }.to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'foo is not a valid iid search term')
end
it 'fails if iid_starts_with contains a non-numeric string with line breaks' do
expect { resolve_epics(iid_starts_with: "foo\n1") }.to raise_error(Gitlab::Graphql::Errors::ArgumentError, "foo\n1 is not a valid iid search term")
end
it 'fails if iid_starts_with contains a string which contains a negative number' do
expect { resolve_epics(iid_starts_with: '-1') }.to raise_error(Gitlab::Graphql::Errors::ArgumentError, '-1 is not a valid iid search term')
end
end
end end
end end
......
...@@ -56,8 +56,6 @@ describe Issuable do ...@@ -56,8 +56,6 @@ describe Issuable do
end end
describe "Scope" do describe "Scope" do
subject { build(:issue) }
it { expect(issuable_class).to respond_to(:opened) } it { expect(issuable_class).to respond_to(:opened) }
it { expect(issuable_class).to respond_to(:closed) } it { expect(issuable_class).to respond_to(:closed) }
it { expect(issuable_class).to respond_to(:assigned) } it { expect(issuable_class).to respond_to(:assigned) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment