You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stratos.apache.org by Chamila De Alwis <ch...@wso2.com> on 2015/04/18 21:10:15 UTC
[Discuss] REST API Improvements
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
Re: [Discuss] REST API Improvements
Posted by Dinithi De Silva <di...@wso2.com>.
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 <ga...@wso2.com> wrote:
> Good analysis Chamila.
>
> Please find some inline comments below.
>
> On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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 : gayang@wso2.com | mobile : +94 766819985
>
>
--
*Dinithi De Silva*
Associate Software Engineer, WSO2 Inc.
m:+94716667655 | e:dinithis@wso2.com | w: www.wso2.com
| a: #20, Palm Grove, Colombo 03
Re: [Discuss] REST API Improvements
Posted by Gayan Gunarathne <ga...@wso2.com>.
Good analysis Chamila.
Please find some inline comments below.
On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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 : gayang@wso2.com | mobile : +94 766819985
Re: [Discuss] REST API Improvements
Posted by Isuru Haththotuwa <is...@apache.org>.
Hi Chamila,
Nice work in analyzing and summarizing the current limitations and possible
improvements. Please find some comments inline
On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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.
>
+1, However we should always expose whatever meaningful information we can
from 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.
>
IMHO for undeployment, it does not make sense to return 201 Created; we
are actually removing a resource.
>
> *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 for using PUT for updating.
>
>
> *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.
>
+1. A Server side exception should throw 500 IMHO. The request validation
should catch if the request is missing some required information,
malformed, etc.
>
> *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.
>
+1. But, should return the HTTP status code from the rest API IMHO, since
its a standard.
>
> *{"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
>
> --
> <http://code.chamiladealwis.com>
> <http://code.chamiladealwis.com>
> Thanks and Regards,
>
> Isuru H.
> <http://code.chamiladealwis.com>
> +94 716 358 048 <http://code.chamiladealwis.com>* <http://wso2.com/>*
>
>
> * <http://wso2.com/>*
>
>
>
Re: [Discuss] REST API Improvements
Posted by Lahiru Sandaruwan <la...@wso2.com>.
On Sun, Apr 19, 2015 at 12:42 PM, Chamila De Alwis <ch...@wso2.com>
wrote:
> Hi Lahiru,
>
> On Sun, Apr 19, 2015 at 10:23 AM, Lahiru Sandaruwan <la...@wso2.com>
> wrote:
>
>> If someone did a PUT with a resource which does not exist, it should add
>> the resource and return 201, right?
>>
>
> Thanks for the feedback. Yes, PUT is required to create a resource and
> send 201 Created if it doesn't exist, however IMO we should advise against
> using update methods to create resources since we have a set of POST
> methods to do the same task.
>
Yah, We do not advice. But i think we should handle that case as well,
since it is the standard way of handling PUT.
Thanks.
>
> There are a number of JIRA tickets for wrong status code related issues. I
> will create the additional ones.
>
>
> 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: lahirus@wso2.com blog: http://lahiruwrites.blogspot.com/
linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146
Re: [Discuss] REST API Improvements
Posted by Chamila De Alwis <ch...@wso2.com>.
Hi Lahiru,
On Sun, Apr 19, 2015 at 10:23 AM, Lahiru Sandaruwan <la...@wso2.com>
wrote:
> If someone did a PUT with a resource which does not exist, it should add
> the resource and return 201, right?
>
Thanks for the feedback. Yes, PUT is required to create a resource and send
201 Created if it doesn't exist, however IMO we should advise against using
update methods to create resources since we have a set of POST methods to
do the same task.
There are a number of JIRA tickets for wrong status code related issues. I
will create the additional ones.
Regards,
Chamila de Alwis
Software Engineer | WSO2 | +94772207163
Blog: code.chamiladealwis.com
Re: [Discuss] REST API Improvements
Posted by Imesh Gunaratne <im...@apache.org>.
+1 for suggestions except for below, we should be able to do these for
4.1.0 GA.
IMO we can drop *202* and *209* status codes as being too granular.
>
202 Accepted might be needed because application deployment and
un-deployment are being executed in the background.
On Sun, Apr 19, 2015 at 10:23 AM, Lahiru Sandaruwan <la...@wso2.com>
wrote:
> Hi Chamila,
>
> Great analysis,
>
> Please find the comments inline,
>
> On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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: lahirus@wso2.com blog: http://lahiruwrites.blogspot.com/
> linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146
>
>
--
Imesh Gunaratne
Technical Lead, WSO2
Committer & PMC Member, Apache Stratos
Re: [Discuss] REST API Improvements
Posted by Lahiru Sandaruwan <la...@wso2.com>.
Hi Chamila,
Great analysis,
Please find the comments inline,
On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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: lahirus@wso2.com blog: http://lahiruwrites.blogspot.com/
linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146
Re: [Discuss] REST API Improvements
Posted by Manula Thantriwatte <ma...@gmail.com>.
Hi,
+1 for these changes.
On Sun, Apr 19, 2015 at 3:10 AM, Chamila De Alwis <ch...@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
>
>
>
--
Regards,
Manula Chathurika Thantriwatte
phone : (+65) 91306970
email : manulachathurika@gmail.com
Linkedin : *http://lk.linkedin.com/in/manulachathurika
<http://lk.linkedin.com/in/manulachathurika>*
blog : http://manulachathurika.blogspot.com/
Re: [Discuss] REST API Improvements
Posted by Isuru Haththotuwa <is...@apache.org>.
Hi Chamila,
On Tue, Apr 28, 2015 at 5:00 PM, Chamila De Alwis <ch...@wso2.com> wrote:
> Hi Isuru,
>
> On Tue, Apr 28, 2015 at 4:36 PM, Isuru Haththotuwa <is...@apache.org>
> wrote:
>
>> IMHO we should expose whatever information we can from the Rest API,
>> since its the interaction point with Stratos. In a scenario where a third
>> party would need to integrate with Stratos via the Rest API it will be
>> helpful if Rest API provides such detailed information. Even when a user is
>> using the CLI or the UI, IMHO its meaningful to let the user know what
>> exactly went wrong. Just my two cents. If the community agrees, I'm ok with
>> using 500 Internal Server error here.
>>
>
> Yes, there should be enough information for a user to figure out what went
> wrong. However it is also important to keep the expected response range to
> a manageable level, so that the API users will be able to manage what
> responses to work on. On that point of view, IMO only a 500 Internal Server
> Error is descriptive enough. Additionally we will be passing a fault string
> in the content body, which will help the user to understand what went
> wrong. Other than that, since most faults which fall in to this category
> cannot be remedied by a client, IMO we don't need that level of granularity.
>
IMHO even if the error cannot be remedied by the client, it's still
important to indicate what the cause is. If we use Internal server error
here, it might mean a bug in the code, or even a connection failure
(RemoteException) or such an issue which is not within the control of
Stratos.
However, since the majority is ok with having 500 internal server error
here, +1 for that + meaningful a response message as Chamila explained.
>
>
> Regards,
> Chamila de Alwis
> Software Engineer | WSO2 | +94772207163
> Blog: code.chamiladealwis.com
>
> --
> <http://code.chamiladealwis.com>
> <http://code.chamiladealwis.com>
> Thanks and Regards,
>
> Isuru H.
> <http://code.chamiladealwis.com>
> +94 716 358 048 <http://code.chamiladealwis.com>* <http://wso2.com/>*
>
>
> * <http://wso2.com/>*
>
>
>
Re: [Discuss] REST API Improvements
Posted by Chamila De Alwis <ch...@wso2.com>.
Hi Isuru,
On Tue, Apr 28, 2015 at 4:36 PM, Isuru Haththotuwa <is...@apache.org>
wrote:
> IMHO we should expose whatever information we can from the Rest API, since
> its the interaction point with Stratos. In a scenario where a third party
> would need to integrate with Stratos via the Rest API it will be helpful if
> Rest API provides such detailed information. Even when a user is using the
> CLI or the UI, IMHO its meaningful to let the user know what exactly went
> wrong. Just my two cents. If the community agrees, I'm ok with using 500
> Internal Server error here.
>
Yes, there should be enough information for a user to figure out what went
wrong. However it is also important to keep the expected response range to
a manageable level, so that the API users will be able to manage what
responses to work on. On that point of view, IMO only a 500 Internal Server
Error is descriptive enough. Additionally we will be passing a fault string
in the content body, which will help the user to understand what went
wrong. Other than that, since most faults which fall in to this category
cannot be remedied by a client, IMO we don't need that level of
granularity.
Regards,
Chamila de Alwis
Software Engineer | WSO2 | +94772207163
Blog: code.chamiladealwis.com
Re: [Discuss] REST API Improvements
Posted by Isuru Haththotuwa <is...@apache.org>.
Hi Lahiru,
On Tue, Apr 28, 2015 at 3:50 PM, Lahiru Sandaruwan <la...@wso2.com> wrote:
> Hi Isuru,
>
> Thanks for the input.
>
> On Tue, Apr 28, 2015 at 3:41 PM, Isuru Haththotuwa <is...@apache.org>
> wrote:
>
>> 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?
>>
>
> Aggree that this would be giving better information. But from the Rest API
> user POV, this is an internal issue, even though it is a timeout/ network
> issue actually.
>
> IMO we do not need to give Autoscaler service or Cloud controller service
> issues(Network/ timeout) to the Rest API user.
>
IMHO we should expose whatever information we can from the Rest API, since
its the interaction point with Stratos. In a scenario where a third party
would need to integrate with Stratos via the Rest API it will be helpful if
Rest API provides such detailed information. Even when a user is using the
CLI or the UI, IMHO its meaningful to let the user know what exactly went
wrong. Just my two cents. If the community agrees, I'm ok with using 500
Internal Server error here.
>
> Thanks.
>
>>
>> On Tue, Apr 28, 2015 at 2:43 PM, Anuruddha Liyanarachchi <
>> anuruddhal@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 <la...@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 <ch...@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: lahirus@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 <th...@wso2.com>nuruddhal@wso2.com
>>>
>>> --
>>> <nu...@wso2.com>
>>> <nu...@wso2.com>
>>> Thanks and Regards,
>>>
>>> Isuru H.
>>> <nu...@wso2.com>
>>> +94 716 358 048 <nu...@wso2.com>* <http://wso2.com/>*
>>>
>>>
>>> * <http://wso2.com/>*
>>>
>>>
>>>
>
>
> --
> --
> Lahiru Sandaruwan
> Committer and PMC member, Apache Stratos,
> Senior Software Engineer,
> WSO2 Inc., http://wso2.com
> lean.enterprise.middleware
>
> phone: +94773325954
> email: lahirus@wso2.com blog: http://lahiruwrites.blogspot.com/
> linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146
>
> --
> <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>
> <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>
> Thanks and Regards,
>
> Isuru H.
> <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>
> +94 716 358 048 <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>*
> <http://wso2.com/>*
>
>
> * <http://wso2.com/>*
>
>
>
Re: [Discuss] REST API Improvements
Posted by Lahiru Sandaruwan <la...@wso2.com>.
Hi Isuru,
Thanks for the input.
On Tue, Apr 28, 2015 at 3:41 PM, Isuru Haththotuwa <is...@apache.org>
wrote:
> 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?
>
Aggree that this would be giving better information. But from the Rest API
user POV, this is an internal issue, even though it is a timeout/ network
issue actually.
IMO we do not need to give Autoscaler service or Cloud controller service
issues(Network/ timeout) to the Rest API user.
Thanks.
>
> On Tue, Apr 28, 2015 at 2:43 PM, Anuruddha Liyanarachchi <
> anuruddhal@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 <la...@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 <ch...@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: lahirus@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 <th...@wso2.com>nuruddhal@wso2.com
>>
>> --
>> <nu...@wso2.com>
>> <nu...@wso2.com>
>> Thanks and Regards,
>>
>> Isuru H.
>> <nu...@wso2.com>
>> +94 716 358 048 <nu...@wso2.com>* <http://wso2.com/>*
>>
>>
>> * <http://wso2.com/>*
>>
>>
>>
--
--
Lahiru Sandaruwan
Committer and PMC member, Apache Stratos,
Senior Software Engineer,
WSO2 Inc., http://wso2.com
lean.enterprise.middleware
phone: +94773325954
email: lahirus@wso2.com blog: http://lahiruwrites.blogspot.com/
linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146
Re: [Discuss] REST API Improvements
Posted by Isuru Haththotuwa <is...@apache.org>.
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 <
anuruddhal@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 <la...@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 <ch...@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: lahirus@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 <th...@wso2.com>nuruddhal@wso2.com
>
> --
> <nu...@wso2.com>
> <nu...@wso2.com>
> Thanks and Regards,
>
> Isuru H.
> <nu...@wso2.com>
> +94 716 358 048 <nu...@wso2.com>* <http://wso2.com/>*
>
>
> * <http://wso2.com/>*
>
>
>
Re: [Discuss] REST API Improvements
Posted by Anuruddha Liyanarachchi <an...@wso2.com>.
+1 for throwing *500 Internal Server Error *for all the RemoteException.
On Tue, Apr 28, 2015 at 2:23 PM, Lahiru Sandaruwan <la...@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 <ch...@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: lahirus@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 <th...@wso2.com>nuruddhal@wso2.com
Re: [Discuss] REST API Improvements
Posted by Gayan Gunarathne <ga...@wso2.com>.
On Tue, Apr 28, 2015 at 2:23 PM, Lahiru Sandaruwan <la...@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?
>
+1. Those will be a internal issues need to fix.Those cases we can send the
500 response.
> Thanks.
>
> On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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: lahirus@wso2.com blog: http://lahiruwrites.blogspot.com/
> linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146
>
>
--
Gayan Gunarathne
Technical Lead
WSO2 Inc. (http://wso2.com)
email : gayang@wso2.com | mobile : +94 766819985
Re: [Discuss] REST API Improvements
Posted by Lahiru Sandaruwan <la...@wso2.com>.
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 <ch...@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: lahirus@wso2.com blog: http://lahiruwrites.blogspot.com/
linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146
Re: [Discuss] REST API Improvements
Posted by Anuruddha Liyanarachchi <an...@wso2.com>.
Hi,
As per the present implementation the ID will be read from the JSON file,
not from the parameter. Therefore two ID's can mismatch.
In order to prevent ID mismatch we need to validate two IDs in the REST API.
On Mon, Apr 20, 2015 at 3:24 PM, Dinithi De Silva <di...@wso2.com> wrote:
> Yes Vishanth. We need to change that too. There was a separate mail thread
> discussing about the response message structure. ([Question] Is is correct
> for two different status codes appear when adding a tenant?).
> We need to change successMessage to statusMessage in the response,but
> still there isn't any final decision on removing the status code from the
> response message.
>
> On Mon, Apr 20, 2015 at 3:13 PM, Vishanth Balasubramaniam <
> vishanthb@wso2.com> wrote:
>
>>
>>
>> On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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"}*
>>>
>>>
>> I think some time back we decided to change the successMessage to
>> statusMessage in the response structure. We can make that change too.
>>
>>
>>>
>>> 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
>>>
>>>
>>>
>>
>>
>> --
>> *Vishanth Balasubramaniam*
>> Software Engineer
>> WSO2 Inc.; http://wso2.com
>> lean.enterprise.middleware
>>
>> mobile: *+94771737718*
>> about me: *http://about.me/vishanth <http://about.me/vishanth>*
>>
>>
>
>
> --
> *Dinithi De Silva*
> Associate Software Engineer, WSO2 Inc.
> m:+94716667655 | e:dinithis@wso2.com | w: www.wso2.com
> | a: #20, Palm Grove, Colombo 03
>
--
*Thanks and Regards,*
Anuruddha Lanka Liyanarachchi
Software Engineer - WSO2
Mobile : +94 (0) 712762611
Tel : +94 112 145 345
a <th...@wso2.com>nuruddhal@wso2.com
Re: [Discuss] REST API Improvements
Posted by Dinithi De Silva <di...@wso2.com>.
Yes Vishanth. We need to change that too. There was a separate mail thread
discussing about the response message structure. ([Question] Is is correct
for two different status codes appear when adding a tenant?).
We need to change successMessage to statusMessage in the response,but still
there isn't any final decision on removing the status code from the
response message.
On Mon, Apr 20, 2015 at 3:13 PM, Vishanth Balasubramaniam <
vishanthb@wso2.com> wrote:
>
>
> On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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"}*
>>
>>
> I think some time back we decided to change the successMessage to
> statusMessage in the response structure. We can make that change too.
>
>
>>
>> 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
>>
>>
>>
>
>
> --
> *Vishanth Balasubramaniam*
> Software Engineer
> WSO2 Inc.; http://wso2.com
> lean.enterprise.middleware
>
> mobile: *+94771737718*
> about me: *http://about.me/vishanth <http://about.me/vishanth>*
>
>
--
*Dinithi De Silva*
Associate Software Engineer, WSO2 Inc.
m:+94716667655 | e:dinithis@wso2.com | w: www.wso2.com
| a: #20, Palm Grove, Colombo 03
Re: [Discuss] REST API Improvements
Posted by Vishanth Balasubramaniam <vi...@wso2.com>.
On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <ch...@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"}*
>
>
I think some time back we decided to change the successMessage to
statusMessage in the response structure. We can make that change too.
>
> 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
>
>
>
--
*Vishanth Balasubramaniam*
Software Engineer
WSO2 Inc.; http://wso2.com
lean.enterprise.middleware
mobile: *+94771737718*
about me: *http://about.me/vishanth <http://about.me/vishanth>*