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

Why does MinIO not support overriding the Response Header value when calling GetObject? #1755

Closed
reedchan7 opened this issue Jan 14, 2023 · 10 comments

Comments

@reedchan7
Copy link
Contributor

reedchan7 commented Jan 14, 2023

MinIO's GetObject Option only supports passing in Request Headers, not response-header parameters to override Response Headers, whereas Amazon S3's documentation indicates that such parameters are supported.

MinIO's GetObject Documentation:

image

Amazon S3 Documentation:

https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html

image

Request Syntax:

GET /Key+?partNumber=PartNumber&response-cache-control=ResponseCacheControl&response-content-disposition=ResponseContentDisposition&response-content-encoding=ResponseContentEncoding&response-content-language=ResponseContentLanguage&response-content-type=ResponseContentType&response-expires=ResponseExpires&versionId=VersionId HTTP/1.1

Now, let's look into the code of MinIO Go SDK, we can find that only the parameters of versionId and PartNumber are supported while building HTTP Request, while other parameters are not supported.

image

What considerations are behind this? When will this feature be supported?

MinIO Go SDK Version: v7.0.45

reedchan7 added a commit to reedchan7/minio-go-sdk that referenced this issue Jan 14, 2023
Support passing in a URL query string in GetObjectOptions.
reedchan7 added a commit to reedchan7/minio-go-sdk that referenced this issue Jan 14, 2023
Support passing in a URL query string in GetObjectOptions.
@klauspost
Copy link
Contributor

@reedchan7 Seems like we have an xy problem

If you describe what you are trying to achieve, instead of a solution to an unknown problem it would be a lot easier to evaluate.

response-content-type, response-content-language, response-content-disposition, response-content-encoding does not affect anything, since it would only make sense in a browser. You are receiving data back as a stream, so none of these matter.

If you are concerned about the headers returned, why not just ignore them, instead of manually overriding?

@reedchan7
Copy link
Contributor Author

reedchan7 commented Jan 14, 2023

@klauspost

We will provide services both on the command-line terminal and the web browser, so even though these Headers may not be effective on the command-line terminal, they are still useful on the browser, including Expires and Content-Disposition, etc.

Moreover, we have connected to several object storage services, such as Aliyun OSS, Tencent Cloud COS, and MinIO, we may extend to Amazon S3 and others. Therefore, we have abstracted the APIs of these services, however, this API behaves differently, hence the confusion above.

However, I don't think this is a big problem now. If we have the requirement, I have forked a copy and implemented the support of overwriting these response-* headers. Later, if we think these headers are not important anymore, we can remove the related logic and switch back to your official implementation.

Thank you for your reply and assistance!

@harshavardhana
Copy link
Member

Response changes for standard header types are provided via presigned URLs for GetObject this has been supported since day 1.

Use presignedGet API for what you want.

@klauspost
Copy link
Contributor

@reedchan7 Makes sense. I don't see any issue with implementing them. They can easily be added without breaking compatibility.

The easiest would probably be to just add them as supportedHeaders

Then (*GetObjectOptions).Set("Response-Content-Type", "text/plain") can be used to set it. You are welcome to send a PR that adds those fields.

@harshavardhana This is something else.

@reedchan7
Copy link
Contributor Author

@klauspost I did this by adding a reqParams to GetObjectOptions and then adding some methods, similar to the presignedGet API. Is this okay? Support passing in a URL query string in GetObjectOptions

@harshavardhana
Copy link
Member

@klauspost it is actually same functionality

@harshavardhana
Copy link
Member

@klauspost I did this by adding a reqParams to GetObjectOptions and then adding some methods, similar to the presignedGet API. Is this okay? Support passing in a URL query string in GetObjectOptions

@reedchan7 that is the correct way to do it.

@reedchan7
Copy link
Contributor Author

@klauspost @harshavardhana Do you need a PR?

@klauspost
Copy link
Contributor

@reedchan7 Yes, though un-export ToQueryValues and remove the pointer change on func (o *GetObjectOptions) Header() - that is a potentially breaking change.

reedchan7 added a commit to reedchan7/minio-go-sdk that referenced this issue Jan 16, 2023
This allows you to pass in the response headers
that you need to override when calling the GetObject method.
@reedchan7
Copy link
Contributor Author

@klauspost @harshavardhana Here is the PR: #1756

I set ToQueryValues to private, and rollback func (o *GetObjectOptions) Header() to func (o GetObjectOptions) Header() (Although this would warn: "Struct Client has methods on both value and pointer receivers. Such usage is not recommended by the Go Documentation.").

I add a supportedQueryValues just like your supportedHeaders.
When SetReqParam or AddReqParam method is invoked, the isStandardQueryValue function would be called to check if the key passed in by the user is supported.

And then I wrote an example.

Please review, thanks.

reedchan7 added a commit to reedchan7/minio-go-sdk that referenced this issue Jan 16, 2023
This allows you to pass in the response headers
that you need to override when calling the GetObject method.
reedchan7 added a commit to reedchan7/minio-go-sdk that referenced this issue Jan 16, 2023
This allows you to pass in the response headers
that you need to override when calling the GetObject method.
@reedchan7 reedchan7 changed the title Why does MinIO not support overwriting the Response Header value when calling GetObject? Why does MinIO not support overriding the Response Header value when calling GetObject? Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants