Treat only 2xx as a successful upload

Every one of these SDKs treated a 3xx as a failure before this work, and the
change to 200-399 came from the design doc's "Spec item 1: 2xx and 3xx are
success". That line is wrong, and the doc is what needs correcting.

Measured against a local server, with the same HTTP clients these SDKs use:

  307/308 + Location  -> followed as POST with the body, arrives as 200
  301/302/303 + Loc.  -> followed as GET with no body, arrives as 200
  302 without Location-> surfaces raw as 302
  300 Multiple Choices-> surfaces raw as 300
  304 Not Modified    -> surfaces raw as 304

So a raw 3xx only reaches the classifier when the client has already declined to
follow it, meaning nothing was uploaded. The one redirect that genuinely works,
307/308, never produces a 3xx here at all — it produces 200 — so narrowing the
bound cannot break it. Nothing was gained by the wider range; a 300, 304, or
Location-less 302 from a proxy was being logged as a delivered batch and dropped
with no error callback.

The narrower bound also needs no new branches: a 3xx is neither 5xx nor in the
retryable 4xx set, so it already falls through to the non-retryable path and
reports a failure.

TAPI does not emit 3xx and has no plans to. This matters because host is
customer-configurable and proxies in front of it are common.

Net::HTTP never follows redirects, so ruby is the most likely to see a raw 3xx, and logging the body was useless for one. It now logs the status and points at the host. Specs and the misnamed "3xx is treated as success" context corrected. ClassLength raised 150 -> 155 for the added branch; comments do not count toward it.

Author: MichaelGHSeg

.rubocop_todo.yml

@@ -73,7 +73,7 @@ Metrics/BlockLength:
 # Offense count: 1
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 150
+  Max: 155
 
 # Offense count: 2
 # Configuration parameters: AllowedMethods, AllowedPatterns, IgnoredMethods.

lib/segment/analytics/response.rb

@@ -14,8 +14,7 @@ def initialize(status = 200, error = nil)
       end
 
       def success?
-        # Spec item 1: 2xx and 3xx are success.
-        status >= 200 && status < 400
+        status >= 200 && status < 300
       end
     end
   end

lib/segment/analytics/transport.rb

@@ -81,7 +81,13 @@ def final_response(status_code, body, error)
         return Response.new(status_code, error) if success_status?(status_code)
         return nil if retryable_status?(status_code)
 
-        logger.error(body)
+        if status_code >= 300 && status_code < 400
+          # Logging the body here would be useless: a redirect has none.
+          logger.error("Unexpected redirect (#{status_code}); batch not uploaded. " \
+                       'Check whether the configured host points at a proxy or redirector.')
+        else
+          logger.error(body)
+        end
         Response.new(status_code, error)
       end
 
@@ -144,9 +150,10 @@ def parse_error(body)
         nil
       end
 
+      # Only 2xx. Net::HTTP does not follow redirects, so a 3xx means nothing was
+      # uploaded; calling it success would drop the batch silently.
       def success_status?(code)
-        # Spec item 1: 2xx and 3xx are success.
-        code >= 200 && code < 400
+        code >= 200 && code < 300
       end
 
       def retryable_status?(code)

spec/segment/analytics/response_spec.rb

@@ -17,8 +17,10 @@ class Analytics
         it { expect(described_class.new(200, nil).success?).to be true }
         it { expect(described_class.new(201, nil).success?).to be true }
         it { expect(described_class.new(204, nil).success?).to be true }
-        it { expect(described_class.new(301, nil).success?).to be true }
-        it { expect(described_class.new(302, nil).success?).to be true }
+        it { expect(described_class.new(300, nil).success?).to be false }
+        it { expect(described_class.new(301, nil).success?).to be false }
+        it { expect(described_class.new(302, nil).success?).to be false }
+        it { expect(described_class.new(304, nil).success?).to be false }
         it { expect(described_class.new(400, nil).success?).to be false }
         it { expect(described_class.new(429, nil).success?).to be false }
         it { expect(described_class.new(500, nil).success?).to be false }

spec/segment/analytics/transport_spec.rb

@@ -205,11 +205,13 @@ class Analytics
             end
           end
 
-          context '3xx is treated as success' do
+          context '3xx is not retried and is not success' do
             let(:status_code) { 301 }
-            it 'returns status without retrying' do
+            it 'returns the status without retrying, and does not report success' do
               expect(subject).not_to receive(:sleep)
-              expect(subject.send(write_key, batch).status).to eq(301)
+              response = subject.send(write_key, batch)
+              expect(response.status).to eq(301)
+              expect(response.success?).to be false
             end
           end
 
@@ -417,9 +419,10 @@ class Analytics
             describe '#success_status?' do
               it { expect(subject.__send__(:success_status?, 200)).to be true }
               it { expect(subject.__send__(:success_status?, 201)).to be true }
-              # Spec item 1: 2xx and 3xx are success.
-              it { expect(subject.__send__(:success_status?, 301)).to be true }
-              it { expect(subject.__send__(:success_status?, 304)).to be true }
+              # Only 2xx: Net::HTTP does not follow redirects, so a 3xx means
+              # nothing was uploaded.
+              it { expect(subject.__send__(:success_status?, 301)).to be false }
+              it { expect(subject.__send__(:success_status?, 304)).to be false }
               it { expect(subject.__send__(:success_status?, 400)).to be false }
               it { expect(subject.__send__(:success_status?, 500)).to be false }
             end