fix: remove broken build request hack (#2874)

Author: ronag

lib/core/request.js

@@ -5,7 +5,6 @@ const {
   NotSupportedError
 } = require('./errors')
 const assert = require('node:assert')
-const { kHTTP2BuildRequest, kHTTP2CopyHeaders, kHTTP1BuildRequest } = require('./symbols')
 const util = require('./util')
 const { channels } = require('./diagnostics.js')
 const { headerNameLowerCasedRecord } = require('./constants')
@@ -149,7 +148,7 @@ class Request {
 
     this.contentType = null
 
-    this.headers = ''
+    this.headers = []
 
     // Only for H2
     this.expectContinue = expectContinue != null ? expectContinue : false
@@ -310,78 +309,10 @@ class Request {
     }
   }
 
-  // TODO: adjust to support H2
   addHeader (key, value) {
     processHeader(this, key, value)
     return this
   }
-
-  static [kHTTP1BuildRequest] (origin, opts, handler) {
-    // TODO: Migrate header parsing here, to make Requests
-    // HTTP agnostic
-    return new Request(origin, opts, handler)
-  }
-
-  static [kHTTP2BuildRequest] (origin, opts, handler) {
-    const headers = opts.headers
-    opts = { ...opts, headers: null }
-
-    const request = new Request(origin, opts, handler)
-
-    request.headers = {}
-
-    if (Array.isArray(headers)) {
-      if (headers.length % 2 !== 0) {
-        throw new InvalidArgumentError('headers array must be even')
-      }
-      for (let i = 0; i < headers.length; i += 2) {
-        processHeader(request, headers[i], headers[i + 1], true)
-      }
-    } else if (headers && typeof headers === 'object') {
-      const keys = Object.keys(headers)
-      for (let i = 0; i < keys.length; i++) {
-        const key = keys[i]
-        processHeader(request, key, headers[key], true)
-      }
-    } else if (headers != null) {
-      throw new InvalidArgumentError('headers must be an object or an array')
-    }
-
-    return request
-  }
-
-  static [kHTTP2CopyHeaders] (raw) {
-    const rawHeaders = raw.split('\r\n')
-    const headers = {}
-
-    for (const header of rawHeaders) {
-      const [key, value] = header.split(': ')
-
-      if (value == null || value.length === 0) continue
-
-      if (headers[key]) {
-        headers[key] += `,${value}`
-      } else {
-        headers[key] = value
-      }
-    }
-
-    return headers
-  }
-}
-
-function processHeaderValue (key, val, skipAppend) {
-  if (val && typeof val === 'object') {
-    throw new InvalidArgumentError(`invalid ${key} header`)
-  }
-
-  val = val != null ? `${val}` : ''
-
-  if (headerCharRegex.exec(val) !== null) {
-    throw new InvalidArgumentError(`invalid ${key} header`)
-  }
-
-  return skipAppend ? val : `${key}: ${val}\r\n`
 }
 
 function processHeader (request, key, val, skipAppend = false) {
@@ -400,10 +331,39 @@ function processHeader (request, key, val, skipAppend = false) {
     }
   }
 
-  if (request.host === null && headerName === 'host') {
+  if (Array.isArray(val)) {
+    const arr = []
+    for (let i = 0; i < val.length; i++) {
+      if (typeof val[i] === 'string') {
+        if (headerCharRegex.exec(val[i]) !== null) {
+          throw new InvalidArgumentError(`invalid ${key} header`)
+        }
+        arr.push(val[i])
+      } else if (val[i] === null) {
+        arr.push('')
+      } else if (typeof val[i] === 'object') {
+        throw new InvalidArgumentError(`invalid ${key} header`)
+      } else {
+        arr.push(`${val[i]}`)
+      }
+    }
+    val = arr
+  } else if (typeof val === 'string') {
     if (headerCharRegex.exec(val) !== null) {
       throw new InvalidArgumentError(`invalid ${key} header`)
     }
+  } else if (val === null) {
+    val = ''
+  } else if (typeof val === 'object') {
+    throw new InvalidArgumentError(`invalid ${key} header`)
+  } else {
+    val = `${val}`
+  }
+
+  if (request.host === null && headerName === 'host') {
+    if (typeof val !== 'string') {
+      throw new InvalidArgumentError('invalid host header')
+    }
     // Consumed by Client
     request.host = val
   } else if (request.contentLength === null && headerName === 'content-length') {
@@ -413,35 +373,22 @@ function processHeader (request, key, val, skipAppend = false) {
     }
   } else if (request.contentType === null && headerName === 'content-type') {
     request.contentType = val
-    if (skipAppend) request.headers[key] = processHeaderValue(key, val, skipAppend)
-    else request.headers += processHeaderValue(key, val)
+    request.headers.push(key, val)
   } else if (headerName === 'transfer-encoding' || headerName === 'keep-alive' || headerName === 'upgrade') {
     throw new InvalidArgumentError(`invalid ${headerName} header`)
   } else if (headerName === 'connection') {
     const value = typeof val === 'string' ? val.toLowerCase() : null
     if (value !== 'close' && value !== 'keep-alive') {
       throw new InvalidArgumentError('invalid connection header')
-    } else if (value === 'close') {
+    }
+
+    if (value === 'close') {
       request.reset = true
     }
   } else if (headerName === 'expect') {
     throw new NotSupportedError('expect header not supported')
-  } else if (Array.isArray(val)) {
-    for (let i = 0; i < val.length; i++) {
-      if (skipAppend) {
-        if (request.headers[key]) {
-          request.headers[key] += `,${processHeaderValue(key, val[i], skipAppend)}`
-        } else {
-          request.headers[key] = processHeaderValue(key, val[i], skipAppend)
-        }
-      } else {
-        request.headers += processHeaderValue(key, val[i])
-      }
-    }
-  } else if (skipAppend) {
-    request.headers[key] = processHeaderValue(key, val, skipAppend)
   } else {
-    request.headers += processHeaderValue(key, val)
+    request.headers.push(key, val)
   }
 }
 

