Add new CopyObject API in Core client #836

harshavardhana · 2017-09-30T17:21:51Z

This PR also fixes the problem of PutObject
call not handling additional headers.

Refer original problem here minio/minio#5000

poornas · 2017-10-02T06:18:21Z

core.go

-	return c.putObjectDo(context.Background(), bucket, object, data, md5Sum, sha256Sum, size, PutObjectOptions{UserMetadata: metadata})
+	delKey := func(m map[string]string, key string) {
+		delete(m, strings.ToLower(key))
+		delete(m, strings.ToUpper(key))


will miss key in mixed-case.

can you give me an example?

example - if "Content-Type" is a key in the metadata map, the above code will not delete the entry.

Oops yeah i should have used http.CanonicalHeaderKey()

poornas · 2017-10-02T06:39:36Z

core.go

+	if v := getKeyValue(metadata, "content-encoding"); v != "" {
+		opts.ContentEncoding = v
+		delKey(metadata, "content-encoding")
+	}


opts := PutObjectOptions{} m := make(map[string]string) for k, v := range metadata { if strings.ToLower(k) == "content-encoding" { opts.ContentEncoding = v } else if strings.ToLower(k) == "content-disposition" { opts.ContentDisposition = v } else if strings.ToLower(k) == "content-type" { opts.ContentType = v } else if strings.ToLower(k) == "cache-control" { opts.CacheControl = v } else { m[k] = metadata[k] } } opts.UserMetadata = m

this would be more concise

This makes sense..

poornas · 2017-10-02T06:40:54Z

core.go

+// CopyObject - copies an object from source object to destination object on server side.
+func (c Core) CopyObject(sourceBucket, sourceObject, destBucket, destObject string, metadata map[string]string) (ObjectInfo, error) {
+	return c.copyObjectDo(context.Background(), sourceBucket, sourceObject, destBucket, destObject, metadata)
+}


need tests.

This PR also fixes the problem of PutObject call not handling additional headers. Refer original problem here minio/minio#5000

donatello · 2017-10-02T16:20:30Z

Sorry to come in late on this. The bug identified minio/minio#5000 (comment) can be fixed properly in ComposeObject().

Particularly, this part https://github.com/minio/minio-go/blob/master/api-compose-object.go#L110-L122 needs to be fixed to handle the standard headers (like Content-Encoding, etc). IIUC, then this PR is not needed (except perhaps to fix PutObject)?

harshavardhana · 2017-10-02T16:37:52Z

Sorry to come in late on this. The bug identified minio/minio#5000 (comment) can be fixed properly in ComposeObject().

Particularly, this part https://github.com/minio/minio-go/blob/master/api-compose-object.go#L110-L122 needs to be fixed to handle the standard headers (like Content-Encoding, etc). IIUC, then this PR is not needed (except perhaps to fix PutObject)?

No we need to add a simpler version @donatello - we shouldn't use ComposeObject IMHO. Which uses multipart API underneath.

donatello · 2017-10-02T17:01:53Z

No we need to add a simpler version @donatello - we shouldn't use ComposeObject IMHO. Which uses multipart API underneath.

@harshavardhana Got it, you're right. ComposeObject would also do a HEAD, which is not desirable in gateway mode where we do expect it to be a one-to-one API mapping.

donatello · 2017-10-02T17:02:42Z

api-s3-datatypes.go

@@ -128,7 +128,7 @@ type initiator struct {
 // copyObjectResult container for copy object response.
 type copyObjectResult struct {
 	ETag         string
-	LastModified string // time string format "2006-01-02T15:04:05.000Z"
+	LastModified time.Time // time string format "2006-01-02T15:04:05.000Z"


How would xmlDecoder() know the format (time layout) with which to parse the time? I am not sure the decoding will be correct as is.

that is the feature of xml Marshal @donatello also we need to have typed value.

For example look at all the List() APIs.

@harshavardhana - xml decoder is using a particular format within the ISO8601 standard by default to parse timestamps. Looks like by dumb luck AWS is using the same format!

donatello · 2017-10-02T17:04:59Z

core.go

+			opts.ContentType = v
+		} else if strings.ToLower(k) == "cache-control" {
+			opts.CacheControl = v
+		} else {


Two headers seem to be missing from this list - content-length and content-md5 according to https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPUT.html

that is set internally @donatello

donatello · 2017-10-02T17:06:51Z

api-compose-object.go

+	// Set all the metadata headers.
+	for k, v := range metadata {
+		headers.Set(k, v)
+	}


For metadata replacement to actually happen we need to make sure the x-amz-metadata-directive header is set to REPLACE. Perhaps we should throw an error if metadata is non-empty and this header is not set?

You are right let me do that.

actually we don't need to.. there is no error @donatello according to CopyObject() if REPLACE is set then the set metadata is honored, otherwise it does copy from the source object by default. So we don't need to error out.

Ah yes. It is not needed, as AWS behaviour is also to silently ignore the headers without the directive header.

deekoder requested review from donatello and poornas October 1, 2017 16:50

poornas requested changes Oct 2, 2017

View reviewed changes

harshavardhana force-pushed the fix-copy-object branch from 7b6a531 to e819428 Compare October 2, 2017 07:30

Add new CopyObject API in Core client

9af77c8

This PR also fixes the problem of PutObject call not handling additional headers. Refer original problem here minio/minio#5000

harshavardhana force-pushed the fix-copy-object branch from e819428 to 9af77c8 Compare October 2, 2017 07:49

harshavardhana mentioned this pull request Oct 2, 2017

Allow CopyObject() in S3 gateway to support metadata minio/minio#5000

Merged

8 tasks

donatello requested changes Oct 2, 2017

View reviewed changes

donatello approved these changes Oct 2, 2017

View reviewed changes

poornas approved these changes Oct 2, 2017

View reviewed changes

deekoder merged commit 9690dc6 into minio:master Oct 2, 2017

harshavardhana deleted the fix-copy-object branch October 2, 2017 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new CopyObject API in Core client #836

Add new CopyObject API in Core client #836

harshavardhana commented Sep 30, 2017

poornas Oct 2, 2017

harshavardhana Oct 2, 2017

poornas Oct 2, 2017

harshavardhana Oct 2, 2017

poornas Oct 2, 2017

harshavardhana Oct 2, 2017

poornas Oct 2, 2017

harshavardhana Oct 2, 2017

donatello commented Oct 2, 2017 •

edited

Loading

harshavardhana commented Oct 2, 2017

donatello commented Oct 2, 2017

donatello Oct 2, 2017

harshavardhana Oct 2, 2017

harshavardhana Oct 2, 2017 •

edited

Loading

donatello Oct 2, 2017

donatello Oct 2, 2017

harshavardhana Oct 2, 2017

donatello Oct 2, 2017

harshavardhana Oct 2, 2017

harshavardhana Oct 2, 2017

donatello Oct 2, 2017

Add new CopyObject API in Core client #836

Add new CopyObject API in Core client #836

Conversation

harshavardhana commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donatello commented Oct 2, 2017 • edited Loading

harshavardhana commented Oct 2, 2017

donatello commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donatello commented Oct 2, 2017 •

edited

Loading

harshavardhana Oct 2, 2017 •

edited

Loading