Merge pull request #50 from airbnb/darnaut--misc-fixes

Misc fixes

.rspec

@@ -0,0 +1 @@
+--require spec_helper

Gemfile

@@ -5,12 +5,17 @@ gem 'zk',                    '~> 1.9.3'
 gem 'unicorn',               '~> 4.8.3'
 gem 'unicorn-worker-killer', '~> 0.4.4'
 gem 'hash-deep-merge',       '~> 0.1.1'
-gem 'oj',                    '= 3.3.2'
+gem 'oj',                    '= 3.13.18'
 gem 'stomp',                 '~> 1.3.2'
 gem 'dogstatsd-ruby',        '= 3.3.0'
-gem 'aws-sdk-sqs',           '= 1.3.0'
+gem 'aws-sdk-sqs',           '= 1.51.1'
 gem 'get_process_mem',       '= 0.2.1'
 
 group :ddtrace do
   gem 'ddtrace', '~> 0.45.0'
 end
+
+group :test do
+  gem 'rspec',      '~> 3.11.0'
+  gem 'rack-test',  '~> 2.0.2'
+end

optica.rb

@@ -91,9 +91,9 @@ def get_nodes(request, fields_to_include=nil)
 
   post '/' do
     begin
-      data = Oj.load(request.body.read, :mode => :strict)
-    rescue JSON::ParserError
-      data = {}
+      data = Oj.safe_load(request.body.read)
+    rescue Oj::ParseError
+      halt(400, 'invalid request payload')
     end
 
     ip = data['ip']

spec/optica_spec.rb

@@ -0,0 +1,73 @@
+require 'spec_helper'
+require './optica.rb'
+
+RSpec.describe Optica do
+  def app
+    Optica
+  end
+
+  before(:all) do
+    Optica.set :logger, double('TestLogger')
+  end
+
+  describe '/ping' do
+    it 'returns PONG' do
+      get '/ping'
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('PONG')
+    end
+  end
+
+  describe '/' do
+    let(:data) {
+      {
+        'ip' => '127.0.0.1',
+        'id' => 'test',
+        'environment' => 'development',
+      }
+    }
+    let(:object_data) {
+      {
+        'ip' => '127.0.0.1',
+        'test' => Object.new,
+      }
+    }
+
+    before(:all) do
+      Optica.set :ip_check, :test
+    end
+
+    before(:each) do
+      Optica.set :store, double('TestStore')
+      event = double('TestEvent')
+      allow(event).to receive(:name).and_return('TestEvent')
+      Optica.set :events, [event]
+      statsd = double('TestStatsd')
+      allow(statsd).to receive(:increment)
+      stub_const('STATSD', statsd)
+    end
+
+    it 'can post data' do
+      expect(Optica.store).to receive(:add).with(data['ip'], data).and_return(data)
+      expect(Optica.events.first).to receive(:send)
+      post('/', Oj.dump(data), 'CONTENT_TYPE' => 'application/json')
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('stored')
+    end
+
+    it 'does not load objects' do
+      loaded_data = nil
+      allow(Optica.store).to receive(:add) do |ip, data|
+        loaded_data = data
+      end
+      post('/', Oj.dump(object_data), 'CONTENT_TYPE' => 'application/json')
+      expect(loaded_data).not_to be_nil
+      expect(loaded_data['test']).to be_a(Hash)
+    end
+
+    it 'rejects invalid JSON data' do
+      post('/', '{', 'CONTENT_TYPE' => 'application/json')
+      expect(last_response.status).to eq(400)
+    end
+  end
+end

spec/spec_helper.rb

