jebena / jebena-python-client

Simple Python Client for the Jebena API Server
Mozilla Public License 2.0
2 stars 1 forks source link

Current implementation of `run_query` cannot return the GQL response if it contains any errors. #1

Closed this-vidor closed 3 years ago

this-vidor commented 3 years ago

A GraphQL query may include many errors (e.g., "Permission Denied"), while still (a) successfully mutating data on the server; and (b) succesfully retrieving information from the server. The current implmentation of run_query does not allow a calling function to see the response to the query if it contains any errors, which makes it difficult and dangerous to use (i.e., a client might change data on the server but be unable to inspect the response to discover what changes were made).

this-vidor commented 3 years ago

Really, the client should not be opinionated about how to handle errors above the HTTP layer; this is for calling code to inspect and manage.

jpotter commented 3 years ago

Ah, good catch; thanks!

Part of the challenge here is to make this client usable in both cli situations and programmatically. There are trade-offs between what we'd want to have happen in these two scenarios. That is, on the command line, if any error is encountered in the GQL query, then by default we would want a non-zero exit code. Programmatically, for simple setups, we'd probably want a similar "by default, raise" setup -- so that simple read-queries fail loudly.

My proposed solution here is to add a kwarg to run_query that's return_instead_of_raise_on_errors: bool = False -- so that programatic uses where the caller knows to handle the error messages can do so, but otherwise, a simple use condition can be thought of as "just call run_query() and you'll get back what you asked for, or an exception if it didn't work."

Thoughts? Does that seem like a good balance to you?

jpotter commented 3 years ago

Per convo with @this-vidor -- we're now logging "all" the details that might be useful in this context; and the default will remain as "raises an exception if any error is encountered." For developers who want to handle the errors directly, passing return_instead_of_raise_on_errors = False to run_query() preserves the more GQL-iphic way of doing this, while giving us a default behavior which is more pythonic ("I got a dict back and can assume it's what I wanted" ... I suppose! :-) )

PR coming up shortly.