Skip to content

Return fault status code on thrown HTTP client exceptions #20

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

Closed
wants to merge 1 commit into from

Conversation

davidhagg
Copy link

Return status code 500 for exceptions thrown by HTTP client and include underlying reason in the response.

This will make it clearer for the consumer of the api to handle fault responses and see the causes of underlying issues.

@thinkingserious
Copy link
Contributor

Hello @davidhagg,

Thank you for the PR!

We will be reviewing this PR soon, in the mean time, if you have not, please be sure to sign our CLA so that we can merge your changes. Thanks again!

Team DX

@thinkingserious thinkingserious added status: code review request requesting a community code review or review from Twilio status: cla needed labels Jan 9, 2017
@davidhagg davidhagg mentioned this pull request Jan 9, 2017
@davidhagg
Copy link
Author

I won't be able to send in the CLA requested, but you could look at this code as inspiration and implement it in a similar fasion :-)

@Tarball
Copy link

Tarball commented Jan 19, 2017

I'm not sure this is the ideal solution. It's faking an HTTP 500 response, even though no such response was actually received from the server. It also still hides the original exception (exception message can be seen in the response content, but can't be caught by code using this library).

@thinkingserious
Copy link
Contributor

Thank you @davidhagg!

I believe we will pursue the solution proposed by @Tarball here: #6

Do you have any thoughts on that proposal?

Thanks!

@davidhagg
Copy link
Author

Hi @thinkingserious yes I'm fine with that solution. The only thing is that it is a breaking change so just make sure to bump the version correctly.

Thanks!

@davidhagg
Copy link
Author

Hello @thinkingserious, do you have an update on the progress of fixing this issue? We are having intermittent issues with calling SendGrid, but we cannot see the underlying cause for this with the current code.

@thinkingserious
Copy link
Contributor

Hi @davidhagg,

This issue is fairly low on our backlog for the following reasons:

  1. There are not many votes for this issue
  2. There is no PR with a CLA signed
  3. This is not the official SDK to connect with the SendGrid API. You can find it here. That SDK used to depend on this repo, but no longer, so that we have static typing.

If 1 & 2 gain traction, that will help push it up our backlog for implementation.

I hope this information helps.

Thanks!

With Best Regards,

Elmer

@davidhagg
Copy link
Author

Ok, thanks.
Had a look at (3) and we were using the wrong lib there.
However, this issue exists in that lib as well, you do have at least one issue about it
sendgrid/sendgrid-csharp#358

@thinkingserious
Copy link
Contributor

Thanks @davidhagg!

I've added your vote to that issue to help move it up our backlog.

@thinkingserious
Copy link
Contributor

Hi @davidhagg,

This issue is now fixed: sendgrid/sendgrid-csharp#434

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants