broadinstitute / cromwell

Scientific workflow engine designed for simplicity & scalability. Trivially transition between one off use cases to massive scale production environments
http://cromwell.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
999 stars 360 forks source link

releaseHold endpoint returns inaccurate status code #3911

Open rexwangcc opened 6 years ago

rexwangcc commented 6 years ago

I'm using CaaS-dev when ran into the issue, with version: 34-66c0fa6-SNAP

When using the releaseHold endpoint to release a workflow, if the workflow_id is in a good format but refers to a non-exist workflow (fake uuid), according to the swagger schema, Cromwell should return 404 to the user. However, now it returns a 500 code which seems to be a wrapper around the actual 404 error:

CromIAM unexpected error: cromwell.api.CromwellClient$UnsuccessfulRequestException: Unmarshalling error: HttpResponse(404 Not Found,List(Server: akka-http/10.1.3, Date: Fri, 20 Jul 2018 13:58:13 GMT),HttpEntity.Strict(text/plain; charset=UTF-8,{
  "status": "fail",
  "message": "Unrecognized workflow ID: f4272a19-37dd-4d2c-ba48-ff3844107bf8"
}),HttpProtocol(HTTP/1.1))

This is not a big problem but just brings some incovenience to the error handling process to the users.

cjllanwarne commented 6 years ago

I think the "CaaS" comment at the end should be put into the headline (ie something like CaaS wraps 404 into 500 on releaseHold requests)

And we should probably check this works in other cases where 404s might be returned, not just releaseHold

rexwangcc commented 6 years ago

@cjllanwarne edited. I did file a similar issue a while ago, not sure if it is still true: https://github.com/broadinstitute/cromwell/issues/2961

geoffjentry commented 6 years ago

Looked at this quickly w/ @salonishah11 and I suspect the problem is due to our "artisanal" friend, cromwell.api.CromwellClient, which as we discovered earlier this week just gleefully eats errors from Cromwell and returns them in an insane way