From 643f60e667ad8394311f95eb9991209f4552a85b Mon Sep 17 00:00:00 2001 From: gabemontero Date: Mon, 8 Dec 2025 15:14:32 -0500 Subject: [PATCH] benjdlambert-1 review changes Signed-off-by: gabemontero --- ...reateRegisterCatalogEntitiesAction.test.ts | 56 +++++++++---------- .../createRegisterCatalogEntitiesAction.ts | 34 ++++------- ...ateUnregisterCatalogEntitiesAction.test.ts | 37 ++++++------ .../createUnregisterCatalogEntitiesAction.ts | 33 +++-------- 4 files changed, 59 insertions(+), 101 deletions(-) diff --git a/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts b/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts index 0c6d35b297..a85456ff42 100644 --- a/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts +++ b/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.test.ts @@ -16,6 +16,7 @@ import { createRegisterCatalogEntitiesAction } from './createRegisterCatalogEntitiesAction'; import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils'; import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha'; +import { ForwardedError } from '@backstage/errors'; describe('createRegisterCatalogEntitiesAction', () => { it('should successfully register a catalog location with a valid URL', async () => { @@ -48,7 +49,6 @@ describe('createRegisterCatalogEntitiesAction', () => { expect(result.output).toEqual({ locationID: mockLocationId, - error: undefined, }); expect(mockCatalog.addLocation).toHaveBeenCalledWith( { @@ -61,7 +61,7 @@ describe('createRegisterCatalogEntitiesAction', () => { ); }); - it('should return an error if locationURL is not provided', async () => { + it('should throw an error if locationURL is not provided', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); @@ -70,17 +70,15 @@ describe('createRegisterCatalogEntitiesAction', () => { actionsRegistry: mockActionsRegistry, }); - const result = await mockActionsRegistry.invoke({ - id: 'test:register-catalog-entities', - input: { locationURL: '' }, - }); - - expect(result.output).toEqual({ - error: 'a location URL must be specified', - }); + await expect( + mockActionsRegistry.invoke({ + id: 'test:register-catalog-entities', + input: { locationURL: '' }, + }), + ).rejects.toThrow('a location URL must be specified'); }); - it('should return an error if locationURL is not a valid URL', async () => { + it('should throw an error if locationURL is not a valid URL', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); @@ -89,17 +87,15 @@ describe('createRegisterCatalogEntitiesAction', () => { actionsRegistry: mockActionsRegistry, }); - const result = await mockActionsRegistry.invoke({ - id: 'test:register-catalog-entities', - input: { locationURL: 'not-a-valid-url' }, - }); - - expect(result.output).toEqual({ - error: 'location URL must be a valid URL string', - }); + await expect( + mockActionsRegistry.invoke({ + id: 'test:register-catalog-entities', + input: { locationURL: 'not-a-valid-url' }, + }), + ).rejects.toThrow('locationURL "not-a-valid-url" an invalid URL'); }); - it('should return an error if catalog.addLocation throws an error', async () => { + it('should throw a ForwardedError if catalog.addLocation throws an error', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); @@ -113,16 +109,14 @@ describe('createRegisterCatalogEntitiesAction', () => { actionsRegistry: mockActionsRegistry, }); - const result = await mockActionsRegistry.invoke({ - id: 'test:register-catalog-entities', - input: { - locationURL: - 'https://github.com/example/repo/blob/main/catalog-info.yaml', - }, - }); - - expect(result.output).toEqual({ - error: errorMessage, - }); + await expect( + mockActionsRegistry.invoke({ + id: 'test:register-catalog-entities', + input: { + locationURL: + 'https://github.com/example/repo/blob/main/catalog-info.yaml', + }, + }), + ).rejects.toThrow(ForwardedError); }); }); diff --git a/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.ts b/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.ts index 167a1bd66a..e2a8fb2984 100644 --- a/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.ts +++ b/plugins/catalog-backend/src/actions/createRegisterCatalogEntitiesAction.ts @@ -15,6 +15,7 @@ */ import { ActionsRegistryService } from '@backstage/backend-plugin-api/alpha'; import { CatalogService } from '@backstage/plugin-catalog-node'; +import { InputError, ForwardedError } from '@backstage/errors'; export const createRegisterCatalogEntitiesAction = ({ catalog, @@ -45,10 +46,7 @@ The dynamically generated, random UUID created to go along with the newly create to the caller. That UUID can be later used if you want to unregister or delete the Location entity and all of its children entities. -If an error occurred during processing, the 'locationID' field will be blank, and the 'error' field will contain -a message describing the error. - -Example invocation and the output from the invocation: +/eExample invocation and the output from the invocation: # Register a Location from a GitHub URL register-catalog-entities locationURL: https://github.com/redhat-ai-dev/model-catalog-example/blob/main/developer-model-service/catalog-info.yaml Output: { @@ -70,19 +68,11 @@ Example invocation and the output from the invocation: .string() .optional() .describe('The location ID generated by the registration'), - error: z - .string() - .optional() - .describe('Error message if creation fails'), }), }, action: async ({ input, credentials }) => { if (!input.locationURL || input.locationURL === '') { - return { - output: { - error: 'a location URL must be specified', - }, - }; + throw new InputError('a location URL must be specified'); } // Validate that the locationURL is a valid URL @@ -90,11 +80,9 @@ Example invocation and the output from the invocation: // eslint-disable-next-line no-new new URL(input.locationURL); } catch { - return { - output: { - error: 'location URL must be a valid URL string', - }, - }; + throw new InputError( + `locationURL "${input.locationURL}" an invalid URL`, + ); } try { const locReq = { @@ -108,15 +96,13 @@ Example invocation and the output from the invocation: return { output: { locationID: result.location.id, - error: undefined, }, }; } catch (error) { - return { - output: { - error: error.message, - }, - }; + throw new ForwardedError( + `catalog import of "${input.locationURL} failed"`, + error, + ); } }, }); diff --git a/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts b/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts index abf03a2f36..225497c315 100644 --- a/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts +++ b/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.test.ts @@ -16,6 +16,7 @@ import { createUnregisterCatalogEntitiesAction } from './createUnregisterCatalogEntitiesAction'; import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils'; import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha'; +import { ForwardedError } from '@backstage/errors'; describe('createUnregisterCatalogEntitiesAction', () => { it('should successfully unregister a catalog location with a valid locationId', async () => { @@ -34,9 +35,7 @@ describe('createUnregisterCatalogEntitiesAction', () => { input: { locationId: 'test-location-id-1234' }, }); - expect(result.output).toEqual({ - error: undefined, - }); + expect(result.output).toEqual({}); expect(mockCatalog.removeLocationById).toHaveBeenCalledWith( 'test-location-id-1234', expect.objectContaining({ @@ -45,7 +44,7 @@ describe('createUnregisterCatalogEntitiesAction', () => { ); }); - it('should return an error if locationId is not provided', async () => { + it('should throw an error if locationId is not provided', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); @@ -54,17 +53,15 @@ describe('createUnregisterCatalogEntitiesAction', () => { actionsRegistry: mockActionsRegistry, }); - const result = await mockActionsRegistry.invoke({ - id: 'test:unregister-catalog-entities', - input: { locationId: '' }, - }); - - expect(result.output).toEqual({ - error: 'a location ID must be specified', - }); + await expect( + mockActionsRegistry.invoke({ + id: 'test:unregister-catalog-entities', + input: { locationId: '' }, + }), + ).rejects.toThrow('a locationID must be specified'); }); - it('should return an error if catalog.removeLocationById throws an error', async () => { + it('should throw a ForwardedError if catalog.removeLocationById throws an error', async () => { const mockActionsRegistry = actionsRegistryServiceMock(); const mockCatalog = catalogServiceMock(); @@ -78,13 +75,11 @@ describe('createUnregisterCatalogEntitiesAction', () => { actionsRegistry: mockActionsRegistry, }); - const result = await mockActionsRegistry.invoke({ - id: 'test:unregister-catalog-entities', - input: { locationId: 'test-location-id-1234' }, - }); - - expect(result.output).toEqual({ - error: errorMessage, - }); + await expect( + mockActionsRegistry.invoke({ + id: 'test:unregister-catalog-entities', + input: { locationId: 'test-location-id-1234' }, + }), + ).rejects.toThrow(ForwardedError); }); }); diff --git a/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.ts b/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.ts index 2164e15f5e..a7ed9b6357 100644 --- a/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.ts +++ b/plugins/catalog-backend/src/actions/createUnregisterCatalogEntitiesAction.ts @@ -15,6 +15,7 @@ */ import { ActionsRegistryService } from '@backstage/backend-plugin-api/alpha'; import { CatalogService } from '@backstage/plugin-catalog-node'; +import { ForwardedError, InputError } from '@backstage/errors'; export const createUnregisterCatalogEntitiesAction = ({ catalog, @@ -38,8 +39,6 @@ where you supply the UUID for a Location entity link that allows for the removal the various Entities (Components, Systems, Resources, APIs, Users, and Groups) that were also created when the Location was imported. -If an error occurred during processing, the 'error' field will contain a message describing the error. - Example invocation and the output from the invocation: # Register a Location from a GitHub URL unregister-catalog-entities locationId: aaa-bbb-ccc-ddd @@ -54,40 +53,24 @@ Example invocation and the output from the invocation: `Location ID returned from the 'register-catalog-entities' call.`, ), }), - output: z => - z.object({ - error: z - .string() - .optional() - .describe('Error message if creation fails'), - }), + output: z => z.object({}), }, action: async ({ input, credentials }) => { if (!input.locationId || input.locationId === '') { - return { - output: { - error: 'a location ID must be specified', - }, - }; + throw new InputError('a locationID must be specified'); } try { await catalog.removeLocationById(input.locationId, { credentials, }); - - return { - output: { - error: undefined, - }, - }; } catch (error) { - return { - output: { - error: error.message, - }, - }; + throw new ForwardedError( + `catalog removal of "${input.locationId} failed"`, + error, + ); } + return { output: {} }; }, }); };