IMHO returning 500 internal server error here might not exactly convey the problem. AFAIU, RemoteException is thrown when we can't connect to a remote host, maybe due to a network issue, or remote host being offline, etc. It might be more explanatory to return 504 (Gateway Timeout), WDYT?
On Tue, Apr 28, 2015 at 2:43 PM, Anuruddha Liyanarachchi < anurudd...@wso2.com> wrote: > +1 for throwing *500 Internal Server Error *for all the RemoteException. > > > > On Tue, Apr 28, 2015 at 2:23 PM, Lahiru Sandaruwan <lahi...@wso2.com> > wrote: > >> Hi, >> >> I have fixed few success codes, and looking into error codes now. For all >> the "RemoteException"s thrown due to service unavailability etc. we should >> through *500 Internal Server Error.* >> >> Wdyt? >> >> Thanks. >> >> On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <chami...@wso2.com> >> wrote: >> >>> Hi devs, >>> >>> After going through the REST API and the tickets that are raised for it, >>> I think we should do another round of refactoring on it. Most API methods >>> will stay unchanged, and there will be no additional methods added or >>> existing methods removed. However there are some improvements that can be >>> done throughout the REST API that can contribute to a better, more >>> standardized API. Some of these suggestions have been under discussion for >>> sometime, and this mail will serve as a place to consolidate them all. >>> >>> *1. Status Codes * >>> There are a number of issues that are related to wrong or no status >>> codes being returned from the API methods. For an example, most GET methods >>> used to return 200 Ok status even for empty result sets. I have fixed most >>> of the issues, however IMO if we can stick to a fixed, consistent, small >>> range of status codes, that can greatly improve the usage experience for >>> the API. >>> >>> Following are the status codes that we are returning now. >>> >>> >>> - *200 Ok* - GET, DELETE, POST, PUT >>> - *201 Created* - POST >>> - *202 Accepted* - Used only twice in deployApplication and >>> undeployApplication POST methods >>> - *209 No Content* - Once in a DELETE method, >>> removeKubernetesHostCluster >>> - *404 Not Found *- GET, POST, PUT, DELETE >>> - *409 Conflict* - POST, PUT >>> - *400 Bad Request* - for RestApiException which is used frequently >>> losing the meaning of the status code >>> - *500 Internal Server Error* >>> >>> >>> We can improve this range by shortening down the status codes being sent >>> back, and avoiding using invalid status codes for certain operations. What >>> I'm proposing looks something like this. >>> >>> >>> - *200 Ok* - GET, DELETE (with content body), PUT - Success >>> - *201 Created* - POST - Resource Created, send Location header >>> - *404 Not Found* - GET, POST, PUT, DELETE >>> - *409 Conflict* - POST, PUT - Resource being added already exists >>> - *400 Bad Request* - Other request error (only) >>> - *500 Internal Server Error* - Error is on our side (which most >>> catch clauses should go to) >>> >>> >>> IMO we can drop *202* and *209* status codes as being too granular. We >>> are always returning a content body for every DELETE request being done. >>> Therefore a client expecting a *209 No Content *status code would not >>> make sense [1]. The case for *202 Accepted* is there, however we can >>> reduce our spectrum of status codes to expect for a client, by returning >>> *201 >>> Created* for deploy/undeploy operations as well. The status of the >>> application deployment will anyway will be queried by *GET >>> "/applications/app-id/runtime"*, so we can use 201 Created without an >>> issue. Furthermore we should stop sending 200 Ok for successful POST >>> operations. >>> >>> *2. Operation Types* >>> Following are the PUT operations that IMO need to be improved. >>> >>> Method NameResource PathSuggested ImprovementaddKubernetesHostPUT >>> "/kubernetesClusters/{kubernetesClusterId}/minion"*POST* >>> "/kubernetesClusters/{kubernetesClusterId}/minion"updateKubernetesHostPUT >>> "/kubernetes/update/host"PUT " >>> */kubernetesClusters/{kubernetesClusterId}/minion/{minionId}*" >>> updateDeploymentPolicyPUT "/deploymentPolicies"PUT "/deploymentPolicies >>> */{deploymentPolicyId}*"updateCartridgePUT "/cartridges"PUT "/cartridges >>> */{cartridgeId}*"updateApplicationPolicyPUT "/applicationPolicies"PUT >>> "/applicationPolicies*/{applicationPolicyId}*"updateAutoscalingPolicyPUT >>> "/autoscalingPolicies"PUT "/autoscalingPolicies*/{autoscalingPolicyId}*" >>> updateNetworkPartitionPUT "/networkPartitions"PUT "/networkPartitions >>> */{networkPartitionId}*"updateTenantPUT "/tenants"PUT "/tenants >>> */{tenantId}*"updateUserPUT "/users"PUT "/users*/{userId}*" >>> >>> PUT operations should point to the exact resource location where as POST >>> operations do not have to [2]. The other alternative to the improvements >>> suggested above is to handle updates for them in the POST operation itself, >>> however it make things more complex for a client using the API. >>> >>> *3. Exception Throwing* >>> Currently we are throwing RestAPIExceptions almost every time a server >>> side exception is caught. This results in *400 Bad Request* being >>> returned for requests that might actually be correct. It should only be >>> thrown for verified bad requests. For this to take place there should be >>> validations for requests and custom exceptions from the server side that >>> reflect the error status in a granular manner. As a side note to this, as >>> I've observed, currently no custom exception from the Autoscaler component >>> is correctly deserialized and mapped at the stub and therefore only an >>> AxisFault is thrown from the server. This should be investigated. >>> >>> *4. Response structure* >>> Currently there are two bean classes for success and error responses >>> (SuccessResponseBean and ErrorResponseBean). From the Jaggery code, based >>> on the status code range, the successMessage or errorMessage is printed. >>> The status code is also sent as statusCode (SuccessResponseBean) and >>> errorCode (ErrorResponseBean). IMO this can be unified in to one response >>> bean. Furthermore, the response should not contain two status codes, which >>> can result in frequent different status codes being returned for one >>> request. Since the API is mostly returning consistent status codes, the use >>> of two status codes can be avoided. >>> >>> *{"statusCode":202,"successMessage":"Application deployed successfully: >>> [application] complex-app"}* >>> >>> >>> >>> These are the improvements that I was able to recognize by going through >>> the code. If you have any input on these please feel free to add. Although >>> some of these can be pushed for a later release, IMO others would greatly >>> improve the API status for the next release. >>> >>> [1] - http://tools.ietf.org/html/rfc7231#section-4.3.5 >>> [2] - http://restcookbook.com/HTTP%20Methods/put-vs-post/ >>> >>> Regards, >>> Chamila de Alwis >>> Software Engineer | WSO2 | +94772207163 >>> Blog: code.chamiladealwis.com >>> >>> >>> >> >> >> -- >> -- >> Lahiru Sandaruwan >> Committer and PMC member, Apache Stratos, >> Senior Software Engineer, >> WSO2 Inc., http://wso2.com >> lean.enterprise.middleware >> >> phone: +94773325954 >> email: lahi...@wso2.com blog: http://lahiruwrites.blogspot.com/ >> linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146 >> >> > > > -- > *Thanks and Regards,* > Anuruddha Lanka Liyanarachchi > Software Engineer - WSO2 > Mobile : +94 (0) 712762611 > Tel : +94 112 145 345 > a <thili...@wso2.com>nurudd...@wso2.com > > -- > <nurudd...@wso2.com> > <nurudd...@wso2.com> > Thanks and Regards, > > Isuru H. > <nurudd...@wso2.com> > +94 716 358 048 <nurudd...@wso2.com>* <http://wso2.com/>* > > > * <http://wso2.com/>* > > >