-
Notifications
You must be signed in to change notification settings - Fork 346
Add support for additional metadata headers. #486
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a brief look at this. At line 585, don't you mean "if not content_type"? And since there is already a default in the arg list, the only way this could happen is if fput is called with an explicit content_type=None or null string, which seems a little strange.
Oh yes good catch @hashbackup |
95127e9
to
3ba598f
Compare
Can you validate @hashbackup this new API change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me, without running it. I'm a dumbass and can't figure out how to get a copy of the changes, sorry. Duh.
The keyword issue (case sensitive) is still a problem I think. It's not a big deal for minio devs to follow a case convention, but will likely trip up others when they start adding their own keywords / metadata. See the change I made to signer.py
Another way would be a case-insensitive dict. Not sure you want to go to the trouble, but I believe it is the most correct solution since http headers actually are case-insensitive.
HTTP headers are case insensitive so it doesn't matter what they are really. So you don't have to worry about it on client. |
True, it doesn't matter. But if you do this:
and pass that as metadata, the minio signing procedure fails because it does a header.title() thing. That fails for the lowercase header. When I initially implemented headers, I ran into this because I used 'x-aws-storage-class' as the header name. Try it. Update: this is a bad test case that actually works. Just use the lowercase header and it fails. |
Ah i see what you mean let me see how to avoid a scenario. |
3ba598f
to
096d875
Compare
Fixed it
Works fine either way, picks one of the duplicated elements using a case insensitive dictionary helped. |
096d875
to
6cb8ae2
Compare
This is implemented since we have already implemented PutObjectWithMetadata() in minio-go. So following the logic here. This adds support to be able to specify `x-amz-storage-class` etc. Fixes minio#482
6cb8ae2
to
1d9e9c1
Compare
This is implemented since we have already implemented
PutObjectWithMetadata() in minio-go. So following the
logic here.
This adds support to be able to specify
x-amz-storage-class
etc.Fixes #482