chore: fixed the logic around nil and non-nil checks

client/redirect.go

@@ -70,28 +70,39 @@ func MaxRedirects(max int) CheckRedirect {
 // supplied.  Additionally, any nil checks are skipped.  If all checks
 // are nil, this function also returns nil.
 func NewCheckRedirects(checks ...CheckRedirect) CheckRedirect {
-	if len(checks) == 0 {
+	// skip nils, but check first before making a copy
+	count := 0
+	for _, c := range checks {
+		if c != nil {
+			count++
+		}
+	}
+
+	if count == 0 {
 		return nil
 	}
 
-	// check nils before allocating a copy
+	// now make our safe copy.  this avoids soft memory leaks, since
+	// this slice will be around a while.
+	clone := make([]CheckRedirect, 0, count)
 	for _, c := range checks {
-		if c == nil {
-			return nil
+		if c != nil {
+			clone = append(clone, c)
 		}
 	}
 
-	// optimization:  if there's only (1) check, just use that
-	if len(checks) == 1 {
-		return checks[0]
+	if len(clone) == 1 {
+		// optimization: use the sole non-nil check as is
+		return clone[0]
 	}
 
-	checks = append([]CheckRedirect{}, checks...)
-	return func(request *http.Request, via []*http.Request) (err error) {
-		for i := 0; err == nil && i < len(checks); i++ {
-			err = checks[i](request, via)
+	return func(request *http.Request, via []*http.Request) error {
+		for _, c := range clone {
+			if err := c(request, via); err != nil {
+				return err
+			}
 		}
 
-		return
+		return nil
 	}
 }

client/redirect_test.go

@@ -191,44 +191,70 @@ func (suite *RedirectTestSuite) testNewCheckRedirectsNil() {
 	)
 
 	suite.Nil(
-		NewCheckRedirects(nil, suite.checkRedirectSuccess),
-	)
-
-	suite.Nil(
-		NewCheckRedirects(suite.checkRedirectSuccess, nil, suite.checkRedirectSuccess),
+		NewCheckRedirects(nil, nil, nil),
 	)
 }
 
 func (suite *RedirectTestSuite) testNewCheckRedirectsSuccess() {
-	for _, count := range []int{1, 2, 5} {
-		suite.Run(fmt.Sprintf("count=%d", count), func() {
-			checkRedirect := NewCheckRedirects(
-				suite.checkRedirectSuccesses(count)...,
-			)
+	suite.Run("NoNils", func() {
+		for _, count := range []int{1, 2, 5} {
+			suite.Run(fmt.Sprintf("count=%d", count), func() {
+				checkRedirect := NewCheckRedirects(
+					suite.checkRedirectSuccesses(count)...,
+				)
+
+				suite.Require().NotNil(checkRedirect)
+
+				// won't matter what's passed, as our test functions don't use the parameters
+				suite.NoError(checkRedirect(nil, nil))
+			})
+		}
+	})
 
-			suite.Require().NotNil(checkRedirect)
+	suite.Run("WithNils", func() {
+		checkRedirect := NewCheckRedirects(
+			suite.checkRedirectSuccess,
+			nil,
+			suite.checkRedirectSuccess,
+		)
 
-			// won't matter what's passed, as our test functions don't use the parameters
-			suite.NoError(checkRedirect(nil, nil))
-		})
-	}
+		suite.Require().NotNil(checkRedirect)
+
+		// won't matter what's passed, as our test functions don't use the parameters
+		suite.NoError(checkRedirect(nil, nil))
+	})
 }
 
 func (suite *RedirectTestSuite) testNewCheckRedirectsFail() {
-	for _, count := range []int{1, 2, 5} {
-		suite.Run(fmt.Sprintf("count=%d", count), func() {
-			components := suite.checkRedirectSuccesses(count)
+	suite.Run("NoNils", func() {
+		for _, count := range []int{1, 2, 5} {
+			suite.Run(fmt.Sprintf("count=%d", count), func() {
+				components := suite.checkRedirectSuccesses(count)
 
-			// any fail will fail the entire check
-			components[len(components)/2] = suite.checkRedirectFail
-			checkRedirect := NewCheckRedirects(components...)
+				// any fail will fail the entire check
+				components[len(components)/2] = suite.checkRedirectFail
+				checkRedirect := NewCheckRedirects(components...)
 
-			suite.Require().NotNil(checkRedirect)
+				suite.Require().NotNil(checkRedirect)
 
-			// won't matter what's passed, as our test functions don't use the parameters
-			suite.Error(checkRedirect(nil, nil))
-		})
-	}
+				// won't matter what's passed, as our test functions don't use the parameters
+				suite.Error(checkRedirect(nil, nil))
+			})
+		}
+	})
+
+	suite.Run("WithNils", func() {
+		checkRedirect := NewCheckRedirects(
+			suite.checkRedirectFail,
+			nil,
+			suite.checkRedirectSuccess,
+		)
+
+		suite.Require().NotNil(checkRedirect)
+
+		// won't matter what's passed, as our test functions don't use the parameters
+		suite.Error(checkRedirect(nil, nil))
+	})
 }
 
 func (suite *RedirectTestSuite) TestNewCheckRedirects() {