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

blob: Add support for key/value Metadata #496

Merged
merged 2 commits into from Oct 2, 2018
Merged

Conversation

vangent
Copy link
Contributor

@vangent vangent commented Sep 28, 2018

Fixes #324.

The most controversial thing in this PR is force-lowercasing the Metadata keys during reading and writing. Provider behavior is inconsistent on this. I think both S3 and GCS store keys case-sensitive, but GCS claims it is case-insensitive

  • GCP claims to be case-insensitive (search for "Field names are case-insensitive" here). However, the backend is actually case-sensitive. The JSON and XML-based APIs handle casing, but the HTTP one does not. Further discussion here.

  • S3 claims to store them in lowercase (search for "user-defined metadata keys in lowercase" here). However, the HTTP API actually returns them capitalized due to Go's header canonicalization; see this long-standing issue complaining about it. It suggests parsing the headers yourself, but the original case can't be recovered that way so I'm not sure how that helps.

So, I think the best way to make the behavior consistent across providers and simple for users is to force-lowercase the metadata keys during reading and writing. There might still be unexpected behavior if:

  1. The provider stores keys case-sensitive
  2. The user is writing metadata using some mechanism other than Go Cloud
    In this scenario, multiple case-sensitive keys will be collapsed into a single key when reading using Go Cloud. Note that this would happen anyway for S3, at least when using the HTTP client.

@googlebot googlebot added the cla: yes Google CLA has been signed! label Sep 28, 2018
@zombiezen
Copy link
Contributor

Another reasonable restriction might be to keep keys as ASCII-clean for now.

Copy link
Contributor

@zombiezen zombiezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a pass, mostly trying to poke around for edge cases about this, since it's tricky. I like this approach of just forcing everything to lowercase. I do suspect it will break things in surprising ways, but I suspect it to happen loudly.

blob/fileblob/fileblob.go Show resolved Hide resolved
blob/drivertest/drivertest.go Show resolved Hide resolved
blob/driver/driver.go Show resolved Hide resolved
blob/gcsblob/gcsblob.go Show resolved Hide resolved
blob/s3blob/s3blob.go Show resolved Hide resolved
blob/driver/driver.go Outdated Show resolved Hide resolved
@vangent
Copy link
Contributor Author

vangent commented Oct 2, 2018

PTAL.

Copy link
Contributor

@zombiezen zombiezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please let @shantuo take a look before merging.

@zombiezen zombiezen removed their assignment Oct 2, 2018
Copy link
Contributor

@shantuo shantuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, one question about driver comment.

blob/driver/driver.go Show resolved Hide resolved
@shantuo shantuo assigned vangent and unassigned shantuo Oct 2, 2018
@vangent vangent merged commit 5bd9aa6 into google:master Oct 2, 2018
@vangent vangent deleted the bloblabels branch October 2, 2018 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants