Response contains HTTP code 200 and error in body if client sends an email when disconnected from Internet

[ilya-chumakov]

Issue Summary

When I try to send an email without network connection to remote SMTP server, a response contains 200 OK in StatusCode and HttpRequestException in Body. I think it denies basic HTTP principles and such behaviour is completely unclear. I propose to throw a real exception. If impossible, the code from 4xx-5xx range should be used.

Steps to Reproduce

var sg = new SendGridAPIClient("my_api_key");

Mail mail = new Mail(from, subject, to, content);

//disconnect from Internet before executing this line
var response = await sg.client.mail.send.post(requestBody: mail.Get());

//writes "OK"
Console.WriteLine(response.StatusCode);

//writes ".NET HttpRequestException, raw message: An error occurred while sending the request"
Console.WriteLine(response.Body.ReadAsStringAsync().Result);

Technical details:

• sendgrid-csharp Version: v8.0.5
• .NET Version: 4.5.2

[thinkingserious]

Hi @chumakov-ilya,

Thanks! We agree and I've added this to our backlog for a fix.

[ottomatic]

I could look into submitting a pull request for this if the library maintainers are interested.

[thinkingserious]

Hello @ottomatic,

A PR is always welcome :)

We would just need a signed CLA to accept it.

Thanks!

With Best Regards,

Elmer

[JonCognioDigital]

We've been having huge problems with this too. It's completely misleading. It shouldn't even be an HTTP response because it didn't even get that far. It's just a piece of c# code calling another piece of .net code and should get an appropriate exception. If it does have to be an HTTP response then @chumakov-ilya is correct it should be an error code.

[thinkingserious]

@jon64digital,

Thanks for the feedback, we have added your vote to the issue.

@ottomatic,

Are you still considering a PR? A few days ago we made the CLA process much easier. You no longer need to sign the CLA I linked to previously. When you make your PR, a link will be generated and all you need to do is click the link, fill out a few fields and then sign it by clicking a button.

[ottomatic]

@thinkingserious ,

Yes, still intending to do this.
I have been busy consuming the SMTP Api instead, since our production servers don't allow outgoing http(s) through the firewall - whereas SMTP on port 587 is allowed.

One thing I would like to do in order to make unit testing easier is to take an HttpClient as an injected dependency in the SendGridAPIClient constructor. This would make it possible to inject a custom RequestHandler into the HttpClient, and in that way take control over the http response. What do you think of this change?

[SendGridDX]

Hello @ottomatic,

I suggest you add a third constructor that allows for injecting the custom RequestHandler into the HttpClient.

Thanks!

Elmer

[Jericho]

@ottomatic If you want a "web-proxy-friendly" C# client for the SendGrid API with injectable HttpClient please have a look at StrongGrid (on github and on nuget).

Also, while we are on the topic of mocking HttpClient calls, whether you decide to use StrongGrid or not I highly recommend you have a look at Richard Szalay's Mockhttp for HttpClient (on github and on nuget) which makes mocking HttpClient very easy. I have used it in a few unit testing projects to mock http calls and couldn't be more impressed.

[SendGridDX]

My apologies @ottomatic,

I was thinking of another SDK. We do have support for Web Proxies in this library: https://github.com/sendgrid/sendgrid-csharp/blob/master/src/SendGrid/SendGridClient.cs#L45

@Jericho,

Thanks for the suggestions on mocking HttpClient calls, very cool!

[Jericho]

@SendGridDX the constructor you pointed out does allow injecting IWebProxy but not HttpClient. Therefore it doesn't allow mocking htpp calls which is what @ottomatic wants to achieve.

I did a little research and found a PR submitted in June 2015 to allow httpclient to be injected but unfortunately it wasn't accepted.

[SendGridDX]

Thanks @Jericho!

@ottomatic,

Does this point you in the right direction? If you have any further questions, please don't hesitate to reach out.

[ottomatic]

Yes, the PR which @Jericho refers to is pretty much what I want to accomplish, and then use that to write a unit test in order to set up the throwing of an exception (e.g. a TimeoutException) when performing the httpclient call. I assume the PR was rejected because it was made against the wrong branch and did not comwe with a signed CLA?

So, the easiest path forward for me is do do both the HttpClient injection and the exception handling (i.e. NOT swallowing the exception) in the same PR. Otherwise I will have to wait for one PR to be approved in order to be able to continue with the issue at hand.

@SendGridDX , Is this approach acceptable?

[SendGridDX]

Thank sounds good @ottomatic, thanks!

[ottomatic]

Allright. I have cloned the repo, and have successfully gotten started on the development.

[ottomatic]

Quick update: I am swamped with work. The unit test is ready (and failing), so I should be able to complete the code as soon as I get a few minutes of spare time.

[benmiller86]

In line with making this library more unit test friendly as @ottomatic has suggested. Would there be any interest in exposing an ISendGridClient etc that the existing classes implement?

I for one would find it beneficial to be able to easily mock up a SendGridClient for my own unit tests, but cannot because none of the methods are virtual etc.

My preferred mocking library is Moq, but as things stand I have had to go down the road of using Microsofts Fakes to make a ShimSendGridClient just so I can write tests for specific response codes being returned.

[ottomatic]

@benmiller86 Your suggestion is already formalised in issue #403 and earlier in #201 . I think that would be a good idea and will upvote.

[ottomatic]

OK, so the pull request has been submitted:
#434

This is my first PR ever!
I think the process so far has been rather painless. Good guidance in the contributing.md document regarding which branch to base the PR on, ow to rebase, etc.

[thinkingserious]

Hello Everyone,

Thanks to @ottomatic, this issue is resolved in v9.1.1 via PR #434

[ottomatic]

Awesome, my pleasure.

[kirajhpang]

Hello, does this issue fully resolve ?

I am getting this issue after implement our own SendGrid Log. The OK return on production, while in documentation it said only for sandbox test. So we need resend the email to our client.

[thinkingserious]

Hello @kirajhpang,

I apologize, I don't quite understand the issue. Could you please provide the following details:

1. What are you expecting to happen?
2. Do you have access to the actual status code? (e.g. 202)
3. Can you share the source code?

Thank you!

Elmer

[ottomatic]

This should have been fixed through the merge of PR #434 as per above.

@kirajhpang, are you sure you have the latest version of the nuget package? (I have not tried it myself, so I cannot vouch that the new code has made it through the release pipeline).

[kirajhpang]

@thinkingserious
Hi, here answer for your question

Actually we facing this issue long time ago, since last time v2 api can't send out email. Some of the emails doesn't reach to Tos or Ccs or Bccs, SendGrid Support also cannot tell particular email did or didn't reach to SendGrid endpoint. Everything is just hanging there. After last major update(current production v9.1.1), the status and exception body can be return from SendGrid, we finally can build our own logging module, and found this issue that might be the root cause.

1. What are you expecting to happen?
The email should be send and return as Accepted on production, but we capture Ok with error message in this case. It happen 1/10 on same type of email we sending out.
After cross check status code documentation on here Status Codes & Errors a Ok status shouldn't appear in production api endpoint.
After consult SendGrid support, they refer me on this issue track.

2. Do you have access to the actual status code? (e.g. 202)
I no sure what does you mean access to the actual status code.

3. Can you share the source code?
As suggestion from @ottomatic, will try v9.2.0 , if exception still happen, will reach here again.

Thanks for the effort for building great tool. 😄

[thinkingserious]

Hi @kirajhpang,

Thanks for the follow up and please let me know how v9.2.0 works for you.

With Best Regards,

Elmer