+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