SIMSBIOHUB-895: Return 404 for Resource Not Found (#350)

* 404 for resource not found

* linter

* fix missed test updates

Author: mauberti-bc

api/src/errors/api-error.test.ts

@@ -1,6 +1,6 @@
 import { expect } from 'chai';
 import { describe } from 'mocha';
-import { ApiErrorType, ApiExecuteSQLError, ApiGeneralError } from './api-error';
+import { ApiErrorType, ApiExecuteSQLError, ApiGeneralError, ApiNotFoundError } from './api-error';
 
 describe('ApiError', () => {
   describe('No error value provided', () => {
@@ -17,5 +17,9 @@ describe('ApiError', () => {
     it('Creates Api execute SQL error', function () {
       expect(new ApiExecuteSQLError(message).name).to.equal(ApiErrorType.EXECUTE_SQL);
     });
+
+    it('Creates Api not found error', function () {
+      expect(new ApiNotFoundError(message).name).to.equal(ApiErrorType.NOT_FOUND);
+    });
   });
 });

api/src/errors/api-error.ts

@@ -5,6 +5,7 @@ export enum ApiErrorType {
   EXECUTE_SQL = 'Error executing SQL query',
   GENERAL = 'Error',
   CONFLICT = 'Conflict',
+  NOT_FOUND = 'Not Found',
   UNKNOWN = 'Unknown Error'
 }
 
@@ -55,6 +56,19 @@ export class ApiConflictError extends ApiError {
   }
 }
 
+/**
+ * Api could not find a requested resource.
+ *
+ * @export
+ * @class ApiNotFoundError
+ * @extends {ApiError}
+ */
+export class ApiNotFoundError extends ApiError {
+  constructor(message: string, errors?: (string | object)[]) {
+    super(ApiErrorType.NOT_FOUND, message, errors);
+  }
+}
+
 /**
  * API executed a query against the database, but the response was missing data, or indicated the query failed.
  *

api/src/errors/http-error.test.ts

@@ -1,7 +1,7 @@
 import { expect } from 'chai';
 import { describe } from 'mocha';
 import { DatabaseError } from 'pg';
-import { ApiError, ApiErrorType } from './api-error';
+import { ApiError, ApiErrorType, ApiNotFoundError } from './api-error';
 import { ensureHTTPError, HTTP400, HTTP401, HTTP403, HTTP409, HTTP500, HTTPError, isAjvError } from './http-error';
 
 describe('HTTPError', () => {
@@ -56,6 +56,16 @@ describe('ensureHTTPError', () => {
     expect(ensuredError.message).to.equal('an api error message');
   });
 
+  it('returns HTTP404 when an ApiNotFoundError is provided', function () {
+    const apiError = new ApiNotFoundError('not found');
+
+    const ensuredError = ensureHTTPError(apiError);
+
+    expect(ensuredError).to.be.instanceof(HTTPError);
+    expect(ensuredError.status).to.equal(404);
+    expect(ensuredError.message).to.equal('not found');
+  });
+
   it('returns a HTTPError when a DatabaseError provided', function () {
     const databaseError = new DatabaseError('a db error message', 1, 'error');
 

api/src/errors/http-error.ts

@@ -1,5 +1,5 @@
 import { DatabaseError } from 'pg';
-import { ApiConflictError, ApiError } from './api-error';
+import { ApiConflictError, ApiError, ApiNotFoundError } from './api-error';
 import { BaseError } from './base-error';
 
 export enum HTTPErrorType {
@@ -153,6 +153,10 @@ export const ensureHTTPError = (error: HTTPError | ApiError | Error | any): HTTP
     return HTTP409.fromApiError(error);
   }
 
+  if (error instanceof ApiNotFoundError) {
+    return HTTP404.fromApiError(error);
+  }
+
   if (error instanceof ApiError) {
     return HTTP500.fromApiError(error);
   }

api/src/openapi/schemas/http-responses.ts

@@ -8,6 +8,9 @@ export const defaultErrorResponses = {
   403: {
     $ref: '#/components/responses/403'
   },
+  404: {
+    $ref: '#/components/responses/404'
+  },
   409: {
     $ref: '#/components/responses/409'
   },

api/src/paths/cart/{cartId}/index.ts

@@ -69,9 +69,6 @@ GET.apiDoc = {
         }
       }
     },
-    404: {
-      description: 'Cart not found'
-    },
     ...defaultErrorResponses
   }
 };

api/src/paths/data-request/{dataRequestId}/index.ts

@@ -93,9 +93,6 @@ GET.apiDoc = {
         }
       }
     },
-    404: {
-      description: 'Data request not found'
-    },
     ...defaultErrorResponses
   }
 };
@@ -130,9 +127,6 @@ PUT.apiDoc = {
     200: {
       description: 'Data request updated successfully'
     },
-    404: {
-      description: 'Data request not found'
-    },
     ...defaultErrorResponses
   }
 };
@@ -160,9 +154,6 @@ DELETE.apiDoc = {
     200: {
       description: 'Data request deleted successfully'
     },
-    404: {
-      description: 'Data request not found'
-    },
     ...defaultErrorResponses
   }
 };

api/src/repositories/authorization/policy-repository.test.ts

@@ -3,7 +3,7 @@ import { describe } from 'mocha';
 import { QueryResult } from 'pg';
 import sinon from 'sinon';
 import sinonChai from 'sinon-chai';
-import { ApiExecuteSQLError } from '../../errors/api-error';
+import { ApiExecuteSQLError, ApiNotFoundError } from '../../errors/api-error';
 import { Policy } from '../../models/policy';
 import { getMockDBConnection } from '../../__mocks__/db';
 import { PolicyRepository } from './policy-repository';
@@ -77,7 +77,8 @@ describe('PolicyRepository', () => {
         await repository.getPolicy('1');
         expect.fail();
       } catch (error) {
-        expect((error as ApiExecuteSQLError).message).to.equal('Failed to get policy');
+        expect(error).to.be.instanceOf(ApiNotFoundError);
+        expect((error as ApiNotFoundError).message).to.equal('Policy not found');
       }
     });
   });

api/src/repositories/authorization/policy-repository.ts

@@ -2,7 +2,7 @@ import { Knex } from 'knex';
 import SQL from 'sql-template-strings';
 import { getKnex } from '../../database/db';
 import { FeatureUrn } from '../../database/urn-utils.interface';
-import { ApiExecuteSQLError } from '../../errors/api-error';
+import { ApiExecuteSQLError, ApiNotFoundError } from '../../errors/api-error';
 import { CountResult } from '../../models/count';
 import { CreatePolicy, Policy, UpdatePolicy } from '../../models/policy';
 import { PolicyEffect } from '../../models/policy-statement';
@@ -58,10 +58,14 @@ export class PolicyRepository extends BaseRepository {
   async getPolicy(policyId: string): Promise<Policy> {
     const response = await this.getPolicies({ policyId });
 
+    if (response.length === 0) {
+      throw new ApiNotFoundError('Policy not found', ['PolicyRepository->getPolicy', { policyId }]);
+    }
+
     if (response.length !== 1) {
-      throw new ApiExecuteSQLError('Failed to get policy', [
+      throw new ApiExecuteSQLError('Unexpected row count', [
         'PolicyRepository->getPolicy',
-        'rowCount was null or undefined, expected rowCount = 1'
+        `expected rowCount=1, actual rowCount=${response.length}`
       ]);
     }
 

api/src/repositories/authorization/policy-statement-condition-repository.test.ts

@@ -3,7 +3,7 @@ import { describe } from 'mocha';
 import { QueryResult } from 'pg';
 import sinon from 'sinon';
 import sinonChai from 'sinon-chai';
-import { ApiExecuteSQLError } from '../../errors/api-error';
+import { ApiExecuteSQLError, ApiNotFoundError } from '../../errors/api-error';
 import { PolicyConditionOperator } from '../../models/policy-statement-condition';
 import { getMockDBConnection } from '../../__mocks__/db';
 import { PolicyStatementConditionRepository } from './policy-statement-condition-repository';
@@ -105,7 +105,8 @@ describe('PolicyStatementConditionRepository', () => {
         await repository.getPolicyStatementCondition('1');
         expect.fail();
       } catch (error) {
-        expect((error as ApiExecuteSQLError).message).to.equal('Failed to get policy statement condition');
+        expect(error).to.be.instanceOf(ApiNotFoundError);
+        expect((error as ApiNotFoundError).message).to.equal('Policy statement condition not found');
       }
     });
   });