dotasek commented 6 years ago

The story below is quite long, but I think it's VERY IMPORTANT. It could remove an enormous impediment for people trying to automate Cytoscape. PLEASE READ IT

While building a script for Barry, I encountered some trouble that pointed out how we're failing py2Cytoscape and (likely) RCy3 users.

The Problem: Our Error Reporting is Failing py2Cytoscape/RCy3 users

Run the following python code:

from py2cytoscape.data.cyrest_client import CyRestClient

cy = CyRestClient()

n = cy.network.get(id="-1")

This is designed to fail. -1 is a garbage SUID that refers to no valid network. What I (and other Python users) would benefit from is a good error report. When something like the above is executed, an error should interrupt execution and say something like the following:

The network with SUID "-1" could not be found.

What actually happens is this trace:

Traceback (most recent call last):
  File "/media/davidotasek/skynet/IN/2018_04_06 Failing py2cytoscape/born_to_lose.py", line 5, in <module>
    n = cy.network.get(id="-1")
  File "/home/davidotasek/anaconda2/lib/python2.7/site-packages/py2cytoscape/data/network_client.py", line 158, in get
    return self.session.get(self.__url + '/' + str(id)).json()
  File "/home/davidotasek/anaconda2/lib/python2.7/site-packages/requests/models.py", line 892, in json
    return complexjson.loads(self.text, **kwargs)
  File "/home/davidotasek/anaconda2/lib/python2.7/json/__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "/home/davidotasek/anaconda2/lib/python2.7/json/decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/home/davidotasek/anaconda2/lib/python2.7/json/decoder.py", line 382, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded

I got this exact error while attempting to build a script. I was sending an SUID that was a root network. Like the -1 value above, it didn't represent a subnetwork. The python method treated this the same as an invalid SUID, hence the error. Because the error told me nothing about the validity of the parameter, and gave me little to indicate what the cause was, I had to investigate upstream.

Tracing the error in network_client.py, the source of the error appears to stem from the code attempting to access a URL similar to this:

http://localhost:1234/v1/networks/-1

This returns an error that explains how we got to the Python error, but isn't much more informative:

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-1"/>
<title>Error 404 Not Found</title>
</head>
<body><h2>HTTP ERROR 404</h2>
<p>Problem accessing /v1/networks/-1. Reason:
<pre>    Not Found</pre></p><hr /><i><small>Powered by Jetty://</small></i><br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                
<br/>                                                

</body>
</html>

CyREST is returning a 404 error, indicating that a network could not be found. The 404 response could be used by Python to guesstimate the cause of the error in this case, and produce a more informative Python error, but in cases such as GET /v1/networks/{networkId}/views/{viewId}/{objectType}, we are no longer delivering as useful a message: any one of three parameters could be causing the 404 response. So for Python to produce a more useful error, CyREST must also produce a more useful error.

If you've gotten this far, I've spent the last 91 lines in my text processor walking you through why my script was failing. This wasted a considerable amount of time, and I am by now quite experienced DEVELOPING for Cytoscape. For a hapless scripter, without the same knowledge and resources, figuring out that error could be even more frustrating. Take those man-hours and multiply them by the number of scripters trying to automate Cytoscape, and then imagine the cost in wages of badly managed error messages. Compare all that to the relatively short time a scripter would spend trying to figure out why The network with SUID "-1" could not be found.

I suspect (no, I know) this problem appears again and again between CyREST, py2cytoscape, and RCy3. Commands are not immune; their error reporting exhibits the same pitfalls.

Proposed Solution: Fully adopt CI-style error reporting.

The CI style of error reporting shows up repeatedly in the CyREST Swagger. What hasn't happened is universal coverage: many legacy CyREST calls and Cytoscape Commands, like the one I pointed out above, still rely on stacktraces and default catch blocks.

A very good example of providing CI Errors can be found in the Diffusion App: https://github.com/cytoscape/diffusion/blob/b53e4195784aa2e216a21b69ec61b7389f041574/src/main/java/org/cytoscape/diffusion/internal/rest/DiffusionResource.java#L157

Errors are reported with a clear message, and unique type that can help downstream libraries like py2Cytoscape and RCy3 identify the causes of failure and pass them on to users.

I would like CyREST to adopt the CI style of error reporting for every endpoint. This would mean that all errors currently reported in the expedient but uninformative ways like HTML error pages and stacktraces would no longer be available, and would be replaced by better, clearer CI JSON.

As these error reports are put in place, I would like RCy3 and py2Cytoscape to pass those errors on.

AlexanderPico commented 6 years ago

