refactor: move redirect logic into handler

nodejs · Jul 19, 2021 · 4b5ed0b · 4b5ed0b
1 parent fd39eb6
commit 4b5ed0b
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 40 deletions.
diff --git a/lib/agent.js b/lib/agent.js
@@ -10,7 +10,6 @@ const Dispatcher = require('./dispatcher')
 const Pool = require('./pool')
 const Client = require('./client')
 const util = require('./core/util')
-const assert = require('assert')
 const RedirectHandler = require('./handler/redirect')
 const { WeakRef, FinalizationRegistry } = require('./compat/dispatcher-weakref')()
 
@@ -125,31 +124,10 @@ class Agent extends Dispatcher {
         this[kFinalizer].register(dispatcher, opts.origin)
       }
 
-      let { maxRedirections = this[kMaxRedirections] } = opts
-
-      if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) {
-        throw new InvalidArgumentError('maxRedirections must be a positive number')
-      }
-
-      if (maxRedirections && util.isStream(opts.body) && util.bodyLength(opts.body) !== 0) {
-        // TODO (fix): Provide some way for the user to cache the file to e.g. /tmp
-        // so that it can be dispatched again?
-        // TODO (fix): Do we need 100-expect support to provide a way to do this properly?
-        maxRedirections = 0
-      }
-
-      if (maxRedirections) {
-        opts = { ...opts, maxRedirections: 0 }
+      const { maxRedirections = this[kMaxRedirections] } = opts
+      if (maxRedirections != null) {
+        opts = { ...opts, maxRedirections: 0 } // Stop sub dispatcher from also redirecting.
         handler = new RedirectHandler(this, maxRedirections, opts, handler)
-
-        /* istanbul ignore next */
-        // TODO (fix): Write a comment why this is needed?
-        if (util.isStream(opts.body)) {
-          opts.body
-            .on('data', function () {
-              assert(false)
-            })
-        }
       }
 
       return dispatcher.dispatch(opts, handler)

diff --git a/lib/client.js b/lib/client.js
@@ -248,18 +248,8 @@ class Client extends Dispatcher {
     }
 
     try {
-      let { maxRedirections = this[kMaxRedirections] } = opts
-
-      if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) {
-        throw new InvalidArgumentError('maxRedirections must be a positive number')
-      }
-
-      if (maxRedirections && util.isStream(opts.body) && util.bodyLength(opts.body) !== 0) {
-        maxRedirections = 0
-      }
-
-      if (maxRedirections) {
-        opts = { ...opts, maxRedirections: 0 }
+      const { maxRedirections = this[kMaxRedirections] } = opts
+      if (maxRedirections != null) {
         handler = new RedirectHandler(this, maxRedirections, opts, handler)
       }
 

diff --git a/lib/handler/redirect.js b/lib/handler/redirect.js
@@ -2,18 +2,38 @@
 
 const util = require('../core/util')
 const assert = require('assert')
+const { InvalidArgumentError } = require('../core/errors')
 
 const redirectableStatusCodes = [300, 301, 302, 303, 307, 308]
 
 class RedirectHandler {
   constructor (dispatcher, maxRedirections, opts, handler) {
+    if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) {
+      throw new InvalidArgumentError('maxRedirections must be a positive number')
+    }
+
     this.dispatcher = dispatcher
     this.location = null
     this.abort = null
-    this.opts = opts
+    this.opts = { ...opts, maxRedirections: 0 } // opts must be a copy
     this.maxRedirections = maxRedirections
     this.handler = handler
     this.history = []
+
+    if (util.isStream(opts.body)) {
+      // TODO (fix): Provide some way for the user to cache the file to e.g. /tmp
+      // so that it can be dispatched again?
+      // TODO (fix): Do we need 100-expect support to provide a way to do this properly?
+      if (util.bodyLength(opts.body) === 0) {
+        // TODO (fix): Write a comment why this is needed?
+        opts.body
+          .on('data', function () {
+            assert(false)
+          })
+      } else {
+        this.maxRedirections = 0
+      }
+    }
   }
 
   onConnect (abort) {
@@ -43,8 +63,6 @@ class RedirectHandler {
     const { origin, pathname, search } = util.parseURL(new URL(this.location, this.opts.origin))
     const path = search ? `${pathname}${search}` : pathname
 
-    this.opts = { ...this.opts }
-
     // Remove headers referring to the original URL.
     // By default it is Host only, unless it's a 303 (see below), which removes also all Content-* headers.
     // https://tools.ietf.org/html/rfc7231#section-6.4