Skip to content

sendgrid-python should define it's own Error class #315

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
w- opened this issue May 20, 2017 · 10 comments
Closed

sendgrid-python should define it's own Error class #315

w- opened this issue May 20, 2017 · 10 comments
Labels
difficulty: medium fix is medium in difficulty status: work in progress Twilio or the community is in the process of implementing type: community enhancement feature request not on Twilio's roadmap

Comments

@w-
Copy link
Contributor

w- commented May 20, 2017

Issue Summary

related to sendgrid/python-http-client#16

Crurently if an application/library consumes sendgrid-python and an API request surfaces a HTTP error, it raises a urllib HTTPError.

Now, in order to handle this specific error, the consuming application needs to import urllib.

Ideally, a consuming application only needs to know about sendgrid-python and not the underlying implementation of python-http-client or urllib in order to handle common place http errors.

Steps to Reproduce

  1. make an api call that returns anything other than HTTP2xx
  2. attempt to handle error raised without importing urllib and without doing a catch all.

Technical details:

  • sendgrid-python Version: master (latest commit: [commit number])
  • Python Version: all
@dibyadas
Copy link

Hi @w- ! I was working on the same thing with ref to the issue #224. We can define a custom SendGrid exception.
One approach could be to raise a SendGridException before the request is made, for those requests that we know are going to return an error code. For ex :- an email request with empty content or empty subject.
Other approach could be to wrap the existing SendGridAPIClient class in a 'SendGridClient' class that will raise SendGridException based on the response codes, among other things.
What are your views on this?

@w-
Copy link
Contributor Author

w- commented May 20, 2017

Hi @dibyadas ,

My specific case here is that there is no way to handle an erronous HTTP response because the underlying urllib will always throw an HTTPError.

IMHO, we should have a specific Error class that represents specifically when the API call returns an non-success HTTP Response. I have not given much thought to the handling of other errors such as the example you provide "where an email request with empty content or empty subject". I just think those are a seperate class of errors.

The sendgrid API V3 documentation outlines how the API returns relatively rich error information with the body of the response often outlining specific issues with the API request.
In the current implementation it is not possible to take advantage of this without having to import urllib at the application level.

Given the specific implementation of sendgrid-python and python-http-client I'm not sure what the best way to achieve this would be.

@thinkingserious thinkingserious added the status: duplicate duplicate issue label May 22, 2017
@thinkingserious
Copy link
Contributor

Thanks for the feedback @w-!

@dibyadas has now created a PR. It would be great if you could take a look and let us know your thoughts.

@w-
Copy link
Contributor Author

w- commented May 22, 2017

@thinkingserious thanks for taking a look at this.
I'm not sure that identifying this as "duplicate" and closing this is appropriate. I feel that this and #224 are completely different topics.
Especially after reviewing the changes from @dibyadas it is clear his focus is around sending mail, whereas mine is around use of the API library in general.
Again, currently any API call made using sendgrid-api that doesn't result in an http2xx returns a urllib HTTPError. There is no way for a consuming application to handle these errors specifically without understanding that sendgrid uses http-python-client and that uses urllib.

@thinkingserious
Copy link
Contributor

Please see the comment I made here.

Thanks for taking the time to help out with your feedback, we greatly appreciate it!

@phrohdoh
Copy link

phrohdoh commented Mar 20, 2018

I'd like to resurrect this issue as sendgrid-python the package still does not export its own error types.

This means, as @w- pointed out, library users must be knowledgeable of an implementation detail (urllib, python-http-client, etc).

Due to this library users must declare python-http-client as an explicit dependency (which means keeping python-http-client and sendgrid in sync).

IMO ideally sendgrid would re-export the error types defined in python-http-client which would allow sendgrid library users to write code similar to the following:

import sendgrid
from sendgrid.helpers.mail import *

sg = sendgrid.SendGridAPIClient(apikey='foobar')
from_email = Email('foo@bar.com', name='Foo')
to_email = Email('qux@baz.net', name = 'Qux')
subject = 'my subject'
content = Content('text/html', 'some html here')
mail = Mail(from_email, subject, to_email, content)

try:
    response = sg.client.mail.send.post(request_body=mail.get())
except sendgrid.exceptions.HTTPError as e:
    print(e.read())

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: help wanted requesting help from the community difficulty: easy fix is easy in difficulty up-for-grabs and removed status: duplicate duplicate issue labels Mar 20, 2018
@thinkingserious
Copy link
Contributor

Thanks for the heads up @phrohdoh!

This issue is officially resurrected :)

@hundredrab
Copy link

If it's still worth it, I would like to have a go at it.

@thinkingserious
Copy link
Contributor

Hi @hundredrab,

It's all yours, but please make your contribution to the v4 branch. Thanks!

With Best Regards,

Elmer

@thinkingserious thinkingserious added status: work in progress Twilio or the community is in the process of implementing hacktoberfest difficulty: medium fix is medium in difficulty and removed difficulty: easy fix is easy in difficulty good first issue status: help wanted requesting help from the community labels Oct 1, 2018
@thinkingserious
Copy link
Contributor

Since there has been no activity on this issue since March 1, 2020, we are closing this issue. Please feel free to reopen or create a new issue if you still require assistance. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: work in progress Twilio or the community is in the process of implementing type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests

6 participants