-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better error respones from WorkflowInstanceResource endpoints #390
Conversation
1 similar comment
c49eae4
to
f5c62c4
Compare
…t response in resources
…quest response in resources instead
6ff81b0
to
2da8fc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an api change. We return text/plain from rest apis if an error occurs instead of the previous application/json.
Well actually we do not even test the response content type in the failure tests. But I believe we should keep it at application/json as it was before.
nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java
Outdated
Show resolved
Hide resolved
nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java
Show resolved
Hide resolved
nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java
Outdated
Show resolved
Hide resolved
...w-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java
Outdated
Show resolved
Hide resolved
import io.nflow.rest.v1.msg.ErrorResponse; | ||
|
||
@Provider | ||
public class StateVariableValueTooLongExceptionMapper implements ExceptionMapper<StateVariableValueTooLongException> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a tradeoff between putting the error catching&converting logic to one place vs replicating it in every method of the api.
I think we could have one generic exception mapper that just catches Throwable and returns ErrorResponse object.
Change nflow-engine to throw IllegalArgumentExceptions instead of IllegalStateExceptions/RuntimeExceptions
Construct error responses with proper HTTP code (400 instead of 500) and message text in WorkflowInstanceResources
Remove ExceptionMapper from nflow-jetty for these cases