Merge branch 'jp-descendant_search' into 'master'

Ignore full path matches when searching groups in REST API

See merge request gitlab-org/gitlab!66431

app/finders/groups_finder.rb

@@ -100,8 +100,7 @@ class GroupsFinder < UnionFinder
   def by_search(groups)
     return groups unless params[:search].present?
 
-    search_in_descendant_groups = params[:parent].present? && include_parent_descendants?
-    groups.search(params[:search], include_parents: !search_in_descendant_groups)
+    groups.search(params[:search], include_parents: !params[:parent].present?)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

doc/api/groups.md

@@ -131,7 +131,7 @@ Parameters:
 | `id`                     | integer/string    | yes      | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) of the immediate parent group |
 | `skip_groups`            | array of integers | no       | Skip the group IDs passed |
 | `all_available`          | boolean           | no       | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for administrators); Attributes `owned` and `min_access_level` have precedence |
-| `search`                 | string            | no       | Return the list of authorized groups matching the search criteria |
+| `search`                 | string            | no       | Return the list of authorized groups matching the search criteria. Only subgroup short paths are searched (not full paths) |
 | `order_by`               | string            | no       | Order groups by `name`, `path` or `id`. Default is `name` |
 | `sort`                   | string            | no       | Order groups in `asc` or `desc` order. Default is `asc` |
 | `statistics`             | boolean           | no       | Include group statistics (administrators only) |
@@ -189,7 +189,7 @@ Parameters:
 | `id`                     | integer/string    | yes      | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) of the immediate parent group |
 | `skip_groups`            | array of integers | no       | Skip the group IDs passed |
 | `all_available`          | boolean           | no       | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for administrators). Attributes `owned` and `min_access_level` have precedence |
-| `search`                 | string            | no       | Return the list of authorized groups matching the search criteria |
+| `search`                 | string            | no       | Return the list of authorized groups matching the search criteria. Only descendant group short paths are searched (not full paths) |
 | `order_by`               | string            | no       | Order groups by `name`, `path`, or `id`. Default is `name` |
 | `sort`                   | string            | no       | Order groups in `asc` or `desc` order. Default is `asc` |
 | `statistics`             | boolean           | no       | Include group statistics (administrators only) |

lib/api/groups.rb

@@ -35,7 +35,8 @@ module API
           :all_available,
           :custom_attributes,
           :owned, :min_access_level,
-          :include_parent_descendants
+          :include_parent_descendants,
+          :search
         )
 
         find_params[:parent] = if params[:top_level_only]
@@ -48,7 +49,6 @@ module API
           find_params.fetch(:all_available, current_user&.can_read_all_resources?)
 
         groups = GroupsFinder.new(current_user, find_params).execute
-        groups = groups.search(params[:search], include_parents: true) if params[:search].present?
         groups = groups.where.not(id: params[:skip_groups]) if params[:skip_groups].present?
 
         order_groups(groups)

spec/finders/groups_finder_spec.rb

@@ -228,13 +228,6 @@ RSpec.describe GroupsFinder do
           )
         end
       end
-
-      context 'with search' do
-        it 'does not search in full path' do
-          params[:search] = public_subgroup.path
-          expect(described_class.new(user, params).execute).to contain_exactly(public_subgroup)
-        end
-      end
     end
 
     context 'with search' do
@@ -249,6 +242,10 @@ RSpec.describe GroupsFinder do
         expect(described_class.new(user, { search: 'test' }).execute).to contain_exactly(test_group)
       end
 
+      it 'does not search in full path if parent is set' do
+        expect(described_class.new(user, { search: 'parent', parent: parent_group }).execute).to be_empty
+      end
+
       context 'with group descendants' do
         let_it_be(:sub_group) { create(:group, :public, name: 'Sub Group', parent: parent_group) }
 

spec/requests/api/groups_spec.rb

@@ -63,6 +63,19 @@ RSpec.describe API::Groups do
     end
   end
 
+  shared_examples 'skips searching in full path' do
+    it 'does not find groups by full path' do
+      subgroup = create(:group, parent: parent, path: "#{parent.path}-subgroup")
+      create(:group, parent: parent, path: 'not_matching_path')
+
+      get endpoint, params: { search: parent.path }
+
+      expect(json_response).to be_an Array
+      expect(json_response.length).to eq(1)
+      expect(json_response.first['id']).to eq(subgroup.id)
+    end
+  end
+
   describe "GET /groups" do
     context "when unauthenticated" do
       it "returns public groups" do
@@ -406,6 +419,22 @@ RSpec.describe API::Groups do
         expect(response_groups).to contain_exactly(group2.id, group3.id)
       end
     end
+
+    context 'when searching' do
+      let_it_be(:subgroup1) { create(:group, parent: group1, path: 'some_path') }
+
+      let(:response_groups) { json_response.map { |group| group['id'] } }
+
+      subject { get api('/groups', user1), params: { search: group1.path } }
+
+      it 'finds also groups with full path matching search param' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to be_an Array
+        expect(response_groups).to match_array([group1.id, subgroup1.id])
+      end
+    end
   end
 
   describe "GET /groups/:id" do
@@ -1424,6 +1453,11 @@ RSpec.describe API::Groups do
         expect(json_response.first).to include('statistics')
       end
     end
+
+    it_behaves_like 'skips searching in full path' do
+      let(:parent) { group1 }
+      let(:endpoint) { api("/groups/#{group1.id}/subgroups", user1) }
+    end
   end
 
   describe 'GET /groups/:id/descendant_groups' do
@@ -1558,6 +1592,11 @@ RSpec.describe API::Groups do
         expect(json_response.first).to include('statistics')
       end
     end
+
+    it_behaves_like 'skips searching in full path' do
+      let(:parent) { group1 }
+      let(:endpoint) { api("/groups/#{group1.id}/descendant_groups", user1) }
+    end
   end
 
   describe "POST /groups" do