bucketExists: Use a bool to indicate if a bucket exists or not

[vadmeste]

This is a breaking API change. bucketExists API will require
a callback with this signature cb(err, bool). This is a better
way to indicate whether a bucket exists or not.

Fixes #648

[vadmeste]

@ebozduman, I checked with aws s3api, this latter starts by making path style request with us-east-1 region in all cases, then when it receives 301 Moved Permanently, it will just follow the new location and update the region. However, I don't understand why we really need to follow the link since 301 Move Permanently also means the bucket exists.

[ebozduman]

Good point.
I also would like to remind you to add the reason why the request in the issue, about "bucketExists" api to just return true or false as its name implies, instead of throwing error messages, is not addressed. If we decided to not address that request for any reason, then it would be a good idea to explain the reason in the issue.

[vadmeste]

@ebozduman this PR is updated without following redirection. I think MovedPermanently status code is enough to indicate that a bucket exists.

[krishnasrinivas]

@vadmeste can you confirm that it works as expected with AWS-S3 bucket in non us-east-1 location

[vadmeste]

@vadmeste can you confirm that it works as expected with AWS-S3 bucket in non us-east-1 location

@krishnasrinivas, yeah it is already tested with AWS with non standard location, AWS returns 301 status code (permanently moved) and the code considers it as the bucket exists and doesn't return any error.

[krishnasrinivas]

@vadmeste I think it is better to be consistent on the request styles (virtual vs path) unless there is a strong reason so that the request styles are predictable. Also some S3 servers don't support path style requests (reason by minio-go now supports virtual-style as config option).

A better fix for this would be change the API of bucketExists like how @dcharbonnier has suggested here #648

So bucketExists will be called like s3Client.bucketExists('bucketname', (err, exists) =>{..})

This will fix it:

diff --git a/src/main/minio.js b/src/main/minio.js
index 38f8af1..d725e37 100644
--- a/src/main/minio.js
+++ b/src/main/minio.js
@@ -645,7 +645,13 @@ export class Client {
       throw new TypeError('callback should be of type "function"')
     }
     var method = 'HEAD'
-    this.makeRequest({method, bucketName}, '', 200, '', false, cb)
+    this.makeRequest({method, bucketName}, '', 200, '', false, err => {
+      if (err) {
+       if (err.code == 'NoSuchBucket' || err.code == 'NotFound') return cb(null, false)
+       return cb(err)
+      }
+      cb(null, true)
+    })
   }
 
   // Remove a bucket.

[vadmeste]

Also some S3 servers don't support path style requests

oups, I didn't know that.

Actually I was following "aws s3api head-bucket" command but also your suggestion seems to be fine and easier than the aws way. Will do it.

[vadmeste]

@krishnasrinivas @ebozduman, updated the PR. But this is a breaking change, is @abperiasamy okay with that ?

[dcharbonnier]

my 2 cents, if there is a plan to break compatibility maybe it would be time to start a 5.x milestone with more changes

[krishnasrinivas]

@vadmeste yes this change is correct. Previous API was implemented by me which was incorrect. Can you increment major version as well?

[vadmeste]

@krishnasrinivas, done, I set '5.0.0' in package.json.

[ruslan-polutsygan]

Latest version(4.0) still throws NoSuchBucket exception.

[ruslan-polutsygan]

Hm. Just seen that it is going to be in 5.0.

[ruslan-polutsygan]

And docs already say it returns boolean - https://docs.minio.io/docs/javascript-client-api-reference#bucketExists
:(

[krishnasrinivas]

@ruslan-polutsygan sorry about that :-( we will make a release soon.