Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong error code when creating a bucket that already exists #263

Closed
trociny opened this issue Oct 25, 2014 · 2 comments
Closed

Wrong error code when creating a bucket that already exists #263

trociny opened this issue Oct 25, 2014 · 2 comments
Assignees
Milestone

Comments

@trociny
Copy link
Contributor

trociny commented Oct 25, 2014

When creating a bucket that already exists, 500 (InternalError) is returned, which looks confusing for a user. According to Amazon documentation it should be 409 (BucketAlreadyExists) or 409 (BucketAlreadyOwnedByYou), depending on if the bucket belongs to the user or not:

http://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html

Below is my attempt to solve the issue.

leo_manager:

diff --git a/src/leo_manager_api.erl b/src/leo_manager_api.erl
index 52656ee..5bde219 100644
--- a/src/leo_manager_api.erl
+++ b/src/leo_manager_api.erl
@@ -1899,7 +1899,9 @@ add_bucket(AccessKey, Bucket, CannedACL) ->

     case leo_s3_bucket:head(AccessKeyBin, BucketBin) of
         ok ->
-            {error, ?ERROR_COULD_NOT_UPDATE_BUCKET};
+            {error, already_yours};
+        {error, forbidden} ->
+            {error, already_exists};
         not_found ->
             add_bucket_1(AccessKeyBin, BucketBin, CannedACL);
         {error, _} ->

leo_gateway:

diff --git a/include/leo_http.hrl b/include/leo_http.hrl
index 427ad3d..bfc89ac 100644
--- a/include/leo_http.hrl
+++ b/include/leo_http.hrl
@@ -88,6 +88,7 @@
 -define(HTTP_ST_BAD_REQ,             400).
 -define(HTTP_ST_FORBIDDEN,           403).
 -define(HTTP_ST_NOT_FOUND,           404).
+-define(HTTP_ST_CONFLICT,            409).
 -define(HTTP_ST_BAD_RANGE,           416).
 -define(HTTP_ST_INTERNAL_ERROR,      500).
 -define(HTTP_ST_SERVICE_UNAVAILABLE, 503).
@@ -128,6 +129,8 @@
 -define(XML_ERROR_CODE_InvalidRange,    "InvalidRange").
 -define(XML_ERROR_CODE_InternalError,   "InternalError").
 -define(XML_ERROR_CODE_ServiceUnavailable, "ServiceUnavailable").
+-define(XML_ERROR_CODE_BucketAlreadyExists, "BucketAlreadyExists").
+-define(XML_ERROR_CODE_BucketAlreadyOwnedByYou, "BucketAlreadyOwnedByYou").

 %% error messages used in a error response
 -define(XML_ERROR_MSG_EntityTooLarge,  "Your proposed upload exceeds the maximum allowed object size.").
@@ -137,6 +140,8 @@
 -define(XML_ERROR_MSG_InvalidRange,    "The requested range cannot be satisfied.").
 -define(XML_ERROR_MSG_InternalError,   "We encountered an internal error. Please try again.").
 -define(XML_ERROR_MSG_ServiceUnavailable,   "Please reduce your request rate.").
+-define(XML_ERROR_MSG_BucketAlreadyExists,     "Please select a different name and try again.").
+-define(XML_ERROR_MSG_BucketAlreadyOwnedByYou, "Your previous request to create the named bucket succeeded and you already own it.").

 %% Macros
 -define(reply_ok(_H,_R),                 cowboy_req:reply(?HTTP_ST_OK,              _H,_R)).    %% 200
@@ -157,6 +162,10 @@
         cowboy_req:reply(?HTTP_ST_BAD_REQ, _H,
                          io_lib:format(?XML_ERROR, [_Code, _Msg,
                                                     xmerl_lib:export_text(_Key), _ReqId]), _R)).
+-define(reply_conflict(_H, _Code, _Msg, _Key, _ReqId, _R),
+        cowboy_req:reply(?HTTP_ST_CONFLICT, _H,
+                         io_lib:format(?XML_ERROR, [_Code, _Msg,
+                                                    xmerl_lib:export_text(_Key), _ReqId]), _R)).
 -define(reply_forbidden(_H, _Key, _ReqId, _R),
         cowboy_req:reply(?HTTP_ST_FORBIDDEN, _H,
                          io_lib:format(?XML_ERROR, [?XML_ERROR_CODE_AccessDenied,
diff --git a/src/leo_gateway_s3_api.erl b/src/leo_gateway_s3_api.erl
index e038c8a..6373ad2 100644
--- a/src/leo_gateway_s3_api.erl
+++ b/src/leo_gateway_s3_api.erl
@@ -167,6 +167,15 @@ put_bucket(Req, Key, #req_params{access_key_id = AccessKeyId,
             ?reply_ok([?SERVER_HEADER], Req);
         {error, ?ERR_TYPE_INTERNAL_ERROR} ->
             ?reply_internal_error([?SERVER_HEADER], Key, <<>>, Req);
+        {error, invalid_access} ->
+            ?reply_bad_request([?SERVER_HEADER], ?XML_ERROR_CODE_AccessDenied,
+                               ?XML_ERROR_MSG_AccessDenied, Key, <<>>, Req);
+        {error, already_exists} ->
+            ?reply_conflict([?SERVER_HEADER], ?XML_ERROR_CODE_BucketAlreadyExists,
+                               ?XML_ERROR_MSG_BucketAlreadyExists, Key, <<>>, Req);
+        {error, already_yours} ->
+            ?reply_conflict([?SERVER_HEADER], ?XML_ERROR_CODE_BucketAlreadyOwnedByYou,
+                               ?XML_ERROR_MSG_BucketAlreadyOwnedByYou, Key, <<>>, Req);
         {error, timeout} ->
             ?reply_timeout([?SERVER_HEADER], Key, <<>>, Req)
     end;
@yosukehara
Copy link
Member

Thank you for your report and sharing the code. I'll check this issue.

@yosukehara yosukehara added this to the 1.2 milestone Oct 28, 2014
@yosukehara yosukehara self-assigned this Oct 28, 2014
@mocchira
Copy link
Member

I've fixed this issue by these two commits,
leo-project/leo_gateway@cf3c58c
leo-project/leo_manager@b2ea6e2
based on @trociny 's attempt with a tiny modification(in case of invalid_access).
This fixes will be included with 1.2.0-pre1.
Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants