Errors in request should yield 400 not 500 response

[jar398]

Generally any error due to inputs (bad parameter names or values, expectations not met) should be a 400. 500 is reserved for when a logic error in the server itself is detected.

Not terribly high priority. I think there is some neo4j help for this.

resp.text = {
  "message" : "Mandatory argument \"ot_node_id\" not supplied.",
  "exception" : "IllegalArgumentException",
  "fullname" : "java.lang.IllegalArgumentException",
  "stacktrace" : [ "org.neo4j.server.plugins.ParameterExtractor.extract(ParameterExtractor.java:47)", "org.neo4j.server.plugins.PluginMethod.invoke(PluginMethod.java:53)", "org.neo4j.server.plugins.PluginManager.invoke(PluginManager.java:168)", "org.neo4j.server.rest.web.ExtensionService.invokeGraphDatabaseExtension(ExtensionService.java:300)", "org.neo4j.server.rest.web.ExtensionService.invokeGraphDatabaseExtension(ExtensionService.java:122)", "java.lang.reflect.Method.invoke(Method.java:498)", "org.neo4j.server.rest.security.SecurityFilter.doFilter(SecurityFilter.java:112)" ]
}
Traceback (most recent call last):
  File "test_node_info.py", line 10, in <module>
    return_bool_data=True)
  File "/Users/jar/a/ot/repo/treemachine/ws-tests/opentreetesting.py", line 169, in test_http_json_method
    raise_for_status(resp)
  File "/Users/jar/a/ot/repo/treemachine/ws-tests/opentreetesting.py", line 202, in raise_for_status
    raise e
requests.exceptions.HTTPError: 500 Server Error: Internal Server Error

[josephwb]

Hmm. Any missing non-optional arguments (like your example above) are handled by neo4j, not anything I do. I imagine it could be changed, but it would require some digging, and as you stated it doesn't seem to be a priority at the moment.

[josephwb]

Should this be merged with https://github.com/OpenTreeOfLife/treemachine/issues/179?

[jar398]

Well #179 is in some sense done because errors do lead to 4xx OR 5xx responses - you fixed that as far as I can tell, and I thank you for it. This is a slightly different issue, selecting 4xx vs. 5xx properly. OK if I close 179?

I can't believe the arrogance of the neo4j people in dissing the HTTP spec, which they've done in multiple ways.

Maybe fixing this can be combined with the switch from POST to GET.

[jar398]

I've just completed a tour of the neo4j source code. Neo4j returns a 400 response whenever the plugin method throws an exception of one of the following two types:

org.neo4j.server.plugins.BadPluginInvocationException
org.neo4j.server.rest.repr.BadInputException

BadInputException has three constructors:

public BadInputException( Throwable cause )
public BadInputException( String message )
public BadInputException( String message, Throwable cause )

and it may be subclassed, if desired.

When throwing one of these, the exception class (or one of its ancestors) has to be added to the 'throws' list for the method, i.e. these are not RuntimeExceptions.

The exception response will be JSON with the following fields:

message
exception
fullname
stacktrace
cause

There is no way to add more fields, and there is no way to control any of these fields other than the message. The value of the message field will always be a string.

So there is no way, using neo4j so-called managed plugins, for us to put a JSON {...} inside the JSON of an exception response. We would have to use the unmanaged plugins feature to do that.

[josephwb]

Is IllegalArgumentException equivalent? This is what has been used in the past.

[jar398]

According to my reading of ExtensionService.java and OutputFormat.java in the neo4j sources, only those two exception classes will yield a 400. Everything else yields a 5xx or some other status code such as 409 that is not appropriate for this situation.

If you've seen a 400 response from IllegalArgumentException, I'd be very interested in that.

It looks like SyntaxException also turns into a bad request status, but that's just some internal neo4j thing.

Here is the key piece of code:

