search

syndesisio / syndesis-rest

The API for Syndesis - a flexible, customizable, cloud-hosted platform that provides core integration capabilities as a service. It leverages Red Hat's existing product architecture using OpenShift Online/Dedicated and Fuse Integration Services.
https://syndesis-staging.b6ff.rh-idev.openshiftapps.com/api/v1/
Apache License 2.0
6 stars 17 forks source link

Return json for error responses #247

Closed gashcrumb closed 7 years ago

gashcrumb commented 7 years ago

In some cases we don't get back JSON error responses, for example:

com.fasterxml.jackson.databind.JsonMappingException: configuredProperties value
 at [Source: io.undertow.servlet.spec.ServletInputStreamImpl@2c784ac8; line: 43, column: 7] (through reference chain: com.redhat.ipaas.model.integration.Integration$Builder["steps"]->java.util.ArrayList[0]->com.redhat.ipaas.model.integration.Step$Builder["configuredProperties"])

Unfortunately the framework we're using for making HTTP requests chokes on this because it assumes JSON responses in all cases. While I plan on working around this, we really should try and have JSON responses for error conditions as well, raw string values are kinda hard to deal with when showing the user a useful error message.

related to -> https://github.com/redhat-ipaas/ipaas-ui/issues/328

gashcrumb commented 7 years ago

This is also not ideal:

<html><body><h1>504 Gateway Time-out</h1>
The server didn't respond in time.
</body></html>
jimmidyson commented 7 years ago

Please can we follow the same approach as Kubernetes which returns a defined Status object including status code, message, etc.

rhuss commented 7 years ago

@gashcrumb Is this the full exception or do you have some more context information for this first issue ? Need to find out the point in the server where this happens.

w.r.t. to the gateway error, this comes from the OpenShift router. On these error messages we dont have much influence, I'm afraid.

Actually I would not rely on a JSON body for everything with HTTP response code != 200. Isn't it that an Ajax error handler is called for the situation above ? IMO a real world 'framework' should be able to cope with non-JSON responses in error cases as this can always happen because we don't have every layer under control (e.g. not the router, not keycloack responses, ...)

Also, the content-type of the response might be useful, but that can probably not used, too.

gashcrumb commented 7 years ago

Yeah, think this error comes from not having a configuredProperties object set on a Step in the flow.

And yeah! was surprised that Restangular assumes json response as well, haven't yet figured out how to work around that.

jimmidyson commented 7 years ago

@rhuss The REST API should absolutely return a JSON body for any application issues.

As for the 504, we should switch to an async approach, request to update storage (REST API persists) and then poll/event driven updates for status. Normal REST API requests should be as quick as possible.

gashcrumb commented 7 years ago

Although actually now that I think about it, for that second error that came through fine. Maybe the Content-Type on the response was being set to JSON even though the body was just a string. Lemme see if I can force that error again really quick

gashcrumb commented 7 years ago

K, nope, this would be the Content-Type I'd expect for a non-JSON response:

Content-Type:text/html

so definitely something I've gotta figure out with restangular...

rhuss commented 7 years ago

@jimmibot agreed that we can fix the timeout issue on the gateway, but this won't prevent you the gateway to send other errors that happen in plain html within the body (guess that the point of this issue). I agree that it would be awesome if everybody would send it in a machine readable format, but we should also be prepared when this is not the case (because we cant guarantee that)

rhuss commented 7 years ago

@gashcrumb back to the original issue: If I understand right this consists of two parts:

rhuss commented 7 years ago

I will look at the generic serialization error stuff.

@KurtStam as you are on the domain model anyway via #246 could you have look to the missing configureProperties on Step ?

gashcrumb commented 7 years ago

The domain model is missing a property 'configuredProperties' and should be fixed so that this error won't happen.

In this case it's likely my fault, as I'm sending a configuredProperties for an HTTP connection with null for the URL string value, as I think we've not yet got validation working on the step config page. So it's a valid error response I think, but it'd be good if it was in JSON.

KurtStam commented 7 years ago

We need to map all exceptions: rest/src/main/java/com/redhat/ipaas/rest/v1/handler/exception/

jludvice commented 7 years ago

Hello @KurtStam. In response to https://github.com/redhat-ipaas/ipaas-ui/issues/328: creating integration on localhost:4200 (using staging ipaas-rest as backend) Error message: Response with status: 0 for URL: null

creating same integration on ipaas-qe instance on openshift: Error message: Response with status: 500 Internal Server Error for URL: https://ipaas-qe.b6ff.rh-idev.openshiftapps.com/api/v1/integrations Stacktrace from ipaas-rest deployment: stacktrace.txt And it looks like it's mapped to json:

{
    "timestamp": 1490859237012,
    "status": 500,
    "error": "Internal Server Error",
    "exception": "org.jboss.resteasy.spi.UnhandledException",
    "message": "com.redhat.ipaas.core.IPaasServerException: An error has occurred.",
    "path": "/api/v1/integrations"
}

How is that possible that one error produces 500 lines of stacktraces in logs? Is this info helpful?

KurtStam commented 7 years ago

@jludvice yes this is helpful. The issue is that the user that is logged in now touches GitHub and Kubernetes for which he needs to be authenticated and authorized. AFAI understand it we are looking into using offline tokens for this now. So trying to fix this for the sync case is not much use since it's about to change anyway.

This brings up a good point about error handling though. The current code in place to handle to exceptions when using RESTEasy has not been updated now that we are using Spring. So while @iocanel is working on the offline token stuff I will take a look at improving the error handling.