HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Python API client

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
clientapipython

Problem

I'm creating a Python API client, after thinking and checking some open source Python clients I created this class, and I'm looking for any feedback (design, best practices...), this is an example URL of the API:

https://www.api.com/collection/resource.json?userid=id&key=apikey&command=value


Here's my class using the requests library:

```
import requests

class APIClient(object):
"""Creates an API client object

:param userid: the API userid found in the control panel
:param api_key: the API key found in the control panel Settings > API

"""
def __init__(self, userid=None, api_key=None):
# userid and api key sent with every request
self.userid = userid
self.key = api_key

# the base production api url
self.base_url = None

# the sandbox api url
self.test_url = None

# boolean value to indicate which url to use
self.testing = False

# the error description if it occured
self.error = None

def request(self, method='GET', path=None, params=None):
"""Makes a request to the API with the given parameters

:param method: the HTTP method to use
:param path: the api path to request
:param params: the parameters to be sent

"""
# if we're testing use the sandbox url
api_url = self.test_url if self.testing else self.base_url

# add the authentication parameters (sent with every request)
params.update({
'userid': self.userid,
'key': self.key
})

# the api request result
result = None

try:
# send a request to the api server
r = requests.request(
method = method,
url = api_url + path + '.json',
params = params,
headers = {'User-Agent': 'Python API Client'}
)

# raise an exception if status code is not 2

Solution

-
Python has a mechanism for handling exceptional situations, but you go to great lengths to suppress it: you catch all the exceptions that might result from your request method and replace them with a string in the .error property of the APIClient object.

This has several problems.

First, it needlessly complicates every API call. A caller can't just write:

r = api.request(...) # might raise an exception
do_something_with(r)


allowing exceptions to pass up the call stack and so eventually appear on the console or in the log. Instead, they have to write:

api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
    # handle the error somehow
else:
    do_something_with(r)


This means that every call to your API needs to consider how to handle errors. What are programmers going to do? Well, mostly likely they will raise these errors:

api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
    raise MyError("API error: {}".format(api.error))
else:
    do_something_with(r)


So why bother suppressing these errors in your API in the first place?

Second, you replace all exceptions that you don't recognize with An unexpected error occurred. So this makes it impossible to distinguish a ProxyError from a TooManyRedirects from an SSLError.

Third, you lose information that was present in the exception objects. For example, an SSLError comes with a description of the problem, for example if the certificate doesn't match the domain, you'll get an error like this:

requests.exceptions.SSLError: hostname 'foo.com' doesn't match 'bar.com'


This kind of information is vital in tracking down the cause of problems. But you replace this with An unexpected error occurred which is, frankly, useless.

-
If the status code is not 200, you:

raise Exception


Which has three problems: (i) you should raise an instance of an exception class, not the class itself; (ii) you should initialize the exception object with data that describes the exception; and (iii) the class Exception is supposed to be the root of the class hierarchy for "built-in, non-system-exiting exceptions" and user exceptions. You shouldn't raise Exception itself, but instead derive a specific exception class and raise an instance of that. So here you would need something like:

class HTTPError(Exception): pass


And then:

raise HTTPError('Status code {}'.format(r.status_code))


But in fact the requests module already has a function for doing this, so all you need to do is:

r.raise_for_status()


-
Your interface for setting API parameters is to set them directly as properties of the API object:

api = APIClient()
api.userid = '123456'
api.key = '0tKk8fAyHTlUv'
api.base_url = 'https://api.com/'
api.test_url = 'https://test.api.com/'
api.testing = True


It would be better for the constructor to take these as keyword arguments: (i) this allows the constructor to check that all required parameters have been set; (ii) parameter values can be checked (if possible); and (iii) the keyword argument mechanism is more flexible than object properties since you can pass a sets of keywords around in the form of a dictionary. So I would expect to write:

api = APIClient(userid = '123456',
                key = '0tKk8fAyHTlUv',
                base_url = 'https://api.com/',
                test_url = 'https://test.api.com/',
                testing = True)

Code Snippets

r = api.request(...) # might raise an exception
do_something_with(r)
api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
    # handle the error somehow
else:
    do_something_with(r)
api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
    raise MyError("API error: {}".format(api.error))
else:
    do_something_with(r)
requests.exceptions.SSLError: hostname 'foo.com' doesn't match 'bar.com'
raise Exception

Context

StackExchange Code Review Q#33919, answer score: 9

Revisions (0)

No revisions yet.