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.

Sign up for GitHub

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

Jump to bottom

Transfer the request_id from the http_headers to error. #845

Merged
merged 1 commit into from
Sep 4, 2019
Merged

Transfer the request_id from the http_headers to error. #845

merged 1 commit into from
Sep 4, 2019

Conversation

AlicaMoser
Copy link
Contributor

@AlicaMoser AlicaMoser commented Sep 4, 2019 •
edited
Loading

When requesting error#request_id it returned nil although the request id was available in the request headers. With version 5 and switch to Net::HTTP this value is now a string, not a symbol which means that the request_id was not stored on the error correctly.

This picture shows a live request to stripe:

image

@AlicaMoser
Transfer the request_id from the http_headers to error.
10548a4 
When requesting error#request_id it returned nil although the request id was available in the request headers. With version 5 and switch to Net::HTTP this value is now a string, not a symbol which means that the request_id was not stored on the error correctly.
@Senjai
Copy link

Senjai commented Sep 4, 2019

cc @ob-stripe / @brandur-stripe 🙏

@brandur-stripe
Copy link
Contributor

Thanks Alica!

I'll also add a new test in a follow up to make sure that this doesn't regress again.

@brandur-stripe brandur-stripe merged commit b7495eb into stripe:master Sep 4, 2019
brandur-stripe pushed a commit that referenced this pull request Sep 4, 2019
@brandur
Add a test to make sure request IDs make it into error objects
751e49e 
Follows up #845 to make sure that this sort of regression is much more
difficult in the future by adding a test that makes sure a request ID is
threaded all the way from an HTTP response back through to an error
object. I verified that the test failed before #845 came in.
@brandur brandur mentioned this pull request Sep 4, 2019
Merged
brandur-stripe pushed a commit that referenced this pull request Sep 4, 2019
@brandur
Add a test to make sure request IDs make it into error objects
761dc80 
Follows up #845 to make sure that this sort of regression is much more
difficult in the future by adding a test that makes sure a request ID is
threaded all the way from an HTTP response back through to an error
object. I verified that the test failed before #845 came in.
@brandur-stripe
Copy link
Contributor

Released as 5.1.1. Followed up with a new test in #846 so it doesn't happen again. Sorry about the trouble!

brandur-stripe pushed a commit that referenced this pull request Sep 4, 2019
@brandur @brandur-stripe
Add a test to make sure request IDs make it into error objects (#846)
52f64b2 
Follows up #845 to make sure that this sort of regression is much more
difficult in the future by adding a test that makes sure a request ID is
threaded all the way from an HTTP response back through to an error
object. I verified that the test failed before #845 came in.
@shushugah
Copy link

Thank you! This was my issue as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlicaMoser @Senjai @brandur-stripe @shushugah