Refactor/http io uncoupling (#1752)

* Simplify HTTPIO top level class

* Reduce complexity of building class

* Rename / organisation of Buffer methods2

* Reduce ABC Score of HTTP IO spec massively

* Split out triple class into 3 single classes

* Partially port spec code for io_http_buffer

* DRY up rspec shared contexts correctly

* Move webrick alias handler to isolated support file for rspec

* Fix require path issues and after hook problem

* Split up of final pieces of spec and partition correctly - remove excess crud

* Clean up some fake objects

* Place server archetype in subject and remove https scheme alteration

* Moved received headers into the mock class

* Remove redundant assign of a localhost entity

* Move body IO into mock server class

* Move starting of server into before hook of shared context

* Re-generate TODO file now a lot of the porting is complete

* Fix up a couple of RSpec named offenses that could slip through

* Move shellwords requirement to correct location and refactor excess complexity of initial build methods

* Add changelog

Author: luke-hill

.rubocop_todo.yml

@@ -1,14 +1,39 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2024-01-17 12:23:33 UTC using RuboCop version 1.56.4.
+# on 2024-02-01 10:58:34 UTC using RuboCop version 1.56.4.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
 # TODO - [LH] -> Oct '23 - 355 files inspected, 642 offenses detected, 205 offenses autocorrectable
 # TODO - [LH] -> Dec '23 - 350 files inspected, 595 offenses detected, 171 offenses autocorrectable
-# TODO - [LH] -> Jan '23 (cleaned up most of the config) - 364 files inspected, 713 offenses detected, 132 offenses autocorrectable
+# TODO - [LH] -> Feb '23 - 370 files inspected, 635 offenses detected, 166 offenses autocorrectable
+
+# Offense count: 10
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: AllowMultipleStyles, EnforcedHashRocketStyle, EnforcedColonStyle, EnforcedLastArgumentHashStyle.
+# SupportedHashRocketStyles: key, separator, table
+# SupportedColonStyles: key, separator, table
+# SupportedLastArgumentHashStyles: always_inspect, always_ignore, ignore_implicit, ignore_explicit
+Layout/HashAlignment:
+  Exclude:
+    - 'lib/cucumber/cli/options.rb'
+
+# Offense count: 19
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: AllowInHeredoc.
+Layout/TrailingWhitespace:
+  Exclude:
+    - 'lib/cucumber/multiline_argument/data_table.rb'
+
+# Offense count: 3
+# This cop supports safe autocorrection (--autocorrect).
+Lint/AmbiguousOperator:
+  Exclude:
+    - 'lib/cucumber/multiline_argument/data_table.rb'
+    - 'lib/cucumber/running_test_case.rb'
+    - 'spec/cucumber/formatter/spec_helper.rb'
 
 # Offense count: 4
 Lint/RescueException:
@@ -118,6 +143,7 @@ RSpec/ContextWording:
     - 'spec/cucumber/formatter/junit_spec.rb'
     - 'spec/cucumber/formatter/publish_banner_printer_spec.rb'
     - 'spec/cucumber/glue/step_definition_spec.rb'
+    - 'spec/support/shared_context/http_server.rb'
 
 # Offense count: 8
 # This cop supports unsafe autocorrection (--autocorrect-all).
@@ -144,7 +170,7 @@ RSpec/EmptyLineAfterFinalLet:
     - 'spec/cucumber/configuration_spec.rb'
     - 'spec/cucumber/hooks_spec.rb'
 
-# Offense count: 83
+# Offense count: 82
 # Configuration parameters: CountAsOne.
 RSpec/ExampleLength:
   Max: 58
@@ -182,19 +208,19 @@ RSpec/ExpectOutput:
 RSpec/HookArgument:
   Enabled: false
 
-# Offense count: 15
+# Offense count: 8
 # Configuration parameters: AssignmentOnly.
 RSpec/InstanceVariable:
   Exclude:
-    - 'spec/cucumber/formatter/http_io_spec.rb'
+    - 'spec/support/shared_context/http_server.rb'
 
 # Offense count: 5
 # Configuration parameters: EnforcedStyle.
 # SupportedStyles: have_received, receive
 RSpec/MessageSpies:
   Exclude:
     - 'spec/cucumber/deprecate_spec.rb'
-    - 'spec/cucumber/formatter/http_io_spec.rb'
+    - 'spec/cucumber/formatter/io_http_buffer_spec.rb'
     - 'spec/cucumber/runtime/hooks_examples.rb'
 
 # Offense count: 15
@@ -310,6 +336,15 @@ Style/ClassVars:
   Exclude:
     - 'spec/cucumber/glue/step_definition_spec.rb'
 
+# Offense count: 2
+# This cop supports unsafe autocorrection (--autocorrect-all).
+# Configuration parameters: EnforcedStyle.
+# SupportedStyles: always, always_true, never
+Style/FrozenStringLiteralComment:
+  Exclude:
+    - 'spec/support/shared_context/http_server.rb'
+    - 'spec/support/webrick_proc_handler_alias.rb'
+
 # Offense count: 7
 # Configuration parameters: AllowedVariables.
 Style/GlobalVars:

CHANGELOG.md

@@ -12,6 +12,9 @@ Please visit [cucumber/CONTRIBUTING.md](https://github.com/cucumber/cucumber/blo
 ### Changed
 - Updated cucumber dependencies (Specifically cucumber-core) ([luke-hill](https://github.com/luke-hill))
 
+- Uncoupled a lot of dual-responsibility complexity in HTTP classes (Specifically the builders/parsers)
+  ([#1752](https://github.com/cucumber/cucumber-ruby/pull/1750) [luke-hill](https://github.com/luke-hill))
+
 ### Removed
 - Some legacy JRuby local testing profiles are now removed ([luke-hill](https://github.com/luke-hill))
 

lib/cucumber/formatter/curl_option_parser.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'shellwords'
+
+module Cucumber
+  module Formatter
+    class CurlOptionParser
+      def self.parse(options)
+        args = Shellwords.split(options)
+
+        url = nil
+        http_method = 'PUT'
+        headers = {}
+
+        until args.empty?
+          arg = args.shift
+          case arg
+          when '-X', '--request'
+            http_method = remove_arg_for(args, arg)
+          when '-H'
+            header_arg = remove_arg_for(args, arg)
+            headers = headers.merge(parse_header(header_arg))
+          else
+            raise StandardError, "#{options} was not a valid curl command. Can't set url to #{arg} it is already set to #{url}" if url
+
+            url = arg
+          end
+        end
+        raise StandardError, "#{options} was not a valid curl command" unless url
+
+        [url, http_method, headers]
+      end
+
+      # TODO: [LH] -> Switch below methods to private
+      def self.remove_arg_for(args, arg)
+        return args.shift unless args.empty?
+
+        raise StandardError, "Missing argument for #{arg}"
+      end
+
+      def self.parse_header(header_arg)
+        parts = header_arg.split(':', 2)
+        raise StandardError, "#{header_arg} was not a valid header" unless parts.length == 2
+
+        { parts[0].strip => parts[1].strip }
+      end
+    end
+  end
+end

lib/cucumber/formatter/http_io.rb

@@ -2,152 +2,18 @@
 
 require 'net/http'
 require 'tempfile'
-require 'shellwords'
+require_relative 'curl_option_parser'
+require_relative 'io_http_buffer'
 
 module Cucumber
   module Formatter
     class HTTPIO
-      class << self
-        # Returns an IO that will write to a HTTP request's body
-        # https_verify_mode can be set to OpenSSL::SSL::VERIFY_NONE
-        # to ignore unsigned certificate - setting to nil will verify the certificate
-        def open(url, https_verify_mode = nil, reporter = nil)
-          @https_verify_mode = https_verify_mode
-          uri, method, headers = CurlOptionParser.parse(url)
-          IOHTTPBuffer.new(uri, method, headers, https_verify_mode, reporter)
-        end
-      end
-    end
-
-    class CurlOptionParser
-      def self.parse(options)
-        args = Shellwords.split(options)
-
-        url = nil
-        http_method = 'PUT'
-        headers = {}
-
-        until args.empty?
-          arg = args.shift
-          case arg
-          when '-X', '--request'
-            http_method = remove_arg_for(args, arg)
-          when '-H'
-            header_arg = remove_arg_for(args, arg)
-            headers = headers.merge(parse_header(header_arg))
-          else
-            raise StandardError, "#{options} was not a valid curl command. Can't set url to #{arg} it is already set to #{url}" if url
-
-            url = arg
-          end
-        end
-        raise StandardError, "#{options} was not a valid curl command" unless url
-
-        [
-          url,
-          http_method,
-          headers
-        ]
-      end
-
-      def self.remove_arg_for(args, arg)
-        return args.shift unless args.empty?
-
-        raise StandardError, "Missing argument for #{arg}"
-      end
-
-      def self.parse_header(header_arg)
-        parts = header_arg.split(':', 2)
-        raise StandardError, "#{header_arg} was not a valid header" unless parts.length == 2
-
-        { parts[0].strip => parts[1].strip }
-      end
-    end
-
-    class IOHTTPBuffer
-      attr_reader :uri, :method, :headers
-
-      def initialize(uri, method, headers = {}, https_verify_mode = nil, reporter = nil)
-        @uri = URI(uri)
-        @method = method
-        @headers = headers
-        @write_io = Tempfile.new('cucumber', encoding: 'UTF-8')
-        @https_verify_mode = https_verify_mode
-        @reporter = reporter || NoReporter.new
-      end
-
-      def close
-        response = send_content(@uri, @method, @headers)
-        @reporter.report(response.body)
-        @write_io.close
-        return if response.is_a?(Net::HTTPSuccess) || response.is_a?(Net::HTTPRedirection)
-
-        raise StandardError, "request to #{uri} failed with status #{response.code}"
-      end
-
-      def write(data)
-        @write_io.write(data)
-      end
-
-      def flush
-        @write_io.flush
-      end
-
-      def closed?
-        @write_io.closed?
-      end
-
-      private
-
-      def send_content(uri, method, headers, attempt = 10)
-        content = (method == 'GET' ? StringIO.new : @write_io)
-        http = build_client(uri, @https_verify_mode)
-
-        raise StandardError, "request to #{uri} failed (too many redirections)" if attempt <= 0
-
-        req = build_request(
-          uri,
-          method,
-          headers.merge(
-            'Content-Length' => content.size.to_s
-          )
-        )
-
-        content.rewind
-        req.body_stream = content
-
-        begin
-          response = http.request(req)
-        rescue SystemCallError
-          # We may get the redirect response before pushing the file.
-          response = http.request(build_request(uri, method, headers))
-        end
-
-        case response
-        when Net::HTTPAccepted
-          send_content(URI(response['Location']), 'PUT', {}, attempt - 1) if response['Location']
-        when Net::HTTPRedirection
-          send_content(URI(response['Location']), method, headers, attempt - 1)
-        end
-        response
-      end
-
-      def build_request(uri, method, headers)
-        method_class_name = "#{method[0].upcase}#{method[1..].downcase}"
-        req = Net::HTTP.const_get(method_class_name).new(uri)
-        headers.each do |header, value|
-          req[header] = value
-        end
-        req
-      end
-
-      def build_client(uri, https_verify_mode)
-        http = Net::HTTP.new(uri.hostname, uri.port)
-        if uri.scheme == 'https'
-          http.use_ssl = true
-          http.verify_mode = https_verify_mode if https_verify_mode
-        end
-        http
+      # Returns an IO that will write to a HTTP request's body
+      # https_verify_mode can be set to OpenSSL::SSL::VERIFY_NONE
+      # to ignore unsigned certificate - setting to nil will verify the certificate
+      def self.open(url, https_verify_mode = nil, reporter = nil)
+        uri, method, headers = CurlOptionParser.parse(url)
+        IOHTTPBuffer.new(uri, method, headers, https_verify_mode, reporter)
       end
     end
   end