fix: avoid cloning requests in FetchBuilder (#228)

Author: andreas-karlsson

.eslintrc

@@ -19,6 +19,7 @@
     "es/no-optional-chaining": "off",
     "es/no-nullish-coalescing-operators": "off",
     "no-inner-declarations": "off",
-    "@typescript-eslint/no-use-before-define": "off"
+    "@typescript-eslint/no-use-before-define": "off",
+    "no-param-reassign": "off"
   }
 }

api/sdk.api.md

@@ -157,7 +157,7 @@ export function pageViews(): Trackable.Manager;
 
 // Warning: (ae-missing-release-tag) "SimpleFetch" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
-// @public (undocumented)
+// @public
 export type SimpleFetch = (request: Request) => Promise<Response>;
 
 // @public

packages/sdk/src/Confidence.ts

@@ -14,10 +14,11 @@ import { SdkId } from './generated/confidence/flags/resolver/v1/types';
 import { Trackable } from './Trackable';
 import { Closer } from './Closer';
 import { Subscribe, Observer, subject, changeObserver } from './observing';
-import { SimpleFetch, WaitUntil } from './types';
+import { WaitUntil } from './types';
 import { FlagResolution } from './FlagResolution';
 import { AccessiblePromise } from './AccessiblePromise';
 import { Telemetry } from './Telemetry';
