Merge pull request #1 from catloafsoft/codex/harden-network-error-reporting

[codex] harden network error reporting

Author: megastep

.github/workflows/integration_tests.yml

@@ -26,15 +26,9 @@ jobs:
       - name: Use Node.js
         uses: actions/setup-node@v4
         with:
-          node-version: '20.x'
-
-      - name: Cache NPM # leverage npm cache on repeated workflow runs if package.json didn't change
-        uses: actions/cache@v4
-        with:
-          path: ~/.npm
-          key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
-          restore-keys: |
-            ${{ runner.os }}-node-
+          node-version: '22.x'
+          cache: 'yarn'
+          cache-dependency-path: yarn.lock
       - name: Install Dependencies
         run: yarn
 

.github/workflows/pr-checks.yml

@@ -17,15 +17,9 @@ jobs:
       - name: Use Node.js
         uses: actions/setup-node@v4
         with:
-          node-version: '20.x'
-
-      - name: Cache NPM # leverage npm cache on repeated workflow runs if package.json didn't change
-        uses: actions/cache@v4
-        with:
-          path: ~/.npm
-          key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
-          restore-keys: |
-            ${{ runner.os }}-node-
+          node-version: '22.x'
+          cache: 'yarn'
+          cache-dependency-path: yarn.lock
       - name: Install Dependencies
         run: yarn
 
@@ -44,15 +38,9 @@ jobs:
       - name: Use Node.js
         uses: actions/setup-node@v4
         with:
-          node-version: '20.x'
-
-      - name: Cache NPM # leverage npm cache on repeated workflow runs if package.json didn't change
-        uses: actions/cache@v4
-        with:
-          path: ~/.npm
-          key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
-          restore-keys: |
-            ${{ runner.os }}-node-
+          node-version: '22.x'
+          cache: 'yarn'
+          cache-dependency-path: yarn.lock
       - name: Install Dependencies
         run: yarn
 
@@ -71,15 +59,9 @@ jobs:
       - name: Use Node.js
         uses: actions/setup-node@v4
         with:
-          node-version: '20.x'
-
-      - name: Cache NPM
-        uses: actions/cache@v4
-        with:
-          path: ~/.npm
-          key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
-          restore-keys: |
-            ${{ runner.os }}-node-
+          node-version: '22.x'
+          cache: 'yarn'
+          cache-dependency-path: yarn.lock
       - name: Install Dependencies
         run: yarn
 

.github/workflows/publish.yml

@@ -12,16 +12,18 @@ jobs:
       - name: Check out code
         uses: actions/checkout@v4
 
-      - name: Build typescript
-        run: |
-          yarn
-          yarn build
-
       - name: Setup node for publishing
         uses: actions/setup-node@v4
         with:
-          node-version: '20.x'
+          node-version: '22.x'
           registry-url: 'https://registry.npmjs.org'
+          cache: 'yarn'
+          cache-dependency-path: yarn.lock
+
+      - name: Build typescript
+        run: |
+          yarn
+          yarn build
 
       - name: Publish to npm
         run: |

.yarnrc.yml

@@ -0,0 +1 @@
+nodeLinker: node-modules

sdk/src/__tests__/internal/network/ApiInteractor.test.ts

@@ -154,6 +154,61 @@ describe('execute tests', () => {
     expect(ApiInteractorImpl.getErrorResponse).toBeCalledTimes(1);
   });
 
