Refactor (#186)

bhornseth · codygustafson · web-flow · commit 0c8b5df40c5c · 2024-12-20T06:37:07.000-07:00
* Eliminate test warnings Each of these changes were causing a longer version of the below warning > WARNING: Using the `raise_error` matcher without providing a specific error > or message risks false positives There are definitely better refactors for each of these, but I'm optimizing for cleaning up the test output, not for improving the code interface or possibly introducing breaking changes * Ecosystem Updates - Add support for Ruby 3.2 and 3.3 - Require Faraday 2.0+ * Add Verbose option This change turns off request and response logging by default and allows users to turn that back on by setting a new `verbose` option to true. * Clean up GitHub Actions script - only build on some pushes, but all pull requests - support manual builds via workflow_dispatch - change publish to be triggered by a GitHub release action - update to latest actions/checkout script * BREAKING: remove `compress` option This change removes the `compress` option from the client entirely. We use the default faraday adapter, which is `net/http` from stdlib. If you read [the documentation for that module][net-http-docs], you'll come across this: > `Net::HTTP` automatically adds Accept-Encoding for compression of > response bodies and automatically decompresses gzip and deflate > responses unless a Range header was sent. In other words, by default, ruby's net/http module will request gzipped content and automatically uncompress it if that's what the response contains. If instead one sets the `Accept-Encoding` header on a request that's made with `net/http`, that default behavior is ignored and we're left to deal with unpacking hte response ourselves as was done in the `FaradayMiddleware` class. In other words, whether the `compress` option is used or not, the behavior has been that we're asking for compressed responses and the option is effectively ignored. I went back to ruby 1.9.2 and the same behavior existed there. Given that, I'm confident this option has always been either ignored or redundant. [net-http-docs]: https://ruby-doc.org/stdlib-3.0.0/libdoc/net/http/rdoc/Net/HTTP.html#class-Net::HTTP-label-Compression * Remove highline dependency Highline is only used within this cli provider, and only to capture and do some minor formatting on the user input. This changeset removes highline entirely and replaces it with equivalent ruby * remove broken Code Climate badge * Update reso_faraday_middleware.rb The new Faraday::Middleware class does not have a `body` method from the response and requires us to retrieve it from the `env` variable. --------- Co-authored-by: Cody Gustafson <cgustafson@fbsdata.com>
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -1,6 +1,13 @@
 name: CI
 
-on: [push, pull_request]
+on:
+  push:
+    branches: 'master'
+  pull_request:
+  workflow_dispatch:
+  release:
+    types: published
+
 
 jobs:
   build:
@@ -9,11 +16,11 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        ruby: [ '2.7', '3.0', '3.1' ]
+        ruby: [ '3.1', '3.2', '3.3' ]
 
     steps:
       - name: repo checkout
-        uses: actions/checkout@v2
+        uses: actions/checkout@v4
 
       - name: Set up Ruby ${{ matrix.ruby }}
         # https://github.com/ruby/setup-ruby
@@ -22,7 +29,7 @@ jobs:
           ruby-version: ${{ matrix.ruby }}
 
       - name: Set up Bundler
-        run: gem install bundler -v 2.1.4
+        run: gem install bundler
 
 
       - name: bundle install
@@ -36,19 +43,19 @@ jobs:
     runs-on: ubuntu-latest
 
     # only run if we pushed a tag
-    if: startsWith(github.ref, 'refs/tags/v')
+    if: github.event_name == 'release'
 
     # require that the build matrix passed
     needs: build
 
     steps:
       - name: repo checkout
-        uses: actions/checkout@v2
+        uses: actions/checkout@v4
 
       - name: Set up Ruby
         uses: ruby/setup-ruby@v1
         with:
-          ruby-version: '2.7'
+          ruby-version: '3.3'
 
       - name: bundle install
         run: |
diff --git a/.ruby-version b/.ruby-version
@@ -1 +1 @@
-2.7.2
+3.2.3
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,3 +1,12 @@
+v2.0.0
+ - Drop support for ruby 2.7
+ - Add support for ruby 3.2+
+ - Require minimum version of faraday 2.0
+ - Disabled request/response logging by default. Old behavior can be restored
+   by setting a new `verbose` configuration option to true
+ - BREAKING: remove `compress` from configuration. gzip compression is now
+   always enabled
+
 v1.6.3
  - Add support for sending end user ip address in request headers
 
diff --git a/README.md b/README.md
@@ -1,6 +1,6 @@
 Spark API
 =====================
-![CI](https://github.com/sparkapi/spark_api/workflows/CI/badge.svg) ![Code Climate](https://codeclimate.com/badge.png)
+![CI](https://github.com/sparkapi/spark_api/workflows/CI/badge.svg) 
 
 A Ruby wrapper for the Spark REST API. Loosely based on ActiveResource to provide models to interact with remote services.
 
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-1.6.3
+2.0.0
diff --git a/lib/spark_api/authentication/oauth2_impl/cli_provider.rb b/lib/spark_api/authentication/oauth2_impl/cli_provider.rb
@@ -1,5 +1,3 @@
-require "highline"
-
 module SparkApi
   module Authentication
     module OAuth2Impl
@@ -18,9 +16,11 @@ def initialize(credentials)
         def redirect(url)
           puts "Missing OAuth2 session, redirecting..."
           puts "Please visit #{url}, login as a user, and paste the authorization code here:"
-          self.code = HighLine.ask("Authorization code?") do |q|
-            q.whitespace = :strip_and_collapse
-            q.validate = /^\w+$/
+          puts "Authorization code?"
+          raw_code = gets.strip
+
+          unless raw_code.match?(/^\w+$/)
+            raise "Invalid authorization code. Please try again."
           end
         end
                 
diff --git a/lib/spark_api/authentication/oauth2_impl/faraday_middleware.rb b/lib/spark_api/authentication/oauth2_impl/faraday_middleware.rb
@@ -6,7 +6,7 @@ module OAuth2Impl
   
       #==OAuth2 Faraday response middleware
       # HTTP Response after filter to package oauth2 responses and bubble up basic api errors.
-      class FaradayMiddleware < Faraday::Response::Middleware
+      class FaradayMiddleware < Faraday::Middleware
 
         def initialize(app)
           super(app)
@@ -42,7 +42,7 @@ def on_complete(env)
       
       #==OAuth2 Faraday response middleware
       # HTTP Response after filter to package oauth2 responses and bubble up basic api errors.
-      class SparkbarFaradayMiddleware < Faraday::Response::Middleware
+      class SparkbarFaradayMiddleware < Faraday::Middleware
   
         def initialize(app)
           super(app)
diff --git a/lib/spark_api/configuration.rb b/lib/spark_api/configuration.rb
@@ -9,9 +9,9 @@ module Configuration
     end
     
     # valid configuration options
-    VALID_OPTION_KEYS = [:api_key, :api_secret, :api_user, :endpoint, 
-      :user_agent, :version, :ssl, :ssl_verify, :oauth2_provider, :authentication_mode, 
-      :auth_endpoint, :callback, :compress, :timeout, :middleware, :dictionary_version, :request_id_chain, :user_ip_address].freeze
+    VALID_OPTION_KEYS = [:api_key, :api_secret, :api_user, :endpoint,
+      :user_agent, :version, :ssl, :ssl_verify, :oauth2_provider, :authentication_mode,
+      :auth_endpoint, :callback, :timeout, :middleware, :dictionary_version, :request_id_chain, :user_ip_address, :verbose].freeze
     OAUTH2_KEYS = [:authorization_uri, :access_uri, :client_id, :client_secret,
       # Requirements for authorization_code grant type
       :redirect_uri,  
@@ -41,12 +41,12 @@ module Configuration
     DEFAULT_SSL = true
     DEFAULT_SSL_VERIFY = true
     DEFAULT_OAUTH2 = nil
-    DEFAULT_COMPRESS = false
     DEFAULT_TIMEOUT = 5 # seconds
     DEFAULT_MIDDLEWARE = 'spark_api'
     DEFAULT_DICTIONARY_VERSION = nil
     DEFAULT_REQUEST_ID_CHAIN = nil
     DEFAULT_USER_IP_ADDRESS = nil
+    DEFAULT_VERBOSE = false
     
     X_SPARK_API_USER_AGENT = "X-SparkApi-User-Agent"
     X_USER_IP_ADDRESS = "X-User-IP-Address"
@@ -79,12 +79,12 @@ def reset_configuration
       self.ssl         = DEFAULT_SSL
       self.ssl_verify  = DEFAULT_SSL_VERIFY
       self.version     = DEFAULT_VERSION
-      self.compress    = DEFAULT_COMPRESS
       self.timeout     = DEFAULT_TIMEOUT
       self.middleware = DEFAULT_MIDDLEWARE
       self.dictionary_version = DEFAULT_DICTIONARY_VERSION
       self.request_id_chain = DEFAULT_REQUEST_ID_CHAIN
       self.user_ip_address = DEFAULT_USER_IP_ADDRESS
+      self.verbose = DEFAULT_VERBOSE
       self
     end
   end
diff --git a/lib/spark_api/configuration/yaml.rb b/lib/spark_api/configuration/yaml.rb
@@ -60,7 +60,7 @@ def self.config_keys()
       end
       
       def self.exists?(name)
-        File.exists? "#{config_path}/#{name}.yml"
+        File.exist? "#{config_path}/#{name}.yml"
       end
 
       def self.build(name)
diff --git a/lib/spark_api/connection.rb b/lib/spark_api/connection.rb
@@ -10,7 +10,6 @@ module Connection
     HTTP_SCHEME = 'http:'
     HTTPS_SCHEME = 'https:'
     ACCEPT_ENCODING = 'Accept-Encoding'
-    COMPRESS_ACCEPT_ENCODING = 'gzip, deflate'
     X_REQUEST_ID_CHAIN = 'X-Request-Id-Chain'
     MIME_JSON = 'application/json'
     MIME_RESO = 'application/json, application/xml'
@@ -27,10 +26,6 @@ def connection(force_ssl = false)
         opts[:url] = @endpoint.sub REG_HTTPS, HTTP_SCHEME
       end
 
-      if self.compress
-        opts[:headers][ACCEPT_ENCODING] = COMPRESS_ACCEPT_ENCODING
-      end
-
       if request_id_chain
         opts[:headers][X_REQUEST_ID_CHAIN] = request_id_chain
       end
@@ -40,7 +35,9 @@ def connection(force_ssl = false)
         conn.options[:timeout] = self.timeout
         conn.adapter Faraday.default_adapter
       end
-      SparkApi.logger.debug { "Connection: #{conn.inspect}" }
+      if self.verbose
+        SparkApi.logger.debug { "Connection: #{conn.inspect}" }
+      end
       conn
     end
     
diff --git a/lib/spark_api/faraday_middleware.rb b/lib/spark_api/faraday_middleware.rb
@@ -5,7 +5,7 @@
 module SparkApi
   #=Spark API Faraday middleware
   # HTTP Response after filter to package api responses and bubble up basic api errors.
-  class FaradayMiddleware < Faraday::Response::Middleware
+  class FaradayMiddleware < Faraday::Middleware
     include SparkApi::PaginateHelper      
     
     def initialize(app)
@@ -15,10 +15,10 @@ def initialize(app)
     # Handles pretty much all the api response parsing and error handling.  All responses that
     # indicate a failure will raise a SparkApi::ClientError exception
     def on_complete(env)
-      env[:body] = decompress_body(env)
-
       body = MultiJson.decode(env[:body])
-      SparkApi.logger.debug{ "Response Body: #{body.inspect}" }
+      if SparkApi.verbose
+        SparkApi.logger.debug{ "Response Body: #{body.inspect}" }
+      end
       unless body.is_a?(Hash) && body.key?("D")
         raise InvalidResponse, "The server response could not be understood"
       end
@@ -80,18 +80,6 @@ def on_complete(env)
       env[:body] = results
     end
 
-    def decompress_body(env)
-      encoding = env[:response_headers]['content-encoding'].to_s.downcase
-
-      if encoding == 'gzip'
-        env[:body] = Zlib::GzipReader.new(StringIO.new(env[:body])).read
-      elsif encoding == 'deflate'
-        env[:body] = Zlib::Inflate.inflate(env[:body])
-      end
-
-      env[:body]
-    end
-
     private
 
     def http_method_override_request?(env)
diff --git a/lib/spark_api/request.rb b/lib/spark_api/request.rb
@@ -93,7 +93,9 @@ def request(method, path, body, options)
           response = authenticator.request(method, request_path, nil, request_opts)
         else
           post_data = process_request_body(body)
-          SparkApi.logger.debug { "#{method.to_s.upcase} Data:   #{post_data}" }
+          if self.verbose
+            SparkApi.logger.debug { "#{method.to_s.upcase} Data:   #{post_data}" }
+          end
           response = authenticator.request(method, request_path, post_data, request_opts)
         end
         request_time = Time.now - start_time
diff --git a/lib/spark_api/reso_faraday_middleware.rb b/lib/spark_api/reso_faraday_middleware.rb
@@ -1,14 +1,9 @@
 
 module SparkApi
-
   class ResoFaradayMiddleware < FaradayMiddleware
-
     def on_complete(env)
-
-      body = decompress_body(env)
-
       begin
-        body = MultiJson.decode(body)
+        body = MultiJson.decode(env[:body])
 
         if body["D"]
           super(env)
@@ -21,9 +16,7 @@ def on_complete(env)
         # some minor format verification
         raise e if body.strip[/\A<\?xml/].nil?
       end
-
     end
-
   end
 
   Faraday::Response.register_middleware :reso_api => ResoFaradayMiddleware
diff --git a/spark_api.gemspec b/spark_api.gemspec
@@ -31,12 +31,10 @@ Gem::Specification.new do |s|
   s.require_paths      = ["lib"]
 
   s.add_dependency 'addressable'
-  s.add_dependency 'faraday', '>= 0.17.3', '< 2.0'
-  s.add_dependency 'multi_json', '~> 1.0'
+  s.add_dependency 'faraday', '>= 2.0.0'
+  s.add_dependency 'multi_json', '> 1.0'
   s.add_dependency 'json', '>= 1.7'
-  s.add_dependency 'builder', '>= 2.1.2', '< 4.0.0'
   s.add_dependency 'will_paginate', '>= 3.0.pre2', '< 4.0.0'
-  s.add_dependency 'highline', '>= 1.0'
 
   # TEST GEMS
   s.add_development_dependency 'rake'
@@ -45,10 +43,7 @@ Gem::Specification.new do |s|
   s.add_development_dependency 'rexml'  #needed for ruby 3
   s.add_development_dependency 'typhoeus'
   s.add_development_dependency 'ci_reporter_rspec'
-  # s.add_development_dependency 'rb-readline'
-  # s.add_development_dependency 'rb-fsevent'
-  # s.add_development_dependency 'simplecov'
+  s.add_development_dependency 'builder', '>= 2.1.2', '< 4.0.0'
   s.add_development_dependency 'simplecov-rcov'
-  # s.add_development_dependency 'guard-rspec'
 end
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -23,7 +23,7 @@
 
 require 'spark_api'
 
-FileUtils.mkdir 'log' unless File.exists? 'log'
+FileUtils.mkdir 'log' unless File.exist? 'log'
 
 module SparkApi
   def self.logger
diff --git a/spec/unit/spark_api/configuration/yaml_spec.rb b/spec/unit/spark_api/configuration/yaml_spec.rb
@@ -27,9 +27,9 @@
     end
     it "should raise an error for a bad configuration" do
       allow(subject).to receive(:env){ {} }
-      expect { subject.load_file("spec/config/spark_api/some_random_key.yml")}.to raise_error
+      expect { subject.load_file("spec/config/spark_api/some_random_key.yml")}.to raise_error(Errno::ENOENT)
       allow(subject).to receive(:env){ {"RAILS_ENV" => "fake_env"} }
-      expect { subject.load_file(api_file)}.to raise_error
+      expect { subject.load_file(api_file)}.to raise_error(NoMethodError) 
     end
   end
   describe "oauth2" do
diff --git a/spec/unit/spark_api/configuration_spec.rb b/spec/unit/spark_api/configuration_spec.rb
@@ -16,6 +16,7 @@
       expect(SparkApi.request_id_chain).to be_nil
       expect(SparkApi.user_ip_address).to be_nil
       expect(SparkApi.middleware).to eq('spark_api')
+      expect(SparkApi.verbose).to be false
     end
   end
 
@@ -28,7 +29,8 @@
                                     :endpoint => "http://api.wade.dev.fbsdata.com",
                                     :timeout => 15,
                                     :request_id_chain => 'foobar',
-                                    :user_ip_address => 'barfoo')
+                                    :user_ip_address => 'barfoo',
+                                    :verbose => true)
  
       expect(client.api_key).to match("key_of_wade")
       expect(client.api_secret).to match("TopSecret")
@@ -39,6 +41,7 @@
       expect(client.timeout).to eq(15)
       expect(client.request_id_chain).to eq('foobar')
       expect(client.user_ip_address).to eq('barfoo')
+      expect(client.verbose).to be true
     end
     
     it "should allow unverified ssl certificates when verification is off" do
@@ -101,6 +104,7 @@
         config.endpoint = "test.api.sparkapi.com"
         config.user_agent = "my useragent"
         config.timeout = 15
+        config.verbose = true
       end
 
       expect(SparkApi.api_key).to match("my_key")
@@ -111,6 +115,7 @@
       expect(SparkApi.user_agent).to match("my useragent")
       expect(SparkApi.oauth2_enabled?()).to be false
       expect(SparkApi.timeout).to eq(15)
+      expect(SparkApi.verbose).to be true
     end
 
     it "should correctly set up the client for oauth2" do
@@ -223,12 +228,6 @@
       expect(c.connection.headers["Accept-Encoding"]).to be_nil
     end
 
-    it "should set gzip header if compress option is set" do
-      c = SparkApi::Client.new(:endpoint => "https://api.sparkapi.com",
-        :compress => true) 
-      expect(c.connection.headers["Accept-Encoding"]).to eq("gzip, deflate")
-    end
-
     it "should set default timeout of 5 seconds" do
       c = SparkApi::Client.new(:endpoint => "https://sparkapi.com")
       expect(c.connection.options[:timeout]).to eq(5)
diff --git a/spec/unit/spark_api/faraday_middleware_spec.rb b/spec/unit/spark_api/faraday_middleware_spec.rb
diff --git a/spec/unit/spark_api/multi_client_spec.rb b/spec/unit/spark_api/multi_client_spec.rb
diff --git a/spec/unit/spark_api/request_spec.rb b/spec/unit/spark_api/request_spec.rb
