Hi,
Nice work in analyzing these Chamila. Me and Anuruddha are going to do the
discussed changes to the REST API.
Thanks
On Sun, Apr 19, 2015 at 10:36 PM, Gayan Gunarathne <gay...@wso2.com> wrote:
> 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
>
>
--
*Dinithi De Silva*
Associate Software Engineer, WSO2 Inc.
m:+94716667655 | e:dinit...@wso2.com | w: www.wso2.com
| a: #20, Palm Grove, Colombo 03
