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

Validate S3 metadata only contains ascii chars #861

Merged
merged 2 commits into from
Apr 4, 2016

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Mar 31, 2016

Otherwise any error message with non-ascii chars won't
generate properly.

The difference being that type('foo %s' % u'bar') -> unicode,
but type('foo {}'.format(u'bar')) -> str.
@jamesls
Copy link
Member Author

jamesls commented Mar 31, 2016

I fixed the failing test by defaulting to unicode literals in exceptions.py. That has the possibility for unintended side effects, but all the botocore/cli tests pass.

The difference being that type('foo %s' % u'bar') -> unicode, but type('foo {}'.format(u'bar')) -> str.

@JordonPhillips
Copy link
Contributor


"""
metadata = params.get('Metadata')
if not metadata or not isinstance(metadata, dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that is an interesting problem in that we cannot validate with the validator that the parameter passed by the user is correct prior to the handler. It would be interesting to expose a new event down the line passed the validation or a helper method to run a quick validation in the future if we run into more of these scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked through several before-parameter-buildevent handlers, and there's several that are susceptible to this problem. I'm thinking it's probably worth having this event in the near future.

@kyleknap
Copy link
Contributor

kyleknap commented Apr 4, 2016

Looks good. Just had a comment to leave out there for future reference. 🚢

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

Successfully merging this pull request may close these issues.

3 participants