Error handling issues, PR candidate

[luke-c-sargent]

PR: #6

While working through the sample code in the readme to add an organism and update its permissions, I ran into an issue while trying to debug. When trying to add an organism, the response is assumed to be a list (as it is in the case of a successful request). The response is not checked, so that if there is an error the script progresses until it causes an error (in this case, TypeError: string indices must be integers)

This PR creates a decorator that can be applied to any function expected to return a dict of the JSON response; if there is an error message, a newly-created exception is raised. The decorator extracts relevant information (which function caused the error message and the error text). Talking with Apollo devs at the hackathon and looking through the Apollo repo, it seems as though the "dictionary with key 'error' having value 'error message'" is the standard method to inform users of an error.

So this:

Traceback (most recent call last):
  File "./test.py", line 24, in <module>
    public=False
  File "/Users/sargentl/code/PAG_2018_hackathon/python-apollo/apollo/organisms/__init__.py", line 52, in add_organism
    return [x for x in response if x['commonName'] == common_name][0]
  File "/Users/sargentl/code/PAG_2018_hackathon/python-apollo/apollo/organisms/__init__.py", line 52, in <listcomp>
    return [x for x in response if x['commonName'] == common_name][0]
TypeError: string indices must be integers

Becomes:

Traceback (most recent call last):
  File "./test_harness.py", line 24, in <module>
    public=False
  File "/Users/sargentl/code/PAG_2018_hackathon/fork/python-apollo/apollo/decorators.py", line 8, in wrapper
    (fn.__name__, r["error"]))
apollo.exceptions.APIErrorResponseException: Apollo Error in function "add_organism":
    -problem saving organism: java.lang.Exception: Invalid directory specified: /Users/sargentl/Documents/jb_testhub/json

Instead of dealing the the error's error, I can see at a glance that I've specified an invalid parameter.

I have only applied the decorator to a single function at this point (the one that led me to discover the issue); it can be added manually to any function that can benefit from it, or added to functions programmatically. This PR is partially to ensure that these changes are desirable and properly implemented before applying them more broadly.

If there are any modifications I can make to conform to the overall style / architecture, let me know.

*: this work done during the GMOD Hackathon at PAG 2018

[hexylena]

Thanks for the PR @luke-c-sargent! Next time just use this text when opening the PR rather than filing a separate issue.

[luke-c-sargent]

8 is a minor fix, but has presented another opportunity for broad error checking I'd like to discuss.

I would like to add checking for required values existing / not being null, and after talking with the Apollo devs it seems as though there are a few values that absolutely cannot be null, but in some cases there are keys where null values would be replaced by defaults server-side. I would like to add this checking as its absence contributes to the generation of esoteric / misleading error messages:

Traceback (most recent call last):
  File "./test_harness.py", line 38, in <module>
    read=True
  File "/Users/sargentl/code/PAG_2018_hackathon/fork/python-apollo/apollo/decorators.py", line 6, in wrapper
    r = fn(*args, **kwargs)
  File "/Users/sargentl/code/PAG_2018_hackathon/fork/python-apollo/apollo/users/__init__.py", line 123, in update_organism_permissions
    response = self.post('updateOrganismPermission', data)
  File "/Users/sargentl/code/PAG_2018_hackathon/fork/python-apollo/apollo/client.py", line 52, in post
    (resp.status_code, resp.text))

Ultimately, this error happens in client.py, so a check could go in there; the problem with that is that, while it has a client_method variable, that is the name of the API call and not the calling method, so it's not QUITE exact in its description of where the problem lies (you'd have to search for the analogous call, ie updateOrganismPermission becomes update_organism_permissions). This isn't the worst thing in the world as, by and large, the functions are just switched from camel case to underscore notation, but it's not something you can just pop in a 'search project' field.

I have some ideas as to how this could be implemented; one is to create classes that have attributes / logic dictated by the API docs. In the case of update_organism_permissions, a UsersChecker class might have a function named update_organism_permissions that, when given the initial arguments, can check to see if all the required elements are in place, or if the conditional requirements are met (if there is no userId, use the optional user value), and return an error if they are not (which would then get caught by the error checker.

Another (simpler, but cluttering) option is to just put that logic in each function directly.

Any thoughts welcome!

[hexylena]

(Sorry for short replies, on holiday.)

up until now I've been writing error checking inside the functions themselves. Some of those could probably be changed into decorators. E.g. you might look into something like https://github.com/deadpixi/contracts/ this would probably be quite simple and clean to do, and would keep the documentation right next to the functions.