fix(scaffolder-gitlab): Use Groups.show instead of search to check path existence

The search API returns multiple results requiring filtering, while show
directly returns the group by path or throws a 404 if not found. This is
more efficient and accurate for checking if a specific path exists.

Signed-off-by: Jellyfrog <Jellyfrog@users.noreply.github.com>

.changeset/easy-deer-eat.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-scaffolder-backend-module-gitlab': patch
+---
+
+Changed `gitlab:group:ensureExists` action to use `Groups.show` API instead of `Groups.search` for checking if a group path exists. This is more efficient as it directly retrieves the group by path rather than searching and filtering results.

plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.examples.test.ts

@@ -23,7 +23,7 @@ import { mockServices } from '@backstage/backend-test-utils';
 
 const mockGitlabClient = {
   Groups: {
-    search: jest.fn(),
+    show: jest.fn(),
     create: jest.fn(),
   },
 };
@@ -44,7 +44,9 @@ describe('gitlab:group:ensureExists', () => {
   });
 
   it(`Should ${examples[0].description}`, async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([]);
+    mockGitlabClient.Groups.show.mockRejectedValue({
+      cause: { response: { status: 404 } },
+    });
     mockGitlabClient.Groups.create.mockResolvedValue({
       id: 3,
       full_path: 'group1',
@@ -82,12 +84,9 @@ describe('gitlab:group:ensureExists', () => {
   });
 
   it(`Should ${examples[1].description}`, async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'group1',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 1, full_path: 'group1' })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } });
     mockGitlabClient.Groups.create.mockResolvedValue({
       id: 3,
       full_path: 'group1/group2',
@@ -127,16 +126,10 @@ describe('gitlab:group:ensureExists', () => {
   });
 
   it(`Should ${examples[2].description}`, async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'group1',
-      },
-      {
-        id: 2,
-        full_path: 'group1/group2',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 1, full_path: 'group1' })
+      .mockResolvedValueOnce({ id: 2, full_path: 'group1/group2' })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } });
     mockGitlabClient.Groups.create.mockResolvedValue({
       id: 3,
       full_path: 'group1/group2/group3',
@@ -199,23 +192,17 @@ describe('gitlab:group:ensureExists', () => {
       input: yaml.parse(examples[3].example).steps[0].input,
     });
 
-    expect(mockGitlabClient.Groups.search).not.toHaveBeenCalled();
+    expect(mockGitlabClient.Groups.show).not.toHaveBeenCalled();
     expect(mockGitlabClient.Groups.create).not.toHaveBeenCalled();
 
     expect(mockContext.output).toHaveBeenCalledWith('groupId', 42);
   });
 
   it(`Should ${examples[4].description}`, async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'group1',
-      },
-      {
-        id: 2,
-        full_path: 'group1/group2',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 1, full_path: 'group1' })
+      .mockResolvedValueOnce({ id: 2, full_path: 'group1/group2' })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } });
     mockGitlabClient.Groups.create.mockResolvedValue({
       id: 3,
       full_path: 'group1/group2/group3',
@@ -255,20 +242,11 @@ describe('gitlab:group:ensureExists', () => {
   });
 
   it(`Should ${examples[5].description}`, async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'group1',
-      },
-      {
-        id: 2,
-        full_path: 'group1/group2',
-      },
-      {
-        id: 3,
-        full_path: 'group1/group2/group3',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 1, full_path: 'group1' })
+      .mockResolvedValueOnce({ id: 2, full_path: 'group1/group2' })
+      .mockResolvedValueOnce({ id: 3, full_path: 'group1/group2/group3' })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } });
     mockGitlabClient.Groups.create.mockResolvedValue({
       id: 4,
       full_path: 'group1/group2/group3/group4',

plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts

@@ -22,7 +22,7 @@ import { mockServices } from '@backstage/backend-test-utils';
 
 const mockGitlabClient = {
   Groups: {
-    search: jest.fn(),
+    show: jest.fn(),
     create: jest.fn(),
   },
 };
@@ -65,16 +65,9 @@ describe('gitlab:group:ensureExists', () => {
   const mockContext = createMockActionContext();
 
   it('should create a new group from string if it does not exists', async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'bar',
-      },
-      {
-        id: 2,
-        full_path: 'foo',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 2, full_path: 'foo' })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } });
 
     mockGitlabClient.Groups.create.mockResolvedValue({
       id: 3,
@@ -97,21 +90,14 @@ describe('gitlab:group:ensureExists', () => {
   });
 
   it('should create a new group from pathstring if it does not exists', async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'bar',
-      },
-      {
-        id: 2,
-        full_path: 'foo',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 2, full_path: 'foo' })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } });
 
