Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
bpo-29729: uuid.UUID now accepts bytes-like object#3801
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
uuid.UUID doesn't require the 'bytes' argument to be an instance ofbytes: accept bytes-like types, bytearray and memoryview for example.
serhiy-storchaka left a comment
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.
Add a "versionchanged" directive in the module documentation.
Hmm,memoryview is not accepted asbytes_le. And the length check works correctly only with memoryview with byte format.
* Accept also bytes-like for bytes_le: convert memoryview to bytes to use slices. Check also the length after the conversion to integer to raise a TypeError even if the length is not 16.* Fix unit tests: pass bytes to 'bytes' and 'bytes_le' parameters* Add more tests on Unicode passed to bytes and bytes_le* Document UUID changes
vstinner commentedSep 28, 2017
Oh, I didn't look at bytes_le. I also modified bytes_le to accept bytes-like objects.
I took care of such issues in my updated PR. |
Lib/uuid.py Outdated
| raiseValueError('badly formed hexadecimal UUID string') | ||
| int=int_(hex,16) | ||
| ifbytes_leisnotNone: | ||
| ifnotisinstance(bytes_le, (bytes_,bytearray)): |
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.
Addint.
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.
I don't understand why bytes_le should accept an int? Do you have an example please?
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.
It should not! But accepts. Add a test forbytes_le=16. 😉
| @@ -0,0 +1,3 @@ | |||
| uuid.UUID now accepts bytes-like object. The constructor doesn't require the | |||
| 'bytes' argument to be an instance of bytes: accept bytes-like types, | |||
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.
... and 'bytes_le'
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.
Oops, fixed.
serhiy-storchaka commentedSep 28, 2017
A ValueError is raised if 'bytes_le' is a str of len != 16. To solve this duplicate |
vstinner commentedSep 28, 2017
I added an unit tests to ensure that aTypeError is raised if you pass a str of any length to bytes or bytes_le. |
vstinner commentedSep 28, 2017
serhiy-storchaka commentedSep 28, 2017
Ah, yes, a bytes constructor raises an error. But still there is a problem with memoryview with non-byte format. |
vstinner commentedSep 28, 2017
PR#3796 has been merged. I merged master into my branch.
I don't think that we should overengineer the code here. We are consenting adults here. You can also find a value to skip input validations. It's really hard to write the perfect validation. If you can a random memoryview, yeah, maybe we get a valid UUID object whereas you shouldn't. Well, I don't think that we should detect this corner case. Why would you do that on purpose? |
vadmium left a comment
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.
It may not be worth supporting every kind of invalid input (unless you have to handle untrusted input). But it is important to implement what the documentation promises (or fix the documentation). As of reva169e89,bytes has to also supportlen.
| # Don't cast int to bytes to get a TypeError above | ||
| ifnotisinstance(bytes_le, (bytes_,bytearray,int_)): | ||
| bytes_le=bytes_(bytes_le) | ||
| iflen(bytes_le)!=16: |
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.
Byabove in the comment, do you mean this check, which is actually below the comment? :)
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.
I removed the comment.
vstinner commentedOct 2, 2017
@vadmium: "It may not be worth supporting every kind of invalid input (unless you have to handle untrusted input). But it is important to implement what the documentation promises (or fix the documentation). As of reva169e89, bytes has to also support len." I'm sorry, but I don't understand what you are expecting from me. What do you mean by "bytes has to also support len"? Do you expect a nicer error message if len() fails? |
vstinner commentedOct 2, 2017
@serhiy-storchaka,@vadmium: Would you mind to review the latest version of my change, please? |
vadmium commentedOct 3, 2017
For your code to work, the object passed as thebytes argument has to implement bytes=bytes_(bytes)# Convert bytes-like to bytes objectiflen(bytes)!=16 . . . Or you could usememoryview: withmemoryview(bytes)asview:ifview.nbytes!=16 . . . Or you could change the documentation: bytes_le now accepts any bytes-like object, andbytes now accepts any bytes-like object for whichlen returns 16. |
vstinner commentedOct 3, 2017
UUID constructor documentation is explicit: I guess that your remarks was on my "versionchanged" markup and the NEWS entry. I added "of 16 bytes" there. |
| int=int_.from_bytes(bytes,byteorder='big') | ||
| # test length after the conversion to raise a TypeError exception | ||
| # if 'bytes' type is str even if 'bytes' length is not 16 | ||
| iflen(bytes)!=16: |
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.
>>> a = array.array('i', [-1]*4)>>> x = int.from_bytes(a, byteorder='big')>>> len(a)4>>> x.bit_length()/816.0And conversely:
>>> a = array.array('i', [-1]*16)>>> x = int.from_bytes(a, byteorder='big')>>> len(a)16>>> x.bit_length()/864.0vadmium commentedOct 4, 2017
Yes I was talking about theversionchanged note, which now says "any bytes-like objects of 16 bytes". But I don't think this is clear enough. Antoine and Serhiy already gave examples of bytes-like objects containing exactly 16 bytes wherelen returns a different number. Since you already copybytes_le into a true bytes object, why not parallel this for thebytes parameter? ifbytesisnotNone:# Check for int so that bytes=16 triggers an exception belowifnotisinstance(bytes, (bytes_,bytearray,int_)):bytes=bytes_(bytes)# Copy any bytes-like objectiflen(bytes)!=16:raiseValueError('bytes is not a 16-byte string')int=int_.from_bytes(bytes,'big') |
vstinner commentedOct 5, 2017
I decided to work onhttps://bugs.python.org/issue29729 because I understand that the proposed change was just to remove an assertion. That's all. It seems like you expect much more changes, much more complex code. It's already my 6th version of my pull request. Honestly, I don't care much of the uuid module. I don't think that anyone ever tried to call UUID(bytes=value) with a type different than bytes (ok, apart the bug reporter ;-)). If I would really need to pass non-bytes, having to cast manually with bytes(value) wouldn't burn my hands. For all these reasons, I abandon my change. Feel free to reuse my code if someone wants to pursue the effort on UUID enhancement ;-) |
serhiy-storchaka commentedOct 5, 2017
The problem with adding support of bytes-like objects is that in different cases this term has different meaning. The definition in the glossary is too wide, in many particular cases we need the support of not just the buffer protocol, but len(), indexing, slicing, iterating, startswith() or even lower(). I think we need different terms for different levels of "bytesness". |
vstinner commentedOct 5, 2017
I think that we should replace the assertion with a regular if and raise a proper TypeError, rather than trying to support "bytes-like" objects. It's fine to require bytes on UUID. UUID is not a common type, and passing bytes to UUID is even less common (IMHO). |
Uh oh!
There was an error while loading.Please reload this page.
uuid.UUID doesn't require the 'bytes' argument to be an instance of
bytes: accept bytes-like types, bytearray and memoryview for example.
https://bugs.python.org/issue29729