Fix dynamodb.types.Binary non-ASCII handling #2492
Merged
+13
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| if isinstance(value, six.text_type): # Support only PY2 for backward compatibility. | ||
| value = value.encode('utf-8') | ||
| elif not isinstance(value, bytes): | ||
| if not isinstance(value, (bytes, six.text_type)): |
There was a problem hiding this comment.
I believe that unicode should still be encoded to UTF-8. The internal representation (value) of the Binary class should always be bytes.
There was a problem hiding this comment.
Sure, that makes sense. I'll fix and post an update, then.
|
Thanks for the pull request! Overall this looks great, but I have one concern above. I definitely agree that encoding to UTF-8 for display may not make sense for arbitrary binary data, but we always want the internal representation to by |
Python 2.6/7 strings are used for both text and non-ASCII byte strings. Since the boto.dynamodb.types.Binary type is used for representing binary (and likely non-ASCII) data, encoding and decoding the internal string representation as UTF-8 doesn't make sense. This commit fixes the Python 2 version of Binary.
|
This looks great, thanks! 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make dynamodb.types.Binary use a raw string as the internal representation, instead of trying to UTF-8 encode and decode binary data. Fixes #2491