From 69a02e7046a35153c42240cf89e255e2504b1b32 Mon Sep 17 00:00:00 2001 From: Alex Vondrak Date: Fri, 21 Feb 2020 23:15:04 -0800 Subject: [PATCH] Revamp the Rails integration's tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove extraneous spans that were being generated due to the ActiveSupport::Notifications subscriptions. There's no need to actually configure these from the tests, since we already have the active_support_spec.rb, and it's not like the config in the rails_spec.rb even matched the one from the Rails generator or anything. These just get in the way of testing the Rails integration, since the custom fields we'd want to make assertions about only wind up on the root span anyway. * Factor out assertions into a set of shared examples, and actually start making assertions about the `request.{action,controller,route}` fields that the integration adds (which weren't even being tested for previously 😱). * Rework the "bad request" tests that reproduce #49. These tests will now break against the old implementation, since they assert that routing information is still present (whereas #50 wipes the fields out with error messages). There are ways to get the routing information without ever trying to parse the GET/POST parameters, so we can still save this use case. Furthermore, the only Rails 5+ specific changes are to whether or not Rails responds with an HTTP 400, so we needn't wipe out the entire `describe` block for Rails < 5. * Add tests to reproduce the issue described in #62, where unrecognized routes may cause the old implementation to erroneously fall back to the GET/POST parameters for routing information. * Add tests that reproduce #31. The bug is actually in the twirp gem, whose middleware fails to rewind the `rack.input` after reading it. I've verified these regression tests against the old version of the Rails integration that didn't check `if request.raw_post`, and they fail appropriately. I wanted to have this test in here to have confidence in changing the implementation of the Rails integration. The `raw_post` workaround of #39 is unnecessary if we actually never look at the GET/POST parameters in the first place (no parsing, no problem). --- spec/honeycomb/integrations/rails_spec.rb | 124 +++++++++++++++++----- 1 file changed, 98 insertions(+), 26 deletions(-) diff --git a/spec/honeycomb/integrations/rails_spec.rb b/spec/honeycomb/integrations/rails_spec.rb index 9a0ffc0c..3bb4baaa 100644 --- a/spec/honeycomb/integrations/rails_spec.rb +++ b/spec/honeycomb/integrations/rails_spec.rb @@ -14,10 +14,6 @@ let(:configuration) do Honeycomb::Configuration.new.tap do |config| config.client = libhoney_client - config.notification_events = %w[ - render_template.action_view - process_action.action_controller - ].freeze end end let(:client) { Honeycomb::Client.new(configuration: configuration) } @@ -43,51 +39,127 @@ class TestController < ActionController::Base def hello - render plain: "Hello World!" + render plain: "Hello #{params[:name]}!" + end + end + + shared_examples_for "the rails integration" do + let(:event_data) { libhoney_client.events.map(&:data) } + + it_behaves_like "event data" + + it "sends the right number of events" do + expect(libhoney_client.events.size).to eq 1 + end + + let(:event) { event_data.first } + + it "sends the right request.controller" do + expect(event["request.controller"]).to eq controller + end + + it "sends the right request.action" do + expect(event["request.action"]).to eq action + end + + it "sends the right request.route" do + expect(event["request.route"]).to eq route end end describe "a standard request" do before do - header("Http-Version", "HTTP/1.0") - header("User-Agent", "RackSpec") - - get "/hello/martin" + get "/hello/world" end it "returns ok" do expect(last_response).to be_ok end - it "sends the right number of events" do - expect(libhoney_client.events.size).to eq 3 + include_examples "the rails integration" do + let(:controller) { "test" } + let(:action) { "hello" } + let(:route) { "GET /hello/:name(.:format)" } end + end - let(:event_data) { libhoney_client.events.map(&:data) } + describe "a request with invalid parameter encoding" do + before do + get "/hello/world?via=%c1" + end - it_behaves_like "event data" + if VERSION >= Gem::Version.new("5") + it "returns bad request" do + expect(last_response).to be_bad_request + end + else + it "returns ok" do + expect(last_response).to be_ok + end + end + + include_examples "the rails integration" do + let(:controller) { "test" } + let(:action) { "hello" } + let(:route) { "GET /hello/:name(.:format)" } + end end - if VERSION >= Gem::Version.new("5") - describe "a bad request" do - before do - header("Http-Version", "HTTP/1.0") - header("User-Agent", "RackSpec") + describe "an unrecognized request" do + before do + get "/unrecognized", action: "action", controller: "controller" + end - get "/hello/martin?via=%c1" - end + it "returns not found" do + expect(last_response).to be_not_found + end - it "returns bad request" do - expect(last_response).to be_bad_request + include_examples "the rails integration" do + let(:controller) { nil } + let(:action) { nil } + let(:route) { nil } + end + end + + describe "twirp bug" do + before do + app.routes.draw do + mount TwirpBug, at: "/twirp", via: :post end + end - it "sends the right number of events" do - expect(libhoney_client.events.size).to eq 1 + # Emulates a bug in the twirp gem. + # + # The twirp middleware calls IO#read on the rack.input without rewinding. + # Later, when Rails tries to parse the request parameters by calling + # IO#read(n) with the content length, we get back nil, which blows up the + # default JSON MIME type handler. + # + # @see https://github.com/honeycombio/beeline-ruby/issues/31 + # @see https://github.com/honeycombio/beeline-ruby/pull/39 + # @see https://github.com/twitchtv/twirp-ruby/blob/c8520030b3e4eb584042b0a8db9ae606a3b6c6f4/lib/twirp/service.rb#L138 + module TwirpBug + def self.call(env) + env["rack.input"].read # don't rewind + ::Rack::Response.new("OK").finish end + end - let(:event_data) { libhoney_client.events.map(&:data) } + before do + env "CONTENT_TYPE", "application/json" + env "CONTENT_LENGTH", 17 + env "rack.input", StringIO.new('{"json":"object"}') + post "/twirp" + end + + it "returns ok" do + expect(last_response).to be_ok + end - it_behaves_like "event data" + include_examples "the rails integration" do + let(:controller) { nil } + let(:action) { nil } + let(:route) { "POST /twirp" } end end end