catalog-backend: add pre-validation and consistent errors, refactor for simplicity

Signed-off-by: Bret Hubbard <hubbard.bret@gmail.com>

plugins/catalog-backend/src/service/createRouter.test.ts

@@ -19,8 +19,8 @@ import { ConfigReader } from '@backstage/config';
 import { NotFoundError } from '@backstage/errors';
 import type { Location } from '@backstage/catalog-client';
 import {
+  ANNOTATION_LOCATION,
   ANNOTATION_ORIGIN_LOCATION,
-  ANNOTATION_SOURCE_LOCATION,
   Entity,
 } from '@backstage/catalog-model';
 import express from 'express';
@@ -415,7 +415,7 @@ describe('createRouter readonly disabled', () => {
 
         const response = await request(app)
           .post('/validate-entity')
-          .send({ entity, location: 'validate:entity' });
+          .send({ entity, location: 'url:validate-entity' });
 
         expect(response.status).toEqual(200);
         expect(orchestrator.process).toHaveBeenCalledTimes(1);
@@ -426,8 +426,8 @@ describe('createRouter readonly disabled', () => {
             metadata: {
               name: 'n',
               annotations: {
-                [ANNOTATION_SOURCE_LOCATION]: 'validate:entity',
-                [ANNOTATION_ORIGIN_LOCATION]: 'validate:entity',
+                [ANNOTATION_LOCATION]: 'url:validate-entity',
+                [ANNOTATION_ORIGIN_LOCATION]: 'url:validate-entity',
               },
             },
           },
@@ -450,10 +450,11 @@ describe('createRouter readonly disabled', () => {
 
         const response = await request(app)
           .post('/validate-entity')
-          .send({ entity, location: 'validate:entity' });
+          .send({ entity, location: 'url:validate-entity' });
 
         expect(response.status).toEqual(400);
-        expect(response.body.error.message).toContain('Invalid entity name');
+        expect(response.body.errors.length).toEqual(1);
+        expect(response.body.errors[0].message).toEqual('Invalid entity name');
         expect(orchestrator.process).toHaveBeenCalledTimes(1);
         expect(orchestrator.process).toHaveBeenCalledWith({
           entity: {
@@ -462,8 +463,8 @@ describe('createRouter readonly disabled', () => {
             metadata: {
               name: 'invalid*name',
               annotations: {
-                [ANNOTATION_SOURCE_LOCATION]: 'validate:entity',
-                [ANNOTATION_ORIGIN_LOCATION]: 'validate:entity',
+                [ANNOTATION_LOCATION]: 'url:validate-entity',
+                [ANNOTATION_ORIGIN_LOCATION]: 'url:validate-entity',
               },
             },
           },
@@ -484,32 +485,23 @@ describe('createRouter readonly disabled', () => {
           .send({ entity, location: null });
 
         expect(response.status).toEqual(400);
-        expect(response.body.error.message).toContain('Malformed request:');
+        expect(response.body.errors.length).toEqual(1);
+        expect(response.body.errors[0].message).toContain('Malformed request:');
         expect(orchestrator.process).toHaveBeenCalledTimes(0);
       });
     });
 
     describe('no entity', () => {
       it('returns 400', async () => {
-        orchestrator.process.mockResolvedValueOnce({
-          ok: false,
-          errors: [
-            new Error(
-              'Entity envelope failed validation before processing; caused by TypeError: <root> must be object - type: object',
-            ),
-          ],
-        });
-
         const response = await request(app)
           .post('/validate-entity')
-          .send({ entity: null, location: 'validate:entity' });
+          .send({ entity: null, location: 'url:entity' });
 
         expect(response.status).toEqual(400);
-        expect(response.body.error.message).toContain(
-          'Entity envelope failed validation',
+        expect(response.body.errors.length).toEqual(1);
+        expect(response.body.errors[0].message).toContain(
+          '<root> must be object - type: object',
         );
-        // We expect this to be called because of an open issue with zod where unknown values are optional https://github.com/colinhacks/zod/issues/493
-        expect(orchestrator.process).toHaveBeenCalledTimes(1);
       });
     });
   });

plugins/catalog-backend/src/service/createRouter.ts

@@ -15,9 +15,15 @@
  */
 
 import { errorHandler } from '@backstage/backend-common';
-import { Entity, stringifyEntityRef } from '@backstage/catalog-model';
+import {
+  ANNOTATION_LOCATION,
+  ANNOTATION_ORIGIN_LOCATION,
+  Entity,
+  parseLocationRef,
+  stringifyEntityRef,
+} from '@backstage/catalog-model';
 import { Config } from '@backstage/config';
-import { InputError, NotFoundError } from '@backstage/errors';
+import { NotFoundError, serializeError } from '@backstage/errors';
 import express from 'express';
 import Router from 'express-promise-router';
 import { Logger } from 'winston';
@@ -34,12 +40,12 @@ import {
   disallowReadonlyMode,
   locationInput,
   validateRequestBody,
-  setRequiredEntityLocationAnnotations,
 } from './util';
 import { z } from 'zod';
 import { parseEntityFacetParams } from './request/parseEntityFacetParams';
 import { RefreshOptions, LocationService, RefreshService } from './types';
 import { CatalogProcessingOrchestrator } from '../processing/types';
+import { validateEntityEnvelope } from '../processing/util';
 
 /**
  * Options used by {@link createRouter}.
@@ -238,18 +244,42 @@ export async function createRouter(
         location: z.string(),
       });
 
-      const body = await validateRequestBody(req, bodySchema);
+      let body: z.infer<typeof bodySchema>;
+      let entity: Entity;
+      let location: { type: string; target: string };
+      try {
+        body = await validateRequestBody(req, bodySchema);
+        entity = validateEntityEnvelope(body.entity);
+        location = parseLocationRef(body.location);
+        if (location.type !== 'url')
+          throw new TypeError(
+            `Invalid location ref ${body.location}, only 'url:<target>' is supported, e.g. url:https://host/path`,
+          );
+      } catch (err) {
+        return res.status(400).json({
+          errors: [serializeError(err)],
+        });
+      }
 
-      const entity = setRequiredEntityLocationAnnotations(
-        body.entity as Entity,
-        body.location,
-      );
       const processingResult = await orchestrator.process({
-        entity,
+        entity: {
+          ...entity,
+          metadata: {
+            ...entity.metadata,
+            annotations: {
+              [ANNOTATION_LOCATION]: body.location,
+              [ANNOTATION_ORIGIN_LOCATION]: body.location,
+              ...entity.metadata.annotations,
+            },
+          },
+        },
       });
+
       if (!processingResult.ok)
-        throw new InputError(processingResult.errors[0].message);
-      return res.status(200).send();
+        res.status(400).json({
+          errors: processingResult.errors.map(e => serializeError(e)),
+        });
+      return res.status(200).end();
     });
   }
 

plugins/catalog-backend/src/service/util.ts

@@ -14,11 +14,6 @@
  * limitations under the License.
  */
 
-import {
-  Entity,
-  ANNOTATION_SOURCE_LOCATION,
-  ANNOTATION_ORIGIN_LOCATION,
-} from '@backstage/catalog-model';
 import { InputError, NotAllowedError } from '@backstage/errors';
 import { Request } from 'express';
 import lodash from 'lodash';
@@ -53,25 +48,6 @@ export const locationInput = z
   })
   .strict(); // no unknown keys;
 
-export function setRequiredEntityLocationAnnotations(
-  entity: Entity,
-  location: string,
-): Entity {
-  const clonedEntity = lodash.cloneDeep(entity);
-  lodash.set(
-    clonedEntity,
-    `metadata.annotations.['${ANNOTATION_SOURCE_LOCATION}']`,
-    location,
-  );
-  lodash.set(
-    clonedEntity,
-    `metadata.annotations.['${ANNOTATION_ORIGIN_LOCATION}']`,
-    location,
-  );
-
-  return clonedEntity;
-}
-
 export async function validateRequestBody<T>(
   req: Request,
   schema: z.Schema<T>,