bad json should be 400 not 500

[ryandawsonuk]

fixes #943

[adriangonz]

It makes sense to return a 400 if the JSON request is incorrect, but should we also return a 400 in this case?

seldon-core/engine/src/main/java/io/seldon/engine/api/rest/RestClientController.java

Lines 231 to 249 in ec18ddf

	try
	{
	SeldonMessage response = predictionService.predict(request);
	String responseJson = ProtoBufUtils.toJson(response);
	return new ResponseEntity<String>(responseJson,HttpStatus.OK);
	}
	catch (InterruptedException e) {
	throw new APIException(ApiExceptionType.ENGINE_INTERRUPTED,e.getMessage());
	} catch (ExecutionException e) {
	if (e.getCause().getClass() == APIException.class){
	throw (APIException) e.getCause();
	}
	else
	{
	throw new APIException(ApiExceptionType.ENGINE_EXECUTION_FAILURE,e.getMessage());
	}
	} catch (InvalidProtocolBufferException e) {
	throw new APIException(ApiExceptionType.ENGINE_INVALID_JSON,"");
	}

I'm assuming that in that case, the downstream predictive service returned a invalid JSON. It's not clear to me if that should be considered a server or client error.

PS: I'm aware this is a bit nitpicky.

[ukclivecox]

It makes sense to return a 400 if the JSON request is incorrect, but should we also return a 400 in this case?

seldon-core/engine/src/main/java/io/seldon/engine/api/rest/RestClientController.java

Lines 231 to 249 in ec18ddf

	try
	{
	SeldonMessage response = predictionService.predict(request);
	String responseJson = ProtoBufUtils.toJson(response);
	return new ResponseEntity<String>(responseJson,HttpStatus.OK);
	}
	catch (InterruptedException e) {
	throw new APIException(ApiExceptionType.ENGINE_INTERRUPTED,e.getMessage());
	} catch (ExecutionException e) {
	if (e.getCause().getClass() == APIException.class){
	throw (APIException) e.getCause();
	}
	else
	{
	throw new APIException(ApiExceptionType.ENGINE_EXECUTION_FAILURE,e.getMessage());
	}
	} catch (InvalidProtocolBufferException e) {
	throw new APIException(ApiExceptionType.ENGINE_INVALID_JSON,"");
	}

I'm assuming that in that case, the downstream predictive service returned a invalid JSON. It's not clear to me if that should be considered a server or client error.

PS: I'm aware this is a bit nitpicky.

I suppose its a 500 as the bad JSON is coming from the model rather than the client?

[ryandawsonuk]

Yeah I guess bad request and bad response should actually be separated as different cases

[ryandawsonuk]

Have updated now

[adriangonz]

/approve

[ryandawsonuk]

/approve

[seldondev]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriangonz, ryandawsonuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ryandawsonuk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment