feat: set content-length header #1381

cristiand391 · 2023-11-24T21:10:20Z

feat: automatically set content-length header on REST/SOAP requests.

We've been getting a few reports that some internal envs started to enforce this header on requests, adding support in jsforce as a safety measure.

Tests

while working on this I found the HTTP module doesn't any UT, there's some coverage for session refresh in different e2e tests but we today we don't have an easy way to test this behavior, I'll add some tests for this when working on
[W-14580746: [jsforce]: increase HTTP module test coverage]
(https://gus.lightning.force.com/lightning/r/ADM_Work__c/a07EE00001fXgwbYAC/view)

QA

not really an easy way to QA this without getting one of those internal envs, but you can try running tests with this env var:
JSFORCE_LOG_LEVEL=DEBUG to get the body size added in each request (I tried adding to mock the logger to grab the value and compare it in e2e tests but couldn't get it to work on browser/tests (you can't use jest specific stuff).

@W-14177773@

had to downgrade it because >= v5 is ESM and using dynamic imports causes webpack to create a new chunk just for this pkg.

cristiand391 · 2023-12-01T15:16:16Z

jest.setup.js

+// only retry on CI
+if (process.env.CI) {
+  jest.retryTimes(3, {logErrorsBeforeRetry: true})
+}


retries are annoying when working locally, should always be enabled on CI.

cristiand391 · 2023-12-01T15:39:52Z

package.json

@@ -207,6 +209,7 @@
  "dependencies": {
    "@babel/runtime": "^7.23.1",
    "@babel/runtime-corejs3": "^7.23.1",
+    "@sindresorhus/is": "^4",


there's already a v6:
https://github.com/sindresorhus/is/releases

but had to use v4 because it's the latest CJS release.

When using ESM modules via await import webpack creates a new chunk just for it instead of keeping it in the main bundle:
https://webpack.js.org/guides/code-splitting/#dynamic-imports

I spent a few hours trying to find some workarounds but couldn't get it to inline this into the main bundle so keeping it at v4 until I find a solution for this (will need to for node-fetch v3 (ESM): #1356)

cristiand391 · 2023-12-01T15:42:40Z

src/http-api.ts

+        `missing 'content-length' header, setting it to: ${bodySize}`,
+      );
+      headers['content-length'] = String(bodySize);
+    }


see: https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2:~:text=A%20user%20agent%20SHOULD,anticipate%20such%20a%0A%20%20%20body.

https://github.com/sindresorhus/got/blob/3822412385506a1efef6580d270eae14086b9b43/source/core/index.ts#L618-L629

cristiand391 · 2023-12-01T15:45:21Z

src/soap.ts

+        `missing 'content-length' header, setting it to: ${bodySize}`,
+      );
+      request.headers['content-length'] = String(bodySize);
+    }


same as this but for SOAP requests:
https://github.com/jsforce/jsforce/pull/1381/files#r1412264591

one difference here is that this only checks if method is POST
while GET is possible, I haven't seen any examples of using non-POST in API docs. We can revert it later if it's not true.

cristiand391 · 2023-12-01T15:46:55Z

src/util/get-body-size.ts

+type FormData = {
+  getBoundary: () => string;
+  getLength: (callback: (error: Error | null, length: number) => void) => void;
+} & Readable;


https://github.com/sindresorhus/got/blob/main/source/core/utils/get-body-size.ts

cristiand391 · 2023-12-01T15:49:46Z

src/util/get-body-size.ts

+): number | undefined {
+  function isFormData(body: unknown): body is FormData {
+    return is.nodeStream(body) && is.function_((body as FormData).getBoundary);
+  }


this looks specifically for this FormData, not the web native or node native versions.

we use FormData in jsforce.

cristiand391 · 2023-12-01T15:53:57Z

src/util/get-body-size.ts

+  }
+
+  return undefined;
+}


the only case not handled here is readable streams:

jsforce/src/types/common.ts

Line 17 in 5369d1d

| NodeJS.ReadableStream

got stopped handling this in v12 too, see:
sindresorhus/got#1486
sindresorhus/got#1510

mshanemc · 2023-12-04T19:09:54Z

src/http-api.ts

+          // remove the `content-length` header after token refresh
+          //
+          // SOAP requests include the access token their the body,
+          // if the first req had an invalid token and jsforce successfully
+          // refreshed it we need to remove the `content-length` header
+          // so that it get's re-calculated again with the new body.
+          //
+          // REST request aren't affected by this because the access token
+          // is sent via HTTP headers
+          //
+          //
+          // `_message` is only present in SOAP requests


