search

sendgrid / sendgrid-csharp

The Official Twilio SendGrid C#, .NetStandard, .NetCore API Library
https://sendgrid.com
MIT License
1.07k stars 588 forks source link

[Suggestion] SendGridClient.SendEmailAsync should throw when a non-success response is received. #775

Closed thomaslevesque closed 4 years ago

thomaslevesque commented 5 years ago

Currently, if I call SendGridClient.SendEmailAsync and the API returns an error, it doesn't throw an exception. I have to check the response (which is made harder than necessary by #774) to see what happened. It's counter-intuitive; if the method doesn't throw, anyone would expect that the mail was successfully sent (or at least that the request was accepted). Even the samples don't show how to check the response for success.

When an error occurs, an appropriately typed exception should be thrown, so that users don't incorrectly assume the request was successful. This would also make it easier to act on the error based on the type of the exception. Currently the user has to know the details of the API to interpret the response. SendGridClient should abstract those details, otherwise it's just a thin wrapper around HttpClient. And doesn't really bring any value.

thinkingserious commented 5 years ago

Love it! Thanks for taking the time to write up this issue. I've added it to our Hacktobefest celebration.

thinkingserious commented 5 years ago

For an example of how to view errors currently, please see this.

For reference, check out the excellent StrongGrid.

Error documentation can be found here.

crnd commented 5 years ago

Any ideas on how this should be done?

  1. Two new exception types like RequestException and ServerException and then have status code and error details in the exception message?
  2. Exceptions for all the error states like RequestPayloadTooLargeException, TooManyRequestsException, ServerUnavailableException and so on?
thomaslevesque commented 5 years ago

@crnd I was thinking about something more business-specific, e.g. "missing recipient" or "empty body" or whatever the SendGrid API returns. Exceptions that just reflect the HTTP status code don't bring much value.

FrederikBolding commented 5 years ago

What is the difference between this and #796? Wouldn't this be fixed by throwing exceptions at MakeRequest?