+  test('network client parse error with client response code is not retried', async () => {
+    // given
+    const expectedError = new QonversionError(
+      QonversionErrorCode.BackendError,
+      'Failed to parse JSON response',
+      undefined,
+      400,
+    );
+    networkClient.execute = jest.fn(() => {
+      throw expectedError;
+    });
+    ApiInteractorImpl.getErrorResponse = savedGetErrorResponse;
+
+    // when
+    const response = await apiInteractor.execute(request, new RetryPolicyExponential(3));
+
+    // then
+    expect(response).toStrictEqual({
+      code: 400,
+      message: 'Failed to parse JSON response',
+      isSuccess: false,
+    });
+    expect(networkClient.execute).toBeCalledTimes(1);
+    expect(apiInteractor.prepareRetryConfig).not.toBeCalled();
+  });
+
+  test('network client parse error with server response code is retried', async () => {
+    // given
+    const retryCount = 2;
+    const expectedError = new QonversionError(
+      QonversionErrorCode.BackendError,
+      'Failed to parse JSON response',
+      undefined,
+      500,
+    );
+    networkClient.execute = jest.fn(() => {throw expectedError});
+    ApiInteractorImpl.getErrorResponse = savedGetErrorResponse;
+    apiInteractor.prepareRetryConfig = jest.fn((retryPolicy, attemptIndex) => ({
+      attemptIndex: attemptIndex + 1,
+      delay: 1,
+      shouldRetry: attemptIndex < retryCount,
+    }));
+
+    // when
+    const response = await apiInteractor.execute(request, new RetryPolicyExponential(retryCount));
+
+    // then
+    expect(response).toStrictEqual({
+      code: 500,
+      message: 'Failed to parse JSON response',
+      isSuccess: false,
+    });
+    expect(networkClient.execute).toBeCalledTimes(retryCount + 1);
+  });
+
   test('retryable error response without retry config', async () => {
     // given
     testResponseCode = 555;
@@ -375,6 +430,67 @@ describe('getErrorResponse tests', () => {
     expect(result).toStrictEqual(expResult);
   });
 
+  test('get error from malformed api error object', () => {
+    // given
+    const networkResponse: RawNetworkResponse = {
+      code: 502,
+      payload: {},
+    };
+
+    // when
+    const result = ApiInteractorImpl.getErrorResponse(networkResponse)
+
+    // then
+    expect(result).toStrictEqual({
+      apiCode: undefined,
+      code: 502,
+      message: 'Unexpected API error response',
+      type: undefined,
+      isSuccess: false,
+    });
+  });
+
+  test('get error from plain text payload', () => {
+    // given
+    const networkResponse: RawNetworkResponse = {
+      code: 503,
+      payload: 'service unavailable',
+    };
+
+    // when
+    const result = ApiInteractorImpl.getErrorResponse(networkResponse)
+
+    // then
+    expect(result).toStrictEqual({
+      apiCode: undefined,
+      code: 503,
+      message: 'Unexpected API error response: service unavailable',
+      type: undefined,
+      isSuccess: false,
+    });
+  });
+
+  test('get error from long plain text payload', () => {
+    // given
+    const payload = 'x'.repeat(130);
+    const networkResponse: RawNetworkResponse = {
+      code: 503,
+      payload,
+    };
+
+    // when
+    const result = ApiInteractorImpl.getErrorResponse(networkResponse)
+
+    // then
+    expect(result).toStrictEqual({
+      apiCode: undefined,
+      code: 503,
+      message: `Unexpected API error response: ${'x'.repeat(120)}...`,
+      type: undefined,
+      isSuccess: false,
+    });
+  });
+
   test('get error from execution error', () => {
     // given
     const executionError = new Error('execution error');
@@ -385,7 +501,27 @@ describe('getErrorResponse tests', () => {
     }).toThrow(executionError);
   });
 
-  test('get error from execution error', () => {
+  test('get error from execution error with response code', () => {
+    // given
+    const executionError = new QonversionError(
+      QonversionErrorCode.BackendError,
+      'Failed to parse JSON response',
+      undefined,
+      400,
+    );
+
+    // when
+    const result = ApiInteractorImpl.getErrorResponse(undefined, executionError);
+
+    // then
+    expect(result).toStrictEqual({
+      code: 400,
+      message: 'Failed to parse JSON response',
+      isSuccess: false,
+    });
+  });
+
+  test('throw when neither response nor execution error is provided', () => {
     // given
 
     // when and then

sdk/src/__tests__/internal/network/NetworkClient.test.ts

@@ -6,6 +6,7 @@ import {
   RequestHeaders,
   RequestType
 } from '../../../internal/network';
+import {QonversionError, QonversionErrorCode} from '../../../index';
 
 const networkClient = new NetworkClientImpl();
 
@@ -31,7 +32,10 @@ describe('execute test', () => {
   const mockFetch = jest.fn(() =>
     Promise.resolve({
       status: testCode,
-      json: () => Promise.resolve(testPayload),
+      headers: {
+        get: (header: string) => header === 'content-type' ? 'application/json' : null,
+      },
+      text: () => Promise.resolve(JSON.stringify(testPayload)),
     })
   );
 
@@ -96,4 +100,112 @@ describe('execute test', () => {
     expect(result).toStrictEqual(expResult);
     expect(fetch).toBeCalledWith(testUrl, expRequest);
   });
+
+  test('execute with empty response body', async () => {
+    // given
+    const request: NetworkRequest = {
+      headers: testHeaders,
+      type: RequestType.GET,
+      url: testUrl
+    };
+    mockFetch.mockImplementationOnce(() =>
+      Promise.resolve({
+        status: 204,
+        headers: {
+          get: () => null,
+        },
+        text: () => Promise.resolve(''),
+      })
+    );
+
+    // when
+    const result = await networkClient.execute(request);
+
+    // then
+    expect(result).toStrictEqual({
+      code: 204,
+      payload: undefined,
+    });
+  });
+
+  test('execute with plain text response body', async () => {
+    // given
+    const request: NetworkRequest = {
+      headers: testHeaders,
+      type: RequestType.GET,
+      url: testUrl
+    };
+    mockFetch.mockImplementationOnce(() =>
+      Promise.resolve({
+        status: 503,
+        headers: {
+          get: (header: string) => header === 'content-type' ? 'text/plain' : null,
+        },
+        text: () => Promise.resolve('service unavailable'),
+      })
+    );
+
+    // when
+    const result = await networkClient.execute(request);
+
+    // then
+    expect(result).toStrictEqual({
+      code: 503,
+      payload: 'service unavailable',
+    });
+  });
+
+  test('execute with malformed json body', async () => {
+    // given
+    const request: NetworkRequest = {
+      headers: testHeaders,
+      type: RequestType.GET,
+      url: testUrl
+    };
+    mockFetch.mockImplementationOnce(() =>
+      Promise.resolve({
+        status: 500,
+        headers: {
+          get: (header: string) => header === 'content-type' ? 'application/json' : null,
+        },
+        text: () => Promise.resolve('{'),
+      })
+    );
+
+    // when and then
+    const execution = networkClient.execute(request);
+    await expect(execution).rejects.toBeInstanceOf(QonversionError);
+    await expect(execution).rejects.toMatchObject({
+      code: QonversionErrorCode.BackendError,
+      details: 'Failed to parse JSON response',
+      responseCode: 500,
+    });
+  });
+
+  test('execute with whitespace-only json body', async () => {
+    // given
+    const request: NetworkRequest = {
+      headers: testHeaders,
+      type: RequestType.GET,
+      url: testUrl
+    };
+    mockFetch.mockImplementationOnce(() =>
+      Promise.resolve({
+        status: 204,
+        headers: {
+          get: (header: string) => header === 'content-type' ? 'application/json' : null,
+        },
+        text: () => Promise.resolve('\n  '),
+      })
+    );
+
+    // when
+    const result = await networkClient.execute(request);
+
+    // then
+    expect(result).toStrictEqual({
+      code: 204,
+      payload: undefined,
+    });
+  });
 });