Hi Chamila,
Great analysis,
Please find the comments inline,
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
>
> If someone did a PUT with a resource which does not exist, it should add
the resource and return 201, right?
>
> -
> - *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.
>
This might be very bad for user experience. Could you please create a Jira?
Thanks.
>
> *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
