openzipkin / zipkin-ruby

zipkin-tracer ruby gem
Apache License 2.0
99 stars 40 forks source link

Duplicate rack environment strings before calling recognize #160

Closed jvans1 closed 5 years ago

jvans1 commented 5 years ago

Certain gems like routing-filter modify the rack environment in place when recognizing a route. Applications that use this type of behavior depend on recognize only being called once. Third parties that use this call break these types of applications if they call recognize without duping the rack variables.

I couldn't really think of a better way to test this without the custom matcher but I'm all ears if someone has a better way to do it!

jcarres-mdsol commented 5 years ago

I'd ask review from @jfeltesse-mdsol and @ykitamura-mdsol , they may have some idea about the test. Not a nice situation :/

ykitamura-mdsol commented 5 years ago

I can only think of separating the logic to a method and test against it...

e.g.

def self.stub_env(env)
  {
    "PATH_INFO" => env[ZipkinTracer::RackHandler::PATH_INFO].dup,
    "REQUEST_METHOD" => env[ZipkinTracer::RackHandler::REQUEST_METHOD].dup
  }
end
let(:stub_env) { Application.stub_env(env) }

%w(PATH_INFO REQUEST_METHOD).each do |field|
  expect(stub_env[field]).to eq(env[field])
  expect(stub_env[field]).not_to equal(env[field])
end
jvans1 commented 5 years ago

@ykitamura-mdsol good suggestion, thanks. I've updated the PR with your change

jfeltesse-mdsol commented 5 years ago

LGTM