Remove content type restriction for signals requests (#1181)

Author: silesky

.changeset/spotty-apes-matter.md

@@ -0,0 +1,6 @@
+---
+'@segment/analytics-signals': minor
+'@segment/analytics-signals-runtime': minor
+---
+
+Add HTTPMethods. Do not gate requests based on content-type.

packages/signals/signals-integration-tests/src/tests/signals-vanilla/network-signals-fetch.test.ts

@@ -10,7 +10,7 @@ test.describe('network signals - fetch', () => {
     indexPage = await new IndexPage().loadAndWait(page, basicEdgeFn)
   })
 
-  test('should not emit non-json requests', async () => {
+  test('should emit non-json requests but not include the data object', async () => {
     await indexPage.network.mockTestRoute('http://localhost/upload', {
       body: JSON.stringify({ foo: 'test' }),
     })
@@ -26,10 +26,16 @@ test.describe('network signals - fetch', () => {
       (el) => el.properties!.data.action === 'request'
     )
 
-    expect(requests).toHaveLength(0)
+    expect(requests[0].properties!.data).toMatchObject({
+      action: 'request',
+      contentType: 'multipart/form-data',
+      data: null,
+      method: 'POST',
+      url: 'http://localhost/upload',
+    })
   })
 
-  test('should try to parse the body of text/plain requests', async () => {
+  test('should try to parse the body of any requests with string in the body', async () => {
     await indexPage.network.mockTestRoute('http://localhost/test', {
       body: JSON.stringify({ foo: 'test' }),
       contentType: 'application/json',
@@ -251,19 +257,4 @@ test.describe('network signals - fetch', () => {
       expect(responses).toHaveLength(1)
     })
   })
-
-  test('not emit response errors if there is no corresponding request, even if correct content type', async () => {
-    await indexPage.network.mockTestRoute('http://localhost/test', {
-      body: JSON.stringify({ foo: 'test' }),
-      contentType: 'application/json',
-      status: 400,
-    })
-
-    await indexPage.network.makeFileUploadRequest('http://localhost/test')
-
-    await indexPage.waitForSignalsApiFlush()
-
-    const networkEvents = indexPage.signalsAPI.getEvents('network')
-    expect(networkEvents).toHaveLength(0)
-  })
 })

packages/signals/signals-runtime/src/web/web-signals-types.ts

@@ -75,7 +75,7 @@ interface BaseNetworkData {
 interface NetworkRequestData extends BaseNetworkData {
   action: 'request'
   url: string
-  method: string
+  method: HTTPMethod
 }
 
 interface NetworkResponseData extends BaseNetworkData {
@@ -85,6 +85,15 @@ interface NetworkResponseData extends BaseNetworkData {
   ok: boolean
 }
 
+export type HTTPMethod =
+  | 'GET'
+  | 'POST'
+  | 'PUT'
+  | 'DELETE'
+  | 'PATCH'
+  | 'HEAD'
+  | 'OPTIONS'
+
 export type NetworkData = NetworkRequestData | NetworkResponseData
 
 export type NetworkSignal = RawSignal<'network', NetworkData>

packages/signals/signals/src/core/client/__tests__/client.test.ts

@@ -49,7 +49,7 @@ describe(SignalsIngestClient, () => {
         data: {
           hello: 'how are you',
         },
-        method: 'post',
+        method: 'POST',
         url: 'http://foo.com',
       },
     })
@@ -63,7 +63,7 @@ describe(SignalsIngestClient, () => {
         "data": {
           "hello": "XXX",
         },
-        "method": "post",
+        "method": "POST",
         "url": "http://foo.com",
       }
     `)

packages/signals/signals/src/core/client/__tests__/redact.test.ts

@@ -93,7 +93,7 @@ describe(redactSignalData, () => {
       {
         contentType: 'application/json',
         action: 'request',
-        method: 'post',
+        method: 'POST',
         url: 'http://foo.com',
         data: { name: 'John Doe', age: 30 },
       },
@@ -103,7 +103,7 @@ describe(redactSignalData, () => {
       {
         contentType: 'application/json',
         action: 'request',
-        method: 'post',
+        method: 'POST',
         url: 'http://foo.com',
         data: { name: 'XXX', age: 999 },
       },

packages/signals/signals/src/core/signal-generators/network-gen/__tests__/network-generator.test.ts

@@ -7,6 +7,7 @@ import { SignalEmitter } from '../../../emitter'
 import { Response } from 'node-fetch'
 import { sleep } from '@internal/test-helpers'
 import { setLocation } from '../../../../test-helpers/set-location'
+import { NetworkSignal } from '@segment/analytics-signals-runtime'
 
 // xhr tests are in integration tests
 describe(NetworkGenerator, () => {
@@ -126,43 +127,54 @@ describe(NetworkGenerator, () => {
     unregister()
   })
 
-  it('should emit response signal but not request signal if content-type for request is not recognized  ', async () => {
+  it('should emit response signal and request signal, regardless of content type', async () => {
     const mockEmitter = { emit: jest.fn() }
     const networkGenerator = new TestNetworkGenerator()
     const unregister = networkGenerator.register(
       mockEmitter as unknown as SignalEmitter
     )
 
     await window.fetch(`/api`, {
-      method: 'GET',
+      method: 'POST',
       headers: { 'content-type': 'text/html' },
       body: 'hello world',
     })
 
     await sleep(100)
-    expect(mockEmitter.emit.mock.calls).toMatchInlineSnapshot(`
+    expect(
+      mockEmitter.emit.mock.calls.flatMap((call) =>
+        call.map((s: NetworkSignal) => s.data.action)
+      )
+    ).toMatchInlineSnapshot(`
       [
-        [
-          {
-            "data": {
-              "action": "response",
-              "contentType": "application/json",
-              "data": {
-                "data": "test",
-              },
-              "ok": true,
-              "status": 200,
-              "url": "http://localhost/api",
-            },
-            "metadata": {
-              "filters": {
-                "allowed": [],
-                "disallowed": [],
-              },
-            },
-            "type": "network",
-          },
-        ],
+        "request",
+        "response",
+      ]
+    `)
+
+    unregister()
+  })
+
+  it('should emit response signal and request signal if no content-type', async () => {
+    const mockEmitter = { emit: jest.fn() }
+    const networkGenerator = new TestNetworkGenerator()
+    const unregister = networkGenerator.register(
+      mockEmitter as unknown as SignalEmitter
+    )
+
+    await window.fetch(`/some-data?foo=123`, {
+      method: 'GET',
+    })
+
+    await sleep(100)
+    expect(
+      mockEmitter.emit.mock.calls.flatMap((call) =>
+        call.map((s: NetworkSignal) => s.data.action)
+      )
+    ).toMatchInlineSnapshot(`
+      [
+        "request",
+        "response",
       ]
     `)
 

packages/signals/signals/src/core/signal-generators/network-gen/index.ts

@@ -12,11 +12,7 @@ import {
   NetworkRequestHandler,
   NetworkResponseHandler,
 } from './network-interceptor'
-import {
-  containsJSONContentType,
-  containsJSONParseableContentType,
-  tryJSONParse,
-} from './helpers'
+import { containsJSONContentType, tryJSONParse } from './helpers'
 
 export type NetworkSettingsConfigSettings = Pick<
   SignalsSettingsConfig,
@@ -58,10 +54,6 @@ export class NetworkGenerator implements SignalGenerator {
     })
 
     const handleRequest: NetworkRequestHandler = (rq) => {
-      if (!containsJSONParseableContentType(rq.headers)) {
-        return
-      }
-
       if (!rq.url || !this.filter.isAllowed(rq.url)) {
         return
       }

packages/signals/signals/src/core/signal-generators/network-gen/network-interceptor.ts

@@ -10,9 +10,22 @@ import {
 let origFetch: typeof window.fetch
 let origXMLHttpRequest: typeof XMLHttpRequest
 
+export type HTTPMethod =
+  | 'GET'
+  | 'POST'
+  | 'PUT'
+  | 'DELETE'
+  | 'PATCH'
+  | 'HEAD'
+  | 'OPTIONS'
+
+const toHTTPMethod = (method: string): HTTPMethod => {
+  return method.toUpperCase() as HTTPMethod
+}
+
 export interface NetworkInterceptorRequest {
   url: string
-  method: string
+  method: HTTPMethod
   contentType: string | undefined
   body: string | undefined
   headers: Headers | undefined
@@ -42,7 +55,7 @@ const createInterceptorRequest = ({
 }: {
   url: URL | string
   body: string | undefined
-  method: string
+  method: HTTPMethod
   headers: Headers | undefined
   id: string
 }): NetworkInterceptorRequest => ({
@@ -130,7 +143,7 @@ export class NetworkInterceptor {
           createInterceptorRequest({
             url: normalizedURL,
             body: typeof options?.body == 'string' ? options.body : undefined,
-            method: options?.method ?? 'GET',
+            method: options?.method ? toHTTPMethod(options.method) : 'GET',
             headers,
             id,
           })
@@ -185,7 +198,7 @@ export class NetworkInterceptor {
     const OrigXMLHttpRequest = window.XMLHttpRequest
     class InterceptedXMLHttpRequest extends OrigXMLHttpRequest {
       _reqURL?: string
-      _reqMethod?: string
+      _reqMethod?: HTTPMethod
       _reqBody?: XMLHttpRequestBodyInit
       _reqHeaders?: Headers
       _reqId = createRequestId()
@@ -263,7 +276,7 @@ export class NetworkInterceptor {
         const [method, url] = args
         try {
           this._reqURL = url.toString()
-          this._reqMethod = method
+          this._reqMethod = toHTTPMethod(method)
         } catch (err) {
           console.log('Error handling request (open)', err)
         }

packages/signals/signals/src/types/__tests__/create-network-signal.test.ts

@@ -21,7 +21,7 @@ describe(createNetworkSignal, () => {
     const data: NetworkData = {
       action: 'request',
       url: 'http://example.com',
-      method: 'post',
+      method: 'POST',
       data: { key: 'value' },
       contentType: 'application/json',
     }

packages/signals/signals/src/types/factories.ts

@@ -65,9 +65,6 @@ export const createNetworkSignal = (
     data: {
       ...data,
       url: normalizeUrl(data.url),
-      ...(data.action === 'request'
-        ? { method: data.method.toUpperCase() }
-        : {}),
     },
     metadata: metadata,
   }