Upgrade Error Handling Functionality

[KaptenJon]

Issue Summary

I override System.Net.ServicePointManager.ServerCertificateValidationCallback (in this case not in a good way) and sendgrids certificate was treated as a bad certificate. When I user SendGridClient it throws away the underlying exception replying with a HttpResponse OK leaving the developer with no clue. Using the nuget package makes it even harder though I can't step through the code.

I suggest to return an other http response statuscode and include the exception messege (maybe also inner exception message) with the returned response.

Steps to Reproduce

1 Override System.Net.ServicePointManager.ServerCertificateValidationCallback =
(sender, certificate, chain, errors) =>
{ return false;}
2. Use sendgridclient and see your ok message (but now send email)

Technical details:

[thinkingserious]

Hello @KaptenJon,

Currently, you can retrieve the error messages as follows: https://github.com/sendgrid/sendgrid-csharp/blob/master/TROUBLESHOOTING.md#error and the status codes we return are: https://sendgrid.com/docs/API_Reference/Web_API_v3/Mail/errors.html

Unfortunately, I'm not quite sure how to reproduce your error. I have not tried overriding System.Net.ServicePointManager.ServerCertificateValidationCallback. Do you mind providing some additional code so I can try to reproduce?

With Best Regards,

Elmer

[KaptenJon]

Overriding is the wrong word maybe :-) do System.Net.ServicePointManager.ServerCertificateValidationCallback = and in the method you provide return false. I used dotnetcore if that matter for the specific sendgridclient. You can also see in the source code, as I tried to explain, that the exception is not provided to the user but only a very general text,and ok status code, making it hard for the user of the lib. Skickat från min Sony Xperia™-smartphone

…

---- Elmer Thomas skrev ---- Hello @KaptenJon<https://github.com/KaptenJon>, Currently, you can retrieve the error messages as follows: https://github.com/sendgrid/sendgrid-csharp/blob/master/TROUBLESHOOTING.md#error and the status codes we return are: https://sendgrid.com/docs/API_Reference/Web_API_v3/Mail/errors.html Unfortunately, I'm not quite sure how to reproduce your error. I have not tried overriding System.Net.ServicePointManager.ServerCertificateValidationCallback. Do you mind providing some additional code so I can try to reproduce? With Best Regards, Elmer — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#404 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKQ1Sp1NZDMGs7_kTIN3hHswpgo6KTDDks5rew4-gaJpZM4MG-8E>.

[thinkingserious]

Hi @KaptenJon,

If there is an exception, we current include the exception message in the response content, like so: https://github.com/sendgrid/sendgrid-csharp/blob/master/src/SendGrid/SendGridClient.cs#L377, then you can access the error message like this: Console.WriteLine(response.Body.ReadAsStringAsync().Result);

Does that help?

[KaptenJon]

Hi, I actually totally missed that reading the code :)
However it is not enough this is what the message in the exception say: "An error occurred while sending the request."
Thats very general, the inner message though: "The underlying connection was closed: Could not establish trust relationship for the SSL/TLS secure channel."
and next level even better...

And I also still think the status code shouldent be ok. I am not an expert in http responses but how about HttpStatusCode.Unauthorized

[KaptenJon]

     response.Content = new StringContent(message + ex.Message + "\n\n" +ex.InnerException?.Message + "\n\n" + ex.InnerException?.InnerException?.Message);

[thinkingserious]

Ah, thanks for the additional details!

We need to refactor our error handling. I will leave this ticket open to track the progress and I've added this to our backlog.

I'm changing the title of this issue to reflect it's new purpose.

Thanks!

[KaptenJon]

No thanks to you, great work! Skickat från min Sony Xperia™-smartphone

…

---- Elmer Thomas skrev ---- Ah, thanks for the additional details! We need to refactor our error handling. I will leave this ticket open to track the progress and I've added this to our backlog. I'm changing the title of this issue to reflect it's new purpose. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#404 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKQ1SiW-c3Qq3ikFdv2ZFL6jQTJz-ENCks5rfdRjgaJpZM4MG-8E>.

[AElvik]

Error handling should not return OK if it isn't. I'd prefer if the exception was not caught so i could handle different cases in my code.

As a workaround I added some code that looks for ".NET HttpRequestException" or ".NET Exception" in the result if status is OK, then i consider it an exception and adds some retry.

[KaptenJon]

Maybe a better workaround for you is to download the source code for the class. There are no dependecies to other parts and then simple to modify. Skickat från min Sony Xperia™-smartphone

…

---- Anders Elvik skrev ---- Error handling should not return OK if it isn't. I'd prefer if the exception was not caught so i could handle different cases in my code. As a workaround I added some code that looks for ".NET HttpRequestException" or ".NET Exception" in the result if status is OK, then i consider it an exception and adds some retry. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#404 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKQ1St69Jk0o6-GCo6FUEjMJtOh9DwOkks5rhXW-gaJpZM4MG-8E>.

[thinkingserious]

@AElvik,

I have added your vote to this issue to help increase the priority in our backlog. Thanks for taking the time to reach out :)

[AElvik]

@KaptenJon Will also need the dto classes, but that should not be to many so I'll try that
@thinkingserious Thanks

[thinkingserious]

Hello everyone,

This issue has been resolved with: #434

[KaptenJon]

Beautiful!!

…

---- Elmer Thomas skrev ---- Hello everyone, This issue has been resolved with: #434<#434> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#404 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKQ1SkG9OgReOkWFg_xusyTFODfaepYAks5rvl3BgaJpZM4MG-8E>.