Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a cleaner temporary request object #129

Merged
merged 2 commits into from
Nov 15, 2018

Conversation

yellowred
Copy link
Contributor

Using Rails.application.routes.router.recognize is not safe as it updates the passed argument, which is request in our case. Specifically it updates rails_req.script_name from "" to "/" which makes request.path to have double slash in the beginning. To solve this issue it has been created a temporary clean request object which will be disposed later.

See also: https://github.com/openzipkin/zipkin-ruby/pull/122/files (PR that has the same line before).

Using `Rails.application.routes.router.recognize` is not safe as it updates the passed argument, which is request in our case. Specifically it updates `rails_req.script_name` from `""` to `"/"` which makes `request.path` to have double slash in the beginning. To solve this issue it has been created a temporary clean request object which will be disposed later.

See also: https://github.com/openzipkin/zipkin-ruby/pull/122/files (PR that has the same line before).
@yellowred
Copy link
Contributor Author

Related to this issue:
#128

@jcarres-mdsol
Copy link
Collaborator

@yellowred fix looks good, can you make it something like

env = {
"PATH_INFO" => env[ZipkinTracer::RackHandler::PATH_INFO], 
"REQUEST_METHOD" => env[ZipkinTracer::RackHandler::REQUEST_METHOD]
}

so we don't have long lines? Github not happy about that

@jcarres-mdsol
Copy link
Collaborator

Also, please update version and changelog

@ykitamura-mdsol
Copy link
Contributor

Looks good to me 👍

@jcarres-mdsol jcarres-mdsol merged commit d92d521 into openzipkin:master Nov 15, 2018
@jcarres-mdsol
Copy link
Collaborator

Thanks!

@yellowred yellowred deleted the bugfix/extra-slash branch November 21, 2018 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants