From f130b8f03e5ebc93fba134f16a2aa5fdfba2d5a5 Mon Sep 17 00:00:00 2001 From: Dan Schultz Date: Mon, 21 Aug 2023 13:13:19 -0400 Subject: [PATCH] Throw on non-ok server responses When the server responds with a non-ok status we want to communicate that response to the calling code base via a thrown error. Prior to this commit server errors would often be completely ignored and could result in odd validation errors instead of communicating the actual issue. As part of this change our delete functions now *throw* when something goes wrong, and so the type signature has been updated to no longer return `boolean` to indicate success but rather a `void`. Issue #59 Handle server errors explicitly Issue #89 Evalauate return types for operation methods --- CHANGELOG.md | 8 +++++++- src/api/__tests__/deleteFolderVo.test.ts | 17 +++++++---------- src/api/__tests__/deleteRecordVo.test.ts | 17 +++++++---------- src/api/deleteFolderVo.ts | 8 ++------ src/api/deleteRecordVo.ts | 8 ++------ src/errors/HttpResponseError.ts | 8 ++++++++ src/errors/index.ts | 1 + src/sdk/__tests__/deleteArchiveRecord.test.ts | 17 +++++++---------- src/sdk/__tests__/deleteFolder.test.ts | 17 +++++++---------- src/sdk/deleteArchiveRecord.ts | 2 +- src/sdk/deleteFolder.ts | 2 +- src/utils/makePermanentApiCall.ts | 7 +++++++ 12 files changed, 57 insertions(+), 55 deletions(-) create mode 100644 src/errors/HttpResponseError.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ca09b54..f370446 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] -- This package no longer supports Node v14 +## Changed +- This package no longer supports Node v14. +- The `deleteFolder` and `deleteArchiveRecord` SDK methods now return void (was boolean). +- All SDK methods now throw if the server responds with a non-OK status. + +## Added +- The `HttpResponseError` type now exists. ## [0.6.0] - 2023-05-18 ## Changed diff --git a/src/api/__tests__/deleteFolderVo.test.ts b/src/api/__tests__/deleteFolderVo.test.ts index 4e30b42..1edcc6b 100644 --- a/src/api/__tests__/deleteFolderVo.test.ts +++ b/src/api/__tests__/deleteFolderVo.test.ts @@ -1,40 +1,37 @@ import nock from 'nock'; import { deleteFolderVo } from '..'; +import { HttpResponseError } from '../../errors'; describe('deleteFolderVo', () => { - it('should return true when a folder is deleted', async () => { + it('should not throw when a folder is deleted', async () => { nock('https://permanent.local') .delete( '/api/folder/delete?folderId=1', ) .reply(200); - const result = await deleteFolderVo( + await expect(deleteFolderVo( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(true); + )).resolves.not.toThrow(); }); - it('should return false when a folder is not deleted', async () => { + it('should throw an error when when a 500 status is returned', async () => { nock('https://permanent.local') .delete( '/api/folder/delete?folderId=1', ) .reply(500); - const result = await deleteFolderVo( + await expect(deleteFolderVo( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(false); + )).rejects.toThrow(HttpResponseError); }); }); diff --git a/src/api/__tests__/deleteRecordVo.test.ts b/src/api/__tests__/deleteRecordVo.test.ts index 9dc5cae..ce2cab0 100644 --- a/src/api/__tests__/deleteRecordVo.test.ts +++ b/src/api/__tests__/deleteRecordVo.test.ts @@ -1,40 +1,37 @@ import nock from 'nock'; import { deleteRecordVo } from '..'; +import { HttpResponseError } from '../../errors'; describe('deleteArchiveRecord', () => { - it('should return true when a record is deleted', async () => { + it('should not throw when a record is deleted', async () => { nock('https://permanent.local') .delete( '/api/record/delete?recordId=1', ) .reply(200); - const result = await deleteRecordVo( + await expect(deleteRecordVo( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(true); + )).resolves.not.toThrow(); }); - it('should return false when a record is not deleted', async () => { + it('should throw an error when when a 500 status is returned', async () => { nock('https://permanent.local') .delete( '/api/record/delete?recordId=1', ) .reply(500); - const result = await deleteRecordVo( + await expect(deleteRecordVo( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(false); + )).rejects.toThrow(HttpResponseError); }); }); diff --git a/src/api/deleteFolderVo.ts b/src/api/deleteFolderVo.ts index b840371..81ef309 100644 --- a/src/api/deleteFolderVo.ts +++ b/src/api/deleteFolderVo.ts @@ -5,17 +5,13 @@ import type { ClientConfiguration } from '../types'; export const deleteFolderVo = async ( clientConfiguration: ClientConfiguration, folderId: number, -): Promise => { +): Promise => { const queryParams = new URLSearchParams({ folderId: `${folderId}`, }); - const response = await makePermanentApiCall( + await makePermanentApiCall( clientConfiguration, `/folder/delete?${queryParams.toString()}`, { method: 'DELETE' }, ); - if (response.status === 200) { - return true; - } - return false; }; diff --git a/src/api/deleteRecordVo.ts b/src/api/deleteRecordVo.ts index 4555737..d737b4d 100644 --- a/src/api/deleteRecordVo.ts +++ b/src/api/deleteRecordVo.ts @@ -5,17 +5,13 @@ import type { ClientConfiguration } from '../types'; export const deleteRecordVo = async ( clientConfiguration: ClientConfiguration, recordId: number, -): Promise => { +): Promise => { const queryParams = new URLSearchParams({ recordId: `${recordId}`, }); - const response = await makePermanentApiCall( + await makePermanentApiCall( clientConfiguration, `/record/delete?${queryParams.toString()}`, { method: 'DELETE' }, ); - if (response.status === 200) { - return true; - } - return false; }; diff --git a/src/errors/HttpResponseError.ts b/src/errors/HttpResponseError.ts new file mode 100644 index 0000000..3b33de6 --- /dev/null +++ b/src/errors/HttpResponseError.ts @@ -0,0 +1,8 @@ +export class HttpResponseError extends Error { + public readonly statusCode: number; + + public constructor(statusCode: number, message: string) { + super(message); + this.statusCode = statusCode; + } +} diff --git a/src/errors/index.ts b/src/errors/index.ts index 118d370..0e75558 100644 --- a/src/errors/index.ts +++ b/src/errors/index.ts @@ -1 +1,2 @@ +export * from './HttpResponseError'; export * from './ValidationError'; diff --git a/src/sdk/__tests__/deleteArchiveRecord.test.ts b/src/sdk/__tests__/deleteArchiveRecord.test.ts index 0a3c110..022458a 100644 --- a/src/sdk/__tests__/deleteArchiveRecord.test.ts +++ b/src/sdk/__tests__/deleteArchiveRecord.test.ts @@ -1,40 +1,37 @@ import nock from 'nock'; import { deleteArchiveRecord } from '..'; +import { HttpResponseError } from '../../errors'; describe('deleteArchiveRecord', () => { - it('should return true when a record is deleted', async () => { + it('should not throw on successful deletion', async () => { nock('https://permanent.local') .delete( '/api/record/delete?recordId=1', ) .reply(200); - const result = await deleteArchiveRecord( + await expect(deleteArchiveRecord( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(true); + )).resolves.not.toThrow(); }); - it('should return false when a record is not deleted', async () => { + it('should throw an error when receiving a 500 response', async () => { nock('https://permanent.local') .delete( '/api/record/delete?recordId=1', ) .reply(500); - const result = await deleteArchiveRecord( + await expect(deleteArchiveRecord( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(false); + )).rejects.toThrow(HttpResponseError); }); }); diff --git a/src/sdk/__tests__/deleteFolder.test.ts b/src/sdk/__tests__/deleteFolder.test.ts index 100aae3..b154042 100644 --- a/src/sdk/__tests__/deleteFolder.test.ts +++ b/src/sdk/__tests__/deleteFolder.test.ts @@ -1,40 +1,37 @@ import nock from 'nock'; import { deleteFolder } from '..'; +import { HttpResponseError } from '../../errors/HttpResponseError'; describe('deleteFolder', () => { - it('should return true when a folder is deleted', async () => { + it('should not throw when a folder is deleted', async () => { nock('https://permanent.local') .delete( '/api/folder/delete?folderId=1', ) .reply(200); - const result = await deleteFolder( + await expect(deleteFolder( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(true); + )).resolves.not.toThrow(); }); - it('should return false when a folder is not deleted', async () => { + it('should throw an error when receiving a 500 response', async () => { nock('https://permanent.local') .delete( '/api/folder/delete?folderId=1', ) .reply(500); - const result = await deleteFolder( + await expect(deleteFolder( { bearerToken: '12345', baseUrl: 'https://permanent.local/api', }, 1, - ); - - expect(result).toBe(false); + )).rejects.toThrow(HttpResponseError); }); }); diff --git a/src/sdk/deleteArchiveRecord.ts b/src/sdk/deleteArchiveRecord.ts index b080ec5..10450e9 100644 --- a/src/sdk/deleteArchiveRecord.ts +++ b/src/sdk/deleteArchiveRecord.ts @@ -6,7 +6,7 @@ import type { export const deleteArchiveRecord = async ( clientConfiguration: ClientConfiguration, archiveRecordId: number, -): Promise => deleteRecordVo( +): Promise => deleteRecordVo( clientConfiguration, archiveRecordId, ); diff --git a/src/sdk/deleteFolder.ts b/src/sdk/deleteFolder.ts index 239a110..31220f1 100644 --- a/src/sdk/deleteFolder.ts +++ b/src/sdk/deleteFolder.ts @@ -6,7 +6,7 @@ import type { export const deleteFolder = async ( clientConfiguration: ClientConfiguration, folderId: number, -): Promise => deleteFolderVo( +): Promise => deleteFolderVo( clientConfiguration, folderId, ); diff --git a/src/utils/makePermanentApiCall.ts b/src/utils/makePermanentApiCall.ts index cc25606..9e5752d 100644 --- a/src/utils/makePermanentApiCall.ts +++ b/src/utils/makePermanentApiCall.ts @@ -1,4 +1,5 @@ import fetch from 'node-fetch'; +import { HttpResponseError } from '../errors'; import type { RequestInit, Response, @@ -38,5 +39,11 @@ export const makePermanentApiCall = async ( headers, }, ); + if (!response.ok) { + throw new HttpResponseError( + response.status, + await response.text(), + ); + } return response; };