@POST
@Path( PATH_GRAPHDB_EXTENSION_METHOD )
public Response invokeGraphDatabaseExtension( @PathParam( "name" )

String name, @PathParam( "method" ) String method, String data ) { try { return output.ok( this.invokeGraphDatabaseExtension( name, method, input.readParameterList( data ) ) ); } catch ( BadInputException e ) { return output.badRequest( e ); } catch ( PluginLookupException e ) { return output.notFound( e ); } catch ( BadPluginInvocationException e ) { return output.badRequest( e.getCause() ); } catch ( SyntaxException e ) { return output.badRequest( e.getCause() ); } catch ( PluginInvocationFailureException e ) { return output.serverError( e.getCause() ); } catch ( Exception e ) { return output.serverError( e ); } }

The fall-through Exception case would be the only one to cover IllegalArgumentException, and that yields a 500:

public Response serverError( Throwable exception )
{
    return response( Response.status( Status.INTERNAL_SERVER_ERROR ),

new ExceptionRepresentation( exception ) ); }

_INTERNAL_SERVER_ERROR https://jax-rs-spec.java.net/nonav/2.0/apidocs/javax/ws/rs/core/Response.Status.html#INTERNAL_SERVER_ERROR_ 500 Internal Server Error, see HTTP/1.1 documentation http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.5.1.

[josephwb]

ok, thanks

[josephwb]

It looks like IllegalArgumentException just calls BadInputException:

curl -v -X POST http://localhost:7474/db/data/ext/tree_of_life_v3/graphdb/node_info -H "cotent-type:application/json" -d '{"node_id":["ott1057518", "ott90560"]}'
* Hostname was NOT found in DNS cache
*   Trying ::1...
* connect to ::1 port 7474 failed: Connection refused
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 7474 (#0)
> POST /db/data/ext/tree_of_life_v3/graphdb/node_info HTTP/1.1
> User-Agent: curl/7.38.0
> Host: localhost:7474
> Accept: */*
> content-type:application/json
> Content-Length: 38
> 
* upload completely sent off: 38 out of 38 bytes
< HTTP/1.1 400 Bad Request
< Content-Length: 1068
< Content-Type: application/json; charset=UTF-8
< Access-Control-Allow-Origin: *
* Server Jetty(6.1.25) is not blacklisted
< Server: Jetty(6.1.25)
< 
{
  "message" : "Could not convert!",
  "exception" : "BadInputException",
  "fullname" : "org.neo4j.server.rest.repr.BadInputException",
  "stacktrace" : [ "org.neo4j.server.rest.repr.RepresentationFormat.convertString(RepresentationFormat.java:330)", "org.neo4j.server.rest.repr.RepresentationFormat$1.convertString(RepresentationFormat.java:164)", "org.neo4j.server.plugins.ParameterList.getString(ParameterList.java:86)", "org.neo4j.server.plugins.StringTypeCaster.get(StringTypeCaster.java:30)", "org.neo4j.server.plugins.ParameterExtractor.extract(ParameterExtractor.java:45)", "org.neo4j.server.plugins.PluginMethod.invoke(PluginMethod.java:53)", "org.neo4j.server.plugins.PluginManager.invoke(PluginManager.java:168)", "org.neo4j.server.rest.web.ExtensionService.invokeGraphDatabaseExtension(ExtensionService.java:300)", "org.neo4j.server.rest.web.ExtensionService.invokeGraphDatabaseExtension(ExtensionService.java:122)", "java.lang.reflect.Method.invoke(Method.java:498)", "org.neo4j.server.rest.security.SecurityFilter.doFilter(SecurityFilter.java:112)" ]
* Connection #0 to host localhost left intact
}

[josephwb]

For single node queries (node_info, subtree) do we want to return a BadInputException or a TaxonNotFoundException since only a single node is involved?

[jar398]

It doesn't matter to me whether BadInputException or a subclass of it is thrown; as far as I'm concerned that's an internal implementation that I don't want to have to worry about. I have no idea what TaxonNotFoundException is, is it a subclass of BadInputException? Multiple exception types are probably not needed internally by the code, and I can't imagine how an API client would use it. But do whatever you like.