@@ -0,0 +1,101 @@
+# This file was generated by the `rspec --init` command. Conventionally, all
+# specs live under a `spec` directory, which RSpec adds to the `$LOAD_PATH`.
+# The generated `.rspec` file contains `--require spec_helper` which will cause
+# this file to always be loaded, without a need to explicitly require it in any
+# files.
+#
+# Given that it is always loaded, you are encouraged to keep this file as
+# light-weight as possible. Requiring heavyweight dependencies from this file
+# will add to the boot time of your test suite on EVERY test run, even for an
+# individual file that may not need all of that loaded. Instead, consider making
+# a separate helper file that requires the additional dependencies and performs
+# the additional setup, and require it from the spec files that actually need
+# it.
+#
+# See https://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration
+
+require 'rspec'
+require 'rack/test'
+
+RSpec.configure do |config|
+  # rspec-expectations config goes here. You can use an alternate
+  # assertion/expectation library such as wrong or the stdlib/minitest
+  # assertions if you prefer.
+  config.expect_with :rspec do |expectations|
+    # This option will default to `true` in RSpec 4. It makes the `description`
+    # and `failure_message` of custom matchers include text for helper methods
+    # defined using `chain`, e.g.:
+    #     be_bigger_than(2).and_smaller_than(4).description
+    #     # => "be bigger than 2 and smaller than 4"
+    # ...rather than:
+    #     # => "be bigger than 2"
+    expectations.include_chain_clauses_in_custom_matcher_descriptions = true
+  end
+
+  # rspec-mocks config goes here. You can use an alternate test double
+  # library (such as bogus or mocha) by changing the `mock_with` option here.
+  config.mock_with :rspec do |mocks|
+    # Prevents you from mocking or stubbing a method that does not exist on
+    # a real object. This is generally recommended, and will default to
+    # `true` in RSpec 4.
+    mocks.verify_partial_doubles = true
+  end
+
+  # This option will default to `:apply_to_host_groups` in RSpec 4 (and will
+  # have no way to turn it off -- the option exists only for backwards
+  # compatibility in RSpec 3). It causes shared context metadata to be
+  # inherited by the metadata hash of host groups and examples, rather than
+  # triggering implicit auto-inclusion in groups with matching metadata.
+  config.shared_context_metadata_behavior = :apply_to_host_groups
+
+  # This allows you to limit a spec run to individual examples or groups
+  # you care about by tagging them with `:focus` metadata. When nothing
+  # is tagged with `:focus`, all examples get run. RSpec also provides
+  # aliases for `it`, `describe`, and `context` that include `:focus`
+  # metadata: `fit`, `fdescribe` and `fcontext`, respectively.
+  config.filter_run_when_matching :focus
+
+  # Allows RSpec to persist some state between runs in order to support
+  # the `--only-failures` and `--next-failure` CLI options. We recommend
+  # you configure your source control system to ignore this file.
+  config.example_status_persistence_file_path = "spec/examples.txt"
+
+  # Limits the available syntax to the non-monkey patched syntax that is
+  # recommended. For more details, see:
+  # https://relishapp.com/rspec/rspec-core/docs/configuration/zero-monkey-patching-mode
+  config.disable_monkey_patching!
+
+  # This setting enables warnings. It's recommended, but in some cases may
+  # be too noisy due to issues in dependencies.
+  config.warnings = true
+
+  # Many RSpec users commonly either run the entire suite or an individual
+  # file, and it's useful to allow more verbose output when running an
+  # individual spec file.
+  if config.files_to_run.one?
+    # Use the documentation formatter for detailed output,
+    # unless a formatter has already been configured
+    # (e.g. via a command-line flag).
+    config.default_formatter = "doc"
+  end
+
+  # Print the 10 slowest examples and example groups at the
+  # end of the spec run, to help surface which specs are running
+  # particularly slow.
+  config.profile_examples = 10
+
+  # Run specs in random order to surface order dependencies. If you find an
+  # order dependency and want to debug it, you can fix the order by providing
+  # the seed, which is printed after each run.
+  #     --seed 1234
+  config.order = :random
+
+  # Seed global randomization in this process using the `--seed` CLI option.
+  # Setting this allows you to use `--seed` to deterministically reproduce
+  # test failures related to randomization by passing the same `--seed` value
+  # as the one that triggered the failure.
+  Kernel.srand config.seed
+
+  # Include testing API for Rack apps.
+  config.include Rack::Test::Methods
+end

store.rb

@@ -174,7 +174,7 @@ def load_instances_from_leader
       uri = "http://localhost:%d/store" % @opts['split_mode_store_port']
       res = open(uri, :read_timeout => @opts['split_mode_http_timeout'])
 
-      remote_store = Oj.load(res.read, :mode => :strict)
+      remote_store = Oj.safe_load(res.read)
       [ remote_store['inst'], remote_store['idx'] ]
     rescue OpenURI::HTTPError, Errno::ECONNREFUSED, Net::ReadTimeout => e
       @log.error "Error loading store from #{uri}: #{e.inspect}; will retry after #{@opts['split_mode_retry_delay']}"
@@ -322,12 +322,12 @@ def get_node(node)
         @zk.get(node)
       end
       STATSD.time('optica.json.parse') do
-        Oj.load(data, :mode => :strict)
+        Oj.safe_load(data)
       end
     rescue ZK::Exceptions::NoNode
       @log.info "node #{node} disappeared"
       {}
-    rescue JSON::ParserError
+    rescue Oj::ParseError
       @log.warn "removing invalid node #{node}: data failed to parse (#{data.inspect})"
       delete(node)
       {}