+import { SimpleFetch } from './fetch-util';
 
 /**
  * Confidence options, to be used for easier initialization of Confidence

packages/sdk/src/EventSenderEngine.ts

@@ -1,7 +1,6 @@
 import { Value } from './Value';
 import { Logger } from './logger';
-import { FetchBuilder, TimeUnit } from './fetch-util';
-import { SimpleFetch } from './types';
+import { FetchBuilder, InternalFetch, SimpleFetch, TimeUnit } from './fetch-util';
 import { EventData } from './events';
 interface Event {
   eventDefinition: string;
@@ -36,7 +35,7 @@ export class EventSenderEngine {
   private readonly flushTimeoutMilliseconds: number;
   private readonly clientSecret: string;
   private readonly maxBatchSize: number;
-  private readonly fetchImplementation: SimpleFetch;
+  private readonly fetchImplementation: InternalFetch;
   private readonly publishUrl: string;
   private readonly logger: Logger;
   private pendingFlush: undefined | ReturnType<typeof setTimeout>;
@@ -70,12 +69,12 @@ export class EventSenderEngine {
     }
     this.fetchImplementation = fetchBuilder
       // update send-time before sending
-      .modifyRequest(async request => {
-        if (request.method === 'POST') {
-          const body = JSON.stringify({ ...(await request.json()), sendTime: new Date().toISOString() });
-          return new Request(request, { body });
+      .modifyRequest(({ method, body }) => {
+        if (method === 'POST' && body) {
+          body = JSON.stringify({ ...JSON.parse(body), sendTime: new Date().toISOString() });
+          return { body };
         }
-        return request;
+        return {};
       })
       .build(fetchImplementation);
   }
@@ -141,15 +140,15 @@ export class EventSenderEngine {
 
   // Made public for unit testing
   public upload(batch: EventBatch): Promise<PublishError[]> {
-    const request = new Request(this.publishUrl, {
+    const request = {
       method: 'POST',
       headers: { 'Content-Type': 'application/json' },
       body: JSON.stringify({
         ...batch,
         events: batch.events.map(e => ({ ...e, eventDefinition: `eventDefinitions/${e.eventDefinition}` })),
       }),
-    });
-    return this.fetchImplementation(request)
+    };
+    return this.fetchImplementation(this.publishUrl, request)
       .then(resp => resp.json())
       .then(({ errors }) => errors);
   }

packages/sdk/src/FlagResolverClient.test.ts

@@ -8,7 +8,7 @@ import {
 } from './FlagResolverClient';
 import { setMaxListeners } from 'node:events';
 import { SdkId } from './generated/confidence/flags/resolver/v1/types';
-import { abortableSleep, FetchBuilder } from './fetch-util';
+import { abortableSleep, FetchBuilder, InternalFetch, SimpleFetch } from './fetch-util';
 import { ApplyFlagsRequest, ResolveFlagsRequest } from './generated/confidence/flags/resolver/v1/api';
 import { FailedFlagResolution, FlagResolution } from './FlagResolution';
 import { Telemetry } from './Telemetry';
@@ -24,7 +24,7 @@ const dummyResolveToken = Uint8Array.from(atob('SGVsbG9Xb3JsZA=='), c => c.charC
 const resolveHandlerMock = jest.fn();
 const applyHandlerMock = jest.fn();
 
-const fetchImplementation = async (request: Request): Promise<Response> => {
+const fetchImplementation: SimpleFetch = async (request): Promise<Response> => {
   await abortableSleep(0, request.signal);
 
   let handler: (reqBody: any) => any;
@@ -327,14 +327,15 @@ describe('Backend environment Evaluation', () => {
 });
 
 describe('intercept', () => {
-  const fetchMock = jest.fn<Promise<Response>, [Request]>();
-  const fetchBuilder = new FetchBuilder();
-  let underTest: typeof fetch;
+  const fetchMock: jest.MockedFn<SimpleFetch> = jest.fn();
+  let underTest: InternalFetch;
 
   beforeEach(() => {
     jest.useFakeTimers();
     jest.setSystemTime(0);
-    underTest = withRequestLogic(fetchMock, console);
+    const fetchBuilder = new FetchBuilder();
+    withRequestLogic(fetchBuilder, console);
+    underTest = fetchBuilder.build(fetchMock);
   });
   afterEach(() => {
     if (jest.getTimerCount() !== 0) throw new Error('test finished with remaining timers');
@@ -347,7 +348,9 @@ describe('intercept', () => {
     );
 
     beforeEach(() => {
-      underTest = withTelemetryData(fetchBuilder, telemetryMock).build(fetchMock);
+      const fetchBuilder = new FetchBuilder();
+      withTelemetryData(fetchBuilder, telemetryMock);
+      underTest = fetchBuilder.build(fetchMock);
       telemetryMock.getSnapshot = jest.fn().mockReturnValue({
         libraryTraces: [
           {
@@ -364,19 +367,8 @@ describe('intercept', () => {
       fetchMock.mockResolvedValue(new Response());
       await underTest('https://resolver.confidence.dev/v1/flags:resolve', { method: 'POST', body: '{}' });
       expect(fetchMock).toBeCalledTimes(1);
-      const request = fetchMock.mock.calls[0][0];
-      expect(request.headers.has('X-Confidence-Telemetry')).toBeTruthy();
-      expect(request.headers.get('X-Confidence-Telemetry')).toEqual('CgwIAxIEdGVzdBoCCAMQBA==');
-    });
-  });
-
-  describe('withRequestLogic', () => {
-    it('should throw on unknown urls', async () => {
-      fetchMock.mockResolvedValue(new Response());
-
-      await expect(underTest('https://resolver.confidence.dev/v1/flags:bad')).rejects.toThrow(
-        'Unexpected url: https://resolver.confidence.dev/v1/flags:bad',
-      );
+      const headers = fetchMock.mock.calls[0][0].headers;
+      expect(headers.get('X-CONFIDENCE-TELEMETRY')).toEqual('CgwIAxIEdGVzdBoCCAMQBA==');
     });
   });
 

packages/sdk/src/FlagResolverClient.ts

@@ -4,7 +4,7 @@ import { Applier, FlagResolution } from './FlagResolution';
 import { Telemetry, Meter } from './Telemetry';
 import { Value } from './Value';
 import { Context } from './context';
-import { FetchBuilder, TimeUnit } from './fetch-util';
+import { FetchBuilder, InternalFetch, SimpleFetch, TimeUnit } from './fetch-util';
 import {
   ResolveFlagsRequest,
   ResolveFlagsResponse,
@@ -18,7 +18,7 @@ import {
   Monitoring,
 } from './generated/confidence/telemetry/v1/telemetry';
 import { Logger } from './logger';
-import { SimpleFetch, WaitUntil } from './types';
+import { WaitUntil } from './types';
 
 const FLAG_PREFIX = 'flags/';
 const retryCodes = new Set([408, 502, 503, 504]);
@@ -99,7 +99,7 @@ export type FlagResolverClientOptions = {
 };
 
 export class FetchingFlagResolverClient implements FlagResolverClient {
-  private readonly fetchImplementation: SimpleFetch;
+  private readonly fetchImplementation: InternalFetch;
   private readonly clientSecret: string;
   private readonly sdk: Sdk;
   private readonly applyDebounce: number;
@@ -127,11 +127,14 @@ export class FetchingFlagResolverClient implements FlagResolverClient {
       version: sdk.version,
       id: LibraryTraces_TraceId.TRACE_ID_RESOLVE_LATENCY,
     });
-    const fetchBuilderWithTelemetry = withTelemetryData(new FetchBuilder(), telemetry);
-    // TODO think about both resolve and apply request logic for backends
-    const fetchWithTelemetry = fetchBuilderWithTelemetry.build(fetchImplementation);
-    this.fetchImplementation =
-      environment === 'backend' ? fetchWithTelemetry : withRequestLogic(fetchWithTelemetry, logger);
+
+    const fetchBuilder = new FetchBuilder();
+    withTelemetryData(fetchBuilder, telemetry);
+    if (environment === 'client') {
+      withRequestLogic(fetchBuilder, logger);
+    }
+
+    this.fetchImplementation = fetchBuilder.build(fetchImplementation);
 
     this.clientSecret = clientSecret;
     this.sdk = sdk;
@@ -214,31 +217,27 @@ export class FetchingFlagResolverClient implements FlagResolverClient {
   }
 
   async apply(request: ApplyFlagsRequest): Promise<void> {
-    const resp = await this.fetchImplementation(
-      new Request(`${this.baseUrl}/flags:apply`, {
-        method: 'POST',
-        headers: {
-          'Content-Type': 'application/json',
-        },
-        body: JSON.stringify(ApplyFlagsRequest.toJSON(request)),
-      }),
-    );
+    const resp = await this.fetchImplementation(`${this.baseUrl}/flags:apply`, {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/json',
+      },
+      body: JSON.stringify(ApplyFlagsRequest.toJSON(request)),
+    });
     if (!resp.ok) {
       throw new Error(`${resp.status}: ${resp.statusText}`);
     }
   }
 
   async resolveFlagsJson(request: ResolveFlagsRequest, signal: AbortSignal): Promise<ResolveFlagsResponse> {
-    const resp = await this.fetchImplementation(
-      new Request(`${this.baseUrl}/flags:resolve`, {
-        method: 'POST',
-        headers: {
-          'Content-Type': 'application/json',
-        },
-        body: JSON.stringify(ResolveFlagsRequest.toJSON(request)),
-        signal,
-      }),
-    );
+    const resp = await this.fetchImplementation(`${this.baseUrl}/flags:resolve`, {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/json',
+      },
+      body: JSON.stringify(ResolveFlagsRequest.toJSON(request)),
+      signal,
+    });
     if (!resp.ok) {
       throw new Error(`${resp.status}: ${resp.statusText}`);
     }
@@ -315,24 +314,19 @@ export class CachingFlagResolverClient implements FlagResolverClient {
   }
 }
 
-export function withTelemetryData(fetchBuilder: FetchBuilder, telemetry: Telemetry): FetchBuilder {
-  return fetchBuilder.modifyRequest(async request => {
+export function withTelemetryData(fetchBuilder: FetchBuilder, telemetry: Telemetry) {
+  fetchBuilder.modifyRequest(({ headers }) => {
     const monitoring = telemetry.getSnapshot();
     if (monitoring.libraryTraces.length > 0) {
-      const headers = new Headers(request.headers);
       const base64Message = btoa(String.fromCharCode(...Monitoring.encode(monitoring).finish()));
 
-      headers.set('X-CONFIDENCE-TELEMETRY', base64Message);
-      return new Request(request, { headers });
+      return { headers: { ...headers, ['X-CONFIDENCE-TELEMETRY']: base64Message } };
     }
-    return request;
+    return {};
   });
 }
 
-export function withRequestLogic(
-  fetchImplementation: (request: Request) => Promise<Response>,
-  logger: Logger,
-): typeof fetch {
+export function withRequestLogic(fetchBuilder: FetchBuilder, logger: Logger) {
   const fetchResolve = new FetchBuilder()
     // infinite retries without delay until aborted by timeout
     .compose(next => async request => {
@@ -347,8 +341,7 @@ export function withRequestLogic(
     .rejectNotOk()
     .retry()
     .rejectOn(response => retryCodes.has(response.status))
-    .rateLimit(1, { initialTokens: 3, maxTokens: 2 })
-    .build(fetchImplementation);
+    .rateLimit(1, { initialTokens: 3, maxTokens: 2 });
 
   const fetchApply = new FetchBuilder()
     .limitPending(1000)
@@ -358,22 +351,17 @@ export function withRequestLogic(
     .rejectOn(response => retryCodes.has(response.status))
     .rateLimit(2)
     // update send-time before sending
-    .modifyRequest(async request => {
-      if (request.method === 'POST') {
-        const body = JSON.stringify({ ...(await request.clone().json()), sendTime: new Date().toISOString() });
-        return new Request(request, { body });
+    .modifyRequest(({ method, body }) => {
+      if (method === 'POST' && body) {
+        body = JSON.stringify({ ...JSON.parse(body), sendTime: new Date().toISOString() });
+        return { body };
       }
-      return request;
-    })
-    .build(fetchImplementation);
-
-  return (
-    new FetchBuilder()
-      .route(url => url.endsWith('flags:resolve'), fetchResolve)
-      .route(url => url.endsWith('flags:apply'), fetchApply)
-      // throw so we notice changes in endpoints that should be handled here
-      .build(request => Promise.reject(new Error(`Unexpected url: ${request.url}`)))
-  );
+      return {};
+    });
+
+  fetchBuilder
+    .route(url => url.endsWith('flags:resolve'), fetchResolve)
+    .route(url => url.endsWith('flags:apply'), fetchApply);
 }
 
 function withTimeout(signal: AbortSignal, timeout: number, reason?: any): AbortSignal {

packages/sdk/src/fetch-util.test.ts

@@ -3,36 +3,6 @@ import { FetchBuilder } from './fetch-util';
 jest.useFakeTimers();
 
 describe('FetchBuilder', () => {
-  describe('abortPrevious', () => {
-    const response = new Response(null, { status: 200 });
-    const simpleFetchMock = jest.fn<Promise<Response>, [Request]>(
-      ({ signal }) =>
-        new Promise((resolve, reject) => {
-          signal.onabort = () => reject(signal.reason);
-          setTimeout(() => resolve(response), 100);
-        }),
-    );
-    it('should pass one request through', async () => {
-      const underTest = new FetchBuilder().abortPrevious().build(simpleFetchMock);
-
-      const result = underTest('http://www.spotify.com');
-      jest.runAllTimers();
-
-      expect(await result).toBe(response);
-    });
-
-    it('should abort first request when a second arrives', async () => {
-      const underTest = new FetchBuilder().abortPrevious().build(simpleFetchMock);
-
-      // we need to turn the catch into a result or node will complain about unh
-      expect(underTest('http://www.spotify.com')).rejects.toThrow('Request superseded');
-      expect(underTest('http://www.spotify.com')).resolves.toBe(response);
-
-      await jest.runAllTimersAsync();
-
-      expect.assertions(2);
-    });
-  });
   describe('rateLimit', () => {
     it('it keeps request rate under maxRate', async () => {
       let requestCount = 0;
@@ -213,7 +183,7 @@ describe('FetchBuilder', () => {
       const simpleFetchMock = jest.fn();
       simpleFetchMock.mockResolvedValue(new Response());
       const requestModifyingFetch = new FetchBuilder()
-        .modifyRequest(async request => new Request(`${request.url}modified`, request))
+        .modifyRequest(async ({ url }) => ({ url: `${url}modified` }))
         .build(simpleFetchMock);
 
       await requestModifyingFetch('http://test.com');