I'm all for better error messaging. However, I don't see this an issue at all for RCy3 users... R script writers will rarely have to handle SUIDs directly and in cases where they do, they will be getting them from prior returns. No one would start inputting numbers "manually"... well, I shouldn't say no one, but if someone did do this, I'm okay with giving them a stacktrace :)

dotasek commented 6 years ago

@AlexanderPico you are correct when it comes to the Cytoscape core.

However, I landed on this because I was using the REST endpoints from the CyNDEx App, and it returned a root network SUID where I expected a subnetwork SUID.

If I stay within the confines of our R/Python libraries, we should be fine, but if I really want to leverage all the apps providing me with automation capabilities, I might run into a lot more situations like this.

jorgeboucas commented 6 years ago

In the cyrest module all functions allow a verbose=True.

If used, this will pass/return whatever came back from the API - check it here and here.

It will then be to the user to do things with more error handling or not.

@dotasek will that do it?

dotasek commented 6 years ago

@jorgeboucas Would it be possible to check the HTTP response for a valid response code (for example, a 200 OK), and automatically switch to verbose if it's not an expected response (for example, a 404)? I think that would definitely give the user all the feedback they need immediately.

bdemchak commented 6 years ago

@jorgeboucas @dotasek @AlexanderPico Hi, all ...

The problem here is that 404 is overloaded. Formally, it means that a resource isn't available. But given that the particular resource could be one of several, it sends the user on an unfortunate hunt. This isn't a good way to earn CyREST a good reputation.

CIResponse deals with this very effectively. To refresh: { "data": {} "errors": {} } ... where a function returns either the "data" or "errors" field. See https://github.com/cytoscape/cytoscape-automation/wiki/App-Developers:-Cytoscape-Function-Best-Practices#ciresponse

As a practical matter, if an endpoint reaches working code, the endpoint should return a CIResponse and HTTP 200, and not an HTTP 404.

If no endpoint can be found, it should return a 404.

This gets us out of all kinds of trouble and gives the caller the best and most accurate information.

How can we make this happen for all endpoints?

dotasek commented 6 years ago

@bdemchak At the moment, most of this falls on CyREST itself... right now, anything legacy in CyREST could return a JSON stacktrace (bad) or Jetty's default error pages in HTML (much worse). The newer functions return CI errors like you mentioned, but I'll have to go through the code and add CI where it's missing.

I like @jorgeboucas's idea with the modification I suggested; if his harmonization call gets anything unexpected back, it could just forward the raw HTTP response... which would give python users a very honest view of what happened. I think that would require as little coding as possible on the Python/R side, and satisfy the requirement of giving scripters what they need to succeed.

jorgeboucas commented 6 years ago

@dotasek 3.5 train hours later, 3 countries, and 2 activities warnings from my watch - yes we can

jorgeboucas commented 6 years ago

I obviously didn't manage anything for the cases where there is the need to parse an HTML.

But for GET and POST if not 200 then go on verbose, spit everything to the stdout and return the error code.

It is also helping with the automated parsing of functions from the swagger/commands to python as now there is a constant if response: return response.

🎉 🎈

dotasek commented 6 years ago

@jorgeboucas that looks good. You might be able to save yourself a few more line of code if you make the comparison by numeric range instead of singling out individual error codes (200, 201, etc). I've found this guide pretty helpful: http://www.restapitutorial.com/httpstatuscodes.html

As for CyREST returning errors in HTML or weirdly formatted JSON, it's up to me to start updating those outputs, so they are not your problem at all. :)

dotasek commented 6 years ago

Going through the source code and removing reference by reference of manually instantiated javax.ws.rs Exceptions:

[x] javax.ws.rs.NotFoundException
[x] javax.ws.rs.WebApplicationException
[x] javax.ws.rs.BadRequestException
[x] javax.ws.rs.ServiceUnavailableException
[x] javax.ws.rs.InternalServerErrorException

dotasek commented 6 years ago

During Testing, discovered that CyExceptionMapper is passing through literal calls of WebApplicationException. This should be mitigated by extending WebApplicationException for anything that intends to throw legacy exceptions (GET Commands, on the whole).

dotasek commented 6 years ago

[x] Expand NOT_FOUND_ERROR fields to be more specific.

For example, /{networkId}/edges/{edgeId}/{type} should have errors specific to networkId, edgeId, and type instead of one generic error.

dotasek commented 6 years ago

Done. Awaiting review by @jorgeboucas before merge into main branches.

cytoscape / cyREST

Universal CI Error Reporting #79

The Problem: Our Error Reporting is Failing py2Cytoscape/RCy3 users

Proposed Solution: Fully adopt CI-style error reporting.