Bookworm-project / BookwormDB

Tools for text tokenization and encoding
MIT License
84 stars 12 forks source link

Error responses in API #95

Open organisciak opened 8 years ago

organisciak commented 8 years ago

The API needs some format for returning errors, rather than giving a traceback (in the case of exceptions) or zeros (in the case of missing terms). This can communicate better down the line to clients.

This is one of the most important needs right now and I'm willing to work on it, but @bmschmidt should have the final say in the error response policy.

I propose a very simple response, { error: String(), errorcode: int() }

We can number errors as we account for them. For example:

Query Error

  1. Malformed query
  2. Term not indexed
  3. Too many words in phrase
  4. facetName doesn't exist
  5. Database databaseName does not exist

Server Error (i.e. please contact admin)

  1. Cannot access host
  2. Memory tables need to be rebuilt
  3. Unknown server error
bmschmidt commented 8 years ago

Good.

One issue with this format is that it is not completely obvious how to distinguish a failed call from a successful one. (A legimate bookworm response could, in theory, be a dict with the keys error and errorcode).

Would it be too cumbersome on the client-API side to return a 500 (Internal error), and then have the data portion of the error description contain the JSON format you propose?

bmschmidt commented 8 years ago

One problem with my suggestion is that there are cases where the API is deployed locally rather than over HTTP.

Maybe an error should just be a single string. That would be unambiguous, because valid responses are always dicts or lists.

"ERROR 101: Query does not parse to valid JSON" "ERROR 102: Search term 'word':'%s' not in database" <- This one will need to specify which term, exactly, isn't, and allow errors on other terms as well. "ERROR 201: Cannot access host" "ERROR 202: Database tables are empty"

organisciak commented 8 years ago

My preference is to communicate in JSON, because that's what clients are looking for when they make the call, and it is what I usually see elsewhere. Your first suggestion sounds better for this reason, and because the header is what tells the callback for d3.json or $.json that there was an error.

Still, this brings up the issue that the existing response isn't structured in any formal way. It would be nice if it was wrapped in the way the very simple JSend spec suggests: {status: 'STRING', data: { ...BW-RESPONSE... }. Of course, that would break our existing apps and cause headaches.

How about this: create an alternative to "method":"return_json" (return_json2? return_jsend?) where a client can ask for a 'nice' JSON response with status, data, and optional error and message keys. Then we wouldn't break old clients and instead can take the time to update them appropriately. The necessary client update, of course, would simply be to read json.data rather than json.

bmschmidt commented 8 years ago

The JSend spec looks like the right way: but yes, I've been trying to dodge that implication for the reasons you say.

I like your plan to tweak the API call for back-compatibility.

Rather than introduce a new method, could we begin to phase in the change from this ticket and specify that iff 'format=="json"`in the base query object, it returns in the better format you propose?

organisciak commented 8 years ago

See 72648d2e17a0cb7f324044f26ac3388df07bfcf3.

I used JSend, and for 'code' I'm using HTTP codes, while 'message' adds more details. Next steps are to investigate database errors to provide more informative responses, and to give errors on unexpected {} responses.