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

all: return error from concrete type for non-UTF8 strings #1322

Merged
merged 8 commits into from
Feb 15, 2019

Conversation

vangent
Copy link
Contributor

@vangent vangent commented Feb 14, 2019

Applied to the same set of strings we're escaping:

  • blob key
  • blob metadata key & value
  • pubsub metdata key & value

Verified with conformance tests.

Fixes #1322.

@vangent vangent requested a review from jba February 14, 2019 17:46
@googlebot googlebot added the cla: yes Google CLA has been signed! label Feb 14, 2019
Copy link
Contributor

@jba jba left a comment

Choose a reason for hiding this comment

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

What about keys passed elsewhere (attributes, NewRangeReader, etc.)?

blob/blob.go Outdated Show resolved Hide resolved
pubsub/pubsub.go Outdated Show resolved Hide resolved
@vangent
Copy link
Contributor Author

vangent commented Feb 14, 2019

What about keys passed elsewhere (attributes, NewRangeReader, etc.)?

For attributes, I haven't added any escaping for those yet. I'm not that worried about it since I don't think there's any expectation that user-invented strings should work (e.g., the reasonable values are defined by some spec). If it becomes a problem we can add validation of those fields, but it's likely to be much more than just "is UTF8".

For NewRangeReader, I expect that they are going to get some kind of error anyway since there's no way the blob exists with a non-UTF8-string key. Do you think it's worth adding a check (and also in Delete, etc.) to short-circuit?

@jba
Copy link
Contributor

jba commented Feb 14, 2019

I expect that they are going to get some kind of error anyway since there's no way the blob exists with a non-UTF8-string key.

Is that true across all providers? None of them support binary key names?

@vangent
Copy link
Contributor Author

vangent commented Feb 14, 2019

I expect that they are going to get some kind of error anyway since there's no way the blob exists with a non-UTF8-string key.

Is that true across all providers? None of them support binary key names?

Pretty sure. Well, memblob probably does. It's probably safer to verify, so I've added that.

@vangent
Copy link
Contributor Author

vangent commented Feb 14, 2019

PTAL

@jba jba merged commit 5d2842a into google:master Feb 15, 2019
@vangent vangent deleted the utf8 branch February 15, 2019 22:09
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.

3 participants