refactor GET object operations #824

aead · 2017-09-25T22:37:04Z

This change refactors get-object calls to take GetOptions.
This is required to use client and server side encryption within
minio-go.

Updates #823

poornas · 2017-09-25T22:56:36Z

travis fail - your examples need to be updated.

aead · 2017-09-25T22:57:38Z

Oh, yes, thanks @poornas 👍

poornas · 2017-09-26T00:42:21Z

api-stat.go

@@ -81,20 +81,19 @@ func extractObjMetadata(header http.Header) http.Header {
 }

 // StatObject verifies if object exists and you have permission to access.
-func (c Client) StatObject(bucketName, objectName string) (ObjectInfo, error) {
+func (c Client) StatObject(bucketName, objectName string, opts GetOptions) (ObjectInfo, error) {


Api.md needs to document this.

poornas · 2017-09-26T00:45:02Z

api-get-object-context.go

-func (c Client) GetObjectWithContext(ctx context.Context, bucketName, objectName string) (*Object, error) {
-	return c.getObjectWithContext(ctx, bucketName, objectName)
+func (c Client) GetObjectWithContext(ctx context.Context, bucketName, objectName string, opts GetOptions) (*Object, error) {
+	return c.getObjectWithContext(ctx, bucketName, objectName, opts)


needs documentation update for GetObjectWithContext and FGetObjectWithContext

poornas · 2017-09-26T00:45:59Z

api-get-options.go

-	http.Header
+// GetOptions are used to specify additional headers or options
+// during GET requests.
+type GetOptions struct {


Can we rename this GetObjectOptions to be symmetric to the PutObjectOptions

My plan was to rename PutObjectOptions -> PutOptions during encryption refactoring.
I can ask Harsha

Actually it is more consistent to keep GetObjectOptions() instead of GetOptions() GetOptions is just too generic.

poornas · 2017-09-26T00:47:44Z

api-get-object-file.go

@@ -72,7 +72,7 @@ func (c Client) fGetObjectWithContext(ctx context.Context, bucketName, objectNam
 	}



needs a FGetEncryptedObject variant to match the FPutEncryptedObject.

poornas · 2017-09-26T22:36:20Z

docs/API.md

@@ -831,7 +870,7 @@ if err != nil {
 }
 ```
 <a name="StatObject"></a>
-### StatObject(bucketName, objectName string) (ObjectInfo, error)
+### StatObject(bucketName, objectName string, opts GetObjectOptions) (ObjectInfo, error)


It looks odd to pass GetObjectOptions to a HEAD call.

The problem is a SSE-C call requires encryption keys - Otherwise there would be no way to get stats of an SSE-C object...

Perhaps StatObject(bucketName, objectName string, opts StatObjectOptions) --> GetObjectOptions --> StatObjectOptions

poornas · 2017-09-26T22:42:11Z

examples/s3/getobject-context.go

@@ -47,7 +47,7 @@ func main() {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
 	defer cancel()

-	reader, err := s3Client.GetObjectWithContext(ctx, "my-bucketname", "my-objectname")
+	reader, err := s3Client.GetObjectWithContext(ctx, "my-bucketname", "my-objectname", minio.GetObjectOptions{})


would be good to have the example demonstrate use of the options

harshavardhana · 2017-09-27T02:34:34Z

Looks like there is a formatting issue @aead can you fix?

poornas · 2017-09-27T17:02:02Z

@aead, travis fails for the example. A couple of minor additions required - FGetEncryptedObject test needed for functional tests. Also believe readme.md is missing link to the example. Are we going to have a StatObjectOptions?

harshavardhana · 2017-09-27T17:08:28Z

@aead, travis fails for the example. A couple of minor additions required - FGetEncryptedObject test needed for functional tests. Also believe readme.md is missing link to the example. Are we going to have a StatObjectOptions?

We can have a simple

// GetObjectOptions are used to specify additional headers or options
// during GET requests.
type GetObjectOptions struct {
	headers map[string]string

	UserMetadata map[string]string
	Materials    encrypt.Materials
}

type StatObjectOptions GetObjectOptions

aead · 2017-09-27T17:24:35Z

@harshavardhana
Won't work nicely. The problem is that we would use Go's type system in a wrong way.
E.g. All functions of GetObjectOptions are not accessible (strong type system).
A solution would be:

type StatObjectOptions struct {
     GetObjectOptions
}

But this will lead to weird constructions like StatObjectOptions{GetObjectObptions{}}

harshavardhana · 2017-09-27T18:45:25Z

A solution would be:

type StatObjectOptions struct {
GetObjectOptions
}
But this will lead to weird constructions like StatObjectOptions{GetObjectObptions{}}

This would be do-able

type StatObjectOptions struct {
     GetObjectOptions
}

StatObjectOptions{} would initialize properly and inherit all the methods.

houndci-bot · 2017-09-27T19:02:49Z

api-get-options.go

-func NewHeadReqHeaders() RequestHeaders {
-	return RequestHeaders{
-		Header: make(http.Header),
+// set adds a key value pair to the options. The


comment on exported method GetObjectOptions.Set should be of the form "Set ..."

poornas

LGTM. Needs rebase

This change refactors get-object calls to take GetOptions. This is required to use client and server side encryption within minio-go.

aead force-pushed the rf-get-object branch 2 times, most recently from 4daeec4 to f7ddad6 Compare September 26, 2017 00:02

deekoder requested review from poornas and harshavardhana September 26, 2017 00:35

poornas requested changes Sep 26, 2017

View reviewed changes

aead force-pushed the rf-get-object branch from c50572a to 233c413 Compare September 26, 2017 22:08

poornas reviewed Sep 26, 2017

View reviewed changes

harshavardhana approved these changes Sep 27, 2017

View reviewed changes

aead force-pushed the rf-get-object branch from cb19676 to 87e3b62 Compare September 27, 2017 19:02

houndci-bot reviewed Sep 27, 2017

View reviewed changes

aead force-pushed the rf-get-object branch 3 times, most recently from ea523f8 to eb3e78a Compare September 27, 2017 20:23

poornas approved these changes Sep 27, 2017

View reviewed changes

refactor GET object operations

5f80bfe

This change refactors get-object calls to take GetOptions. This is required to use client and server side encryption within minio-go.

aead force-pushed the rf-get-object branch from eb3e78a to 5f80bfe Compare September 27, 2017 21:49

deekoder merged commit cb3571b into minio:master Sep 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor GET object operations #824

refactor GET object operations #824

aead commented Sep 25, 2017

poornas commented Sep 25, 2017

aead commented Sep 25, 2017

poornas Sep 26, 2017

poornas Sep 26, 2017

poornas Sep 26, 2017

aead Sep 26, 2017

harshavardhana Sep 26, 2017

aead Sep 26, 2017

poornas Sep 26, 2017

poornas Sep 26, 2017

aead Sep 26, 2017

harshavardhana Sep 26, 2017 •

edited

poornas Sep 26, 2017

harshavardhana commented Sep 27, 2017

poornas commented Sep 27, 2017

harshavardhana commented Sep 27, 2017 •

edited

aead commented Sep 27, 2017

harshavardhana commented Sep 27, 2017

houndci-bot Sep 27, 2017

poornas left a comment

		@@ -72,7 +72,7 @@ func (c Client) fGetObjectWithContext(ctx context.Context, bucketName, objectNam
		}

refactor GET object operations #824

refactor GET object operations #824

Conversation

aead commented Sep 25, 2017

poornas commented Sep 25, 2017

aead commented Sep 25, 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

Choose a reason for hiding this comment

harshavardhana Sep 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana commented Sep 27, 2017

poornas commented Sep 27, 2017

harshavardhana commented Sep 27, 2017 • edited

aead commented Sep 27, 2017

harshavardhana commented Sep 27, 2017

Choose a reason for hiding this comment

poornas left a comment

Choose a reason for hiding this comment

harshavardhana Sep 26, 2017 •

edited

harshavardhana commented Sep 27, 2017 •

edited