Good analysis Chamila.
Please find some inline comments below.
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.
>
IMO If the failure happen with the server side validation, then it should
be a 400 bad request. But if something going wrong with the sever due to
some internal exception only we need to send the 500 internal server error.
> *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.
>
+1 to use the PUT for the update request.yeah.If we are using PUT to update
a resource completely you need to provide the specific resource id. If we
do not know the actual resource location and do not have any idea where to
store it, we can use POST and let the server decide.
>
> *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.
>
Yeah. I think we may need to identify what are the exceptions, we are going
to throw the internal server error. IMO Even it is a exception, every time
we can't send the internal server error response
>
> *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"}*
>
+1 .It is better to have unified response all the time
>
>
>
> 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
>
>
>
Thanks,
Gayan
--
Gayan Gunarathne
Technical Lead
WSO2 Inc. (http://wso2.com)
email : gay...@wso2.com | mobile : +94 766819985
