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