-    mockGitlabClient.Groups.create.mockResolvedValue({
-      id: 3,
-      full_path: 'foo/bar',
-    });
+    mockGitlabClient.Groups.create
+      .mockResolvedValueOnce({ id: 3, full_path: 'foo/bar' })
+      .mockResolvedValueOnce({ id: 4, full_path: 'foo/bar/baz' });
 
     await action.handler({
       ...mockContext,
@@ -121,24 +107,31 @@ describe('gitlab:group:ensureExists', () => {
       },
     });
 
-    expect(mockGitlabClient.Groups.create).toHaveBeenCalledWith('bar', 'bar', {
-      parentId: 2,
-    });
+    expect(mockGitlabClient.Groups.create).toHaveBeenNthCalledWith(
+      1,
+      'bar',
+      'bar',
+      {
+        parentId: 2,
+      },
+    );
 
-    expect(mockContext.output).toHaveBeenCalledWith('groupId', 3);
+    expect(mockGitlabClient.Groups.create).toHaveBeenNthCalledWith(
+      2,
+      'baz',
+      'baz',
+      {
+        parentId: 3,
+      },
+    );
+
+    expect(mockContext.output).toHaveBeenCalledWith('groupId', 4);
   });
 
   it('should create a new group from object if it does not exists', async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'bar',
-      },
-      {
-        id: 2,
-        full_path: 'foo',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 2, full_path: 'foo' })
+      .mockRejectedValueOnce({ cause: { response: { status: 404 } } });
 
     mockGitlabClient.Groups.create.mockResolvedValue({
       id: 3,
@@ -168,20 +161,9 @@ describe('gitlab:group:ensureExists', () => {
   });
 
   it('should return existing group if it does exists', async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'bar',
-      },
-      {
-        id: 2,
-        full_path: 'foo',
-      },
-      {
-        id: 42,
-        full_path: 'foo/bar',
-      },
-    ]);
+    mockGitlabClient.Groups.show
+      .mockResolvedValueOnce({ id: 2, full_path: 'foo' })
+      .mockResolvedValueOnce({ id: 42, full_path: 'foo/bar' });
 
     await action.handler({
       ...mockContext,
@@ -206,19 +188,17 @@ describe('gitlab:group:ensureExists', () => {
       },
     });
 
-    expect(mockGitlabClient.Groups.search).not.toHaveBeenCalled();
+    expect(mockGitlabClient.Groups.show).not.toHaveBeenCalled();
     expect(mockGitlabClient.Groups.create).not.toHaveBeenCalled();
 
     expect(mockContext.output).toHaveBeenCalledWith('groupId', 42);
   });
 
   it('should use the token from the integration config when none is provided', async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'foobar',
-      },
-    ]);
+    mockGitlabClient.Groups.show.mockResolvedValue({
+      id: 1,
+      full_path: 'foobar',
+    });
 
     await action.handler({
       ...mockContext,
@@ -236,12 +216,10 @@ describe('gitlab:group:ensureExists', () => {
   });
 
   it('should use a provided token as bearer authentication', async () => {
-    mockGitlabClient.Groups.search.mockResolvedValue([
-      {
-        id: 1,
-        full_path: 'foobar',
-      },
-    ]);
+    mockGitlabClient.Groups.show.mockResolvedValue({
+      id: 1,
+      full_path: 'foobar',
+    });
 
     await action.handler({
       ...mockContext,

plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts

@@ -88,12 +88,18 @@ export const createGitlabGroupEnsureExistsAction = (options: {
       let parentId: number | null = null;
       for (const { name, slug } of pathIterator(path)) {
         const fullPath: string = currentPath ? `${currentPath}/${slug}` : slug;
-        const result = (await api.Groups.search(
-          fullPath,
-        )) as unknown as Array<GroupSchema>; // recast since the return type for search is wrong in the gitbeaker typings
-        const subGroup = result.find(
-          searchPathElem => searchPathElem.full_path === fullPath,
-        );
+        let subGroup: GroupSchema | undefined;
+        try {
+          subGroup = (await api.Groups.show(
+            fullPath,
+          )) as unknown as GroupSchema;
+        } catch (error: any) {
+          // 404 means the group doesn't exist - we'll create it below
+          // Any other error should be thrown
+          if (error.cause?.response?.status !== 404) {
+            throw error;
+          }
+        }
         if (!subGroup) {
           ctx.logger.info(`creating missing group ${fullPath}`);