style nit: I prefer /**/ style for more than like 2 lines of comments

mshanemc · 2023-12-04T19:11:27Z

src/http-api.ts

+            Object.hasOwn(request, '_message') &&
+            request.headers &&
+            request.headers['content-length']


'_message' in request && request.headers?.['content-length']

mshanemc · 2023-12-04T19:21:09Z

src/http-api.ts

+      !cannotHaveBody &&
+      !!request.body &&
+      !headers['transfer-encoding'] &&
+      !headers['content-length'] &&


if 'content-length' is set to 0, does this still do what you want?

mshanemc · 2023-12-04T19:21:41Z

src/soap.ts

+      request.method === 'POST' &&
+      request.headers &&
+      !request.headers['transfer-encoding'] &&
+      !request.headers['content-length'] &&


same question here about 0

mshanemc · 2023-12-04T19:25:40Z

src/util/get-body-size.ts

+import { HttpBody } from '../types';
+import is from '@sindresorhus/is';
+
+export function getBodySize(


+1 for using an exported function. It should be simple to write some true UT, which jsforce lacks, for this.

mshanemc · 2023-12-04T19:36:43Z

src/soap.ts

@@ -233,6 +234,21 @@ export class SOAP<S extends Schema> extends HttpApi<S> {
  /** @override */
  beforeSend(request: HttpRequest & { _message: object }) {


I'm not a fan of this design (pass an object for mutation). I know you didn't start it.

But if http-api isn't exported and is only used via the 2 classes that extend it (soap and bulk?) maybe it's possible to fix it?

Or maybe that's too big and it's easier to stay in line with what jsforce is already doing?

mshanemc · 2024-01-02T15:26:44Z

QA: created a record via sf data create record (uses POST) and JSFORCE_LOG_LEVEL=DEBUG

missing 'content-length' header, setting it to: 18

Seems fine

feat: set content-length header

822eb73

cristiand391 changed the base branch from master to 3.0 November 24, 2023 21:10

cristiand391 added 7 commits November 27, 2023 11:14

fix: downgrade sindresorhus/is

8b15078

had to downgrade it because >= v5 is ESM and using dynamic imports causes webpack to create a new chunk just for this pkg.

fix: no async

63e9f42

fix: form data length

1206f76

fix(soap): handle content-lenght header on session refresh

f67bc8e

test(jest): only retry on CI

7fac95d

fix(rest): other methods can set content-length

11084e6

chore: logging

01a2043

cristiand391 marked this pull request as ready for review December 1, 2023 15:13

cristiand391 commented Dec 1, 2023

View reviewed changes

mshanemc reviewed Dec 4, 2023

View reviewed changes

cristiand391 added 2 commits December 13, 2023 14:56

test: add UT for body size func

c698f1e

fix: check object prop

b440acb

mshanemc approved these changes Dec 15, 2023

View reviewed changes

mshanemc merged commit 442d045 into 3.0 Jan 2, 2024
15 checks passed

cristiand391 deleted the cd/add-http-content-length-header branch January 2, 2024 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: set content-length header #1381

feat: set content-length header #1381

cristiand391 commented Nov 24, 2023 •

edited

cristiand391 Dec 1, 2023

cristiand391 Dec 1, 2023 •

edited

cristiand391 Dec 1, 2023

cristiand391 Dec 1, 2023

cristiand391 Dec 1, 2023

cristiand391 Dec 1, 2023

cristiand391 Dec 1, 2023

mshanemc Dec 4, 2023

mshanemc Dec 4, 2023

mshanemc Dec 4, 2023

mshanemc Dec 4, 2023

mshanemc Dec 4, 2023

mshanemc Dec 4, 2023

mshanemc Dec 4, 2023

mshanemc commented Jan 2, 2024

		@@ -233,6 +234,21 @@ export class SOAP<S extends Schema> extends HttpApi<S> {
		/** @override */
		beforeSend(request: HttpRequest & { _message: object }) {

feat: set content-length header #1381

feat: set content-length header #1381

Conversation

cristiand391 commented Nov 24, 2023 • edited

Tests

QA

Choose a reason for hiding this comment

cristiand391 Dec 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc commented Jan 2, 2024

cristiand391 commented Nov 24, 2023 •

edited

cristiand391 Dec 1, 2023 •

edited