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/>*
>
>
>