lib/core/symbols.js

@@ -56,9 +56,6 @@ module.exports = {
   kMaxResponseSize: Symbol('max response size'),
   kHTTP2Session: Symbol('http2Session'),
   kHTTP2SessionState: Symbol('http2Session state'),
-  kHTTP2BuildRequest: Symbol('http2 build request'),
-  kHTTP1BuildRequest: Symbol('http1 build request'),
-  kHTTP2CopyHeaders: Symbol('http2 copy headers'),
   kRetryHandlerDefaultRetry: Symbol('retry agent default retry'),
   kConstruct: Symbol('constructable'),
   kListeners: Symbol('listeners'),

lib/core/util.js

@@ -516,6 +516,7 @@ kEnumerableProperty.enumerable = true
 module.exports = {
   kEnumerableProperty,
   nop,
+
   isDisturbed,
   isErrored,
   isReadable,

lib/dispatcher/client-h1.js

@@ -817,12 +817,12 @@ function writeH1 (client, request) {
 
     const [bodyStream, contentType] = extractBody(body)
     if (request.contentType == null) {
-      headers += `content-type: ${contentType}\r\n`
+      headers.push('content-type', contentType)
     }
     body = bodyStream.stream
     contentLength = bodyStream.length
   } else if (util.isBlobLike(body) && request.contentType == null && body.type) {
-    headers += `content-type: ${body.type}\r\n`
+    headers.push('content-type', body.type)
   }
 
   if (body && typeof body.read === 'function') {
@@ -922,8 +922,19 @@ function writeH1 (client, request) {
     header += 'connection: close\r\n'
   }
 
-  if (headers) {
-    header += headers
+  if (Array.isArray(headers)) {
+    for (let n = 0; n < headers.length; n += 2) {
+      const key = headers[n + 0]
+      const val = headers[n + 1]
+
+      if (Array.isArray(val)) {
+        for (let i = 0; i < val.length; i++) {
+          header += `${key}: ${val[i]}\r\n`
+        }
+      } else {
+        header += `${key}: ${val}\r\n`
+      }
+    }
   }
 
   if (channels.sendHeaders.hasSubscribers) {

lib/dispatcher/client-h2.js

@@ -3,7 +3,6 @@
 const assert = require('node:assert')
 const { pipeline } = require('node:stream')
 const util = require('../core/util.js')
-const Request = require('../core/request.js')
 const {
   RequestContentLengthMismatchError,
   RequestAbortedError,
@@ -26,7 +25,6 @@ const {
   // HTTP2
   kMaxConcurrentStreams,
   kHTTP2Session,
-  kHTTP2CopyHeaders,
   kResume
 } = require('../core/symbols.js')
 
@@ -215,10 +213,6 @@ function writeH2 (client, request) {
   const session = client[kHTTP2Session]
   const { body, method, path, host, upgrade, expectContinue, signal, headers: reqHeaders } = request
 
-  let headers
-  if (typeof reqHeaders === 'string') headers = Request[kHTTP2CopyHeaders](reqHeaders.trim())
-  else headers = reqHeaders
-
   if (upgrade) {
     errorRequest(client, request, new Error('Upgrade not supported for H2'))
     return false
@@ -228,6 +222,24 @@ function writeH2 (client, request) {
     return false
   }
 
+  const headers = {}
+  for (let n = 0; n < reqHeaders.length; n += 2) {
+    const key = reqHeaders[n + 0]
+    const val = reqHeaders[n + 1]
+
+    if (Array.isArray(val)) {
+      for (let i = 0; i < val.length; i++) {
+        if (headers[key]) {
+          headers[key] += `,${val[i]}`
+        } else {
+          headers[key] = val[i]
+        }
+      }
+    } else {
+      headers[key] = val
+    }
+  }
+
   /** @type {import('node:http2').ClientHttp2Stream} */
   let stream
 

lib/dispatcher/client.js

@@ -60,8 +60,6 @@ const {
   kHTTPContext,
   // HTTP2
   kMaxConcurrentStreams,
-  kHTTP2BuildRequest,
-  kHTTP1BuildRequest,
   kResume
 } = require('../core/symbols.js')
 const connectH1 = require('./client-h1.js')
@@ -296,12 +294,7 @@ class Client extends DispatcherBase {
 
   [kDispatch] (opts, handler) {
     const origin = opts.origin || this[kUrl].origin
-
-    // TODO (fix): Why do these need to be
-    // TODO (fix): This can happen before connect...
-    const request = this[kHTTPContext]?.version === 'h2'
-      ? Request[kHTTP2BuildRequest](origin, opts, handler)
-      : Request[kHTTP1BuildRequest](origin, opts, handler)
+    const request = new Request(origin, opts, handler)
 
     this[kQueue].push(request)
     if (this[kResuming]) {

test/node-test/diagnostics-channel/get.js

@@ -32,9 +32,9 @@ test('Diagnostics channel - get', (t) => {
     assert.equal(request.completed, false)
     assert.equal(request.method, 'GET')
     assert.equal(request.path, '/')
-    assert.equal(request.headers, 'bar: bar\r\n')
+    assert.deepStrictEqual(request.headers, ['bar', 'bar'])
     request.addHeader('hello', 'world')
-    assert.equal(request.headers, 'bar: bar\r\nhello: world\r\n')
+    assert.deepStrictEqual(request.headers, ['bar', 'bar', 'hello', 'world'])
   })
 
   let _connector
@@ -81,7 +81,7 @@ test('Diagnostics channel - get', (t) => {
       'hello: world'
     ]
 
-    assert.equal(headers, expectedHeaders.join('\r\n') + '\r\n')
+    assert.deepStrictEqual(headers, expectedHeaders.join('\r\n') + '\r\n')
   })
 
   diagnosticsChannel.channel('undici:request:headers').subscribe(({ request, response }) => {

test/node-test/diagnostics-channel/post-stream.js

@@ -33,9 +33,9 @@ test('Diagnostics channel - post stream', (t) => {
     assert.equal(request.completed, false)
     assert.equal(request.method, 'POST')
     assert.equal(request.path, '/')
-    assert.equal(request.headers, 'bar: bar\r\n')
+    assert.deepStrictEqual(request.headers, ['bar', 'bar'])
     request.addHeader('hello', 'world')
-    assert.equal(request.headers, 'bar: bar\r\nhello: world\r\n')
+    assert.deepStrictEqual(request.headers, ['bar', 'bar', 'hello', 'world'])
     assert.deepStrictEqual(request.body, body)
   })
 

test/node-test/diagnostics-channel/post.js

@@ -31,9 +31,9 @@ test('Diagnostics channel - post', (t) => {
     assert.equal(request.completed, false)
     assert.equal(request.method, 'POST')
     assert.equal(request.path, '/')
-    assert.equal(request.headers, 'bar: bar\r\n')
+    assert.deepStrictEqual(request.headers, ['bar', 'bar'])
     request.addHeader('hello', 'world')
-    assert.equal(request.headers, 'bar: bar\r\nhello: world\r\n')
+    assert.deepStrictEqual(request.headers, ['bar', 'bar', 'hello', 'world'])
     assert.deepStrictEqual(request.body, Buffer.from('hello world'))
   })
 
@@ -81,7 +81,7 @@ test('Diagnostics channel - post', (t) => {
       'hello: world'
     ]
 
-    assert.equal(headers, expectedHeaders.join('\r\n') + '\r\n')
+    assert.deepStrictEqual(headers, expectedHeaders.join('\r\n') + '\r\n')
   })
 
   diagnosticsChannel.channel('undici:request:headers').subscribe(({ request, response }) => {