Strip base64 padding characters in Datastore's (to|from)_legacy_urlsafe #3560
Conversation
|
Thanks for using your personal account. Did you have a chance to see my comments on #3559? |
|
Hi @dhermes , thanks for your helpful comments! I made another commit to my master to follow your comments. Let me know if it's already good to go or not! |
| @@ -324,7 +325,8 @@ def from_legacy_urlsafe(cls, urlsafe): | |||
| This is intended to work with the "legacy" representation of a | |||
| datastore "Key" used within Google App Engine (a so-called | |||
| "Reference"). This assumes that ``urlsafe`` was created within an App | |||
| Engine app via something like ``ndb.Key(...).urlsafe()``. | |||
| Engine app via something like ``ndb.Key(...).urlsafe()``. The base64 | |||
| encoded values will have padding removed. | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -333,6 +335,9 @@ def from_legacy_urlsafe(cls, urlsafe): | |||
| :rtype: :class:`~google.cloud.datastore.key.Key`. | |||
| :returns: The key corresponding to ``urlsafe``. | |||
| """ | |||
| padding = b'=' * (-len(urlsafe) % 4) | |||
| urlsafe += padding | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Since both stripped and non-stripped padding should be acceptable.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@michaelenglo I've added some new tests to actually make sure the padding gets added/stripped. This LGTM and I'll merge one CircleCI is green. Thanks a lot! |
|
Thanks a lot @dhermes ! I appreciate your responsiveness and helpfulness since day one. Hope to get more chance to learn and contribute to the library! |
…_legacy_urlsafe (#3560) Also * add padding characters in `from_legacy_urlsafe` if needed * add an extra example in the unit tests that actually requires base64 padding
…_legacy_urlsafe (googleapis#3560) Also * add padding characters in `from_legacy_urlsafe` if needed * add an extra example in the unit tests that actually requires base64 padding
…_legacy_urlsafe (googleapis#3560) Also * add padding characters in `from_legacy_urlsafe` if needed * add an extra example in the unit tests that actually requires base64 padding
…_legacy_urlsafe (googleapis#3560) Also * add padding characters in `from_legacy_urlsafe` if needed * add an extra example in the unit tests that actually requires base64 padding
Hi,
I am proposing a change to the Google Cloud Platform Datastore's Python client library, specifically the Key module. @dhermes and I have discussed about removing the padding characters ("=") from regular b64encoded urlsafe to match the version that is used in Datastore (which does not have the padding character "="). This is done to ensure consistency in using and reproducing Datastore's convention for urlsafe key.
Please let me know if there is any other steps I may not have included and are needed to do to improve the changes.