-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adds optional location_prefix kwarg in to_legacy_urlsafe #4635
Adds optional location_prefix kwarg in to_legacy_urlsafe #4635
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I've signed the CLA |
a2beb53
to
af85667
Compare
CLAs look good, thanks! |
If this isn't acceptable as a change in the repo, anyone wanting to do the same should be able to use this snippet in their code: import base64
from google.cloud.datastore import _app_engine_key_pb2
from google.cloud.datastore.key import _to_legacy_path
def to_urlsafe(key, location_prefix=None):
"""Convert to a base64 encode urlsafe string for App Engine.
This is intended to work with the "legacy" representation of a
datastore "Key" used within Google App Engine (a so-called
"Reference"). The returned string can be used as the ``urlsafe``
argument to ``ndb.Key(urlsafe=...)``. The base64 encoded values
will have padding removed.
.. note::
The string returned by ``to_legacy_urlsafe`` is equivalent, but
not identical, to the string returned by ``ndb``. The location
prefix may need to be specified to obtain idendentical urlsafe
keys.
:type key: google.cloud.datastore.key.Key
:param key: The key that you wish to convert to urlsafe
:type location_prefix: str
:param location_prefix: The location prefix of ones project. Often
this value is 's~', but may be 'dev~', 'e~', or
other location prefixes currently unknown.
:rtype: bytes
:returns: A bytestring containing the key encoded as URL-safe base64.
"""
if location_prefix:
project_id = location_prefix + key._project
else:
project_id = key._project
reference = _app_engine_key_pb2.Reference(
app=project_id,
path=_to_legacy_path(key._path), # Avoid the copy.
name_space=key.namespace,
)
raw_bytes = reference.SerializeToString()
return base64.urlsafe_b64encode(raw_bytes) |
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.
Thanks for filing. I don't see this as controversial in any way, so should be easy to merge!
Do you mind adding a unit test for the newly added parameter? If not, let me know and I can lend a hand.
@@ -310,13 +310,25 @@ def to_legacy_urlsafe(self): | |||
.. note:: | |||
|
|||
The string returned by ``to_legacy_urlsafe`` is equivalent, but | |||
not identical, to the string returned by ``ndb``. | |||
not identical, to the string returned by ``ndb``. The location | |||
prefix may need to be specified to obtain idendentical urlsafe |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
keys. | ||
|
||
:type location_prefix: str | ||
:param location_prefix: The location prefix of ones project. Often |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
||
:type location_prefix: str | ||
:param location_prefix: The location prefix of ones project. Often | ||
this value is 's~', but may be 'dev~', 'e~', or |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
||
:rtype: bytes | ||
:returns: A bytestring containing the key encoded as URL-safe base64. | ||
""" | ||
if location_prefix: |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@egalpin Thanks for the update, I had some strange GitHub refresh state and never saw them. My bad. |
@@ -430,6 +435,15 @@ def test_from_legacy_urlsafe_needs_padding(self): | |||
self.assertIsNone(key.namespace) | |||
self.assertEqual(key.flat_path, self._URLSAFE_FLAT_PATH2) | |||
|
|||
def test_from_legacy_urlsafe_with_location_prefix(self): |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
c84c8d2
to
55808e6
Compare
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 all authors are ok 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 |
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.
LGTM
I need update author details on some commits (working with multiple GH accounts on this machine) |
@egalpin The CLA issue was because I pushed a commit to the PR and the CLA-bot gets confused by multi-author PRs. No need to do anything. Cheers! |
Fixes #3597
I encountered an issue with respect to urlsafe keys in a system that is using both
google-cloud-datastore
andndb
in different services. I was receiving incompatible urlsafe keys when using the output ofto_legacy_urlsafe
in things likendb.Key(urlsafe=<output_from_legacy_urlsafe>)
running under App Engine. I received the following error:I then tried the suggestion as per #3597 of adding the location prefix to the project under cloud-datastore like so:
datastore.Client(project='s~your-app-id')
Unfortunately, I was met with this error (where
your-app-id
is a real project, with active datastore db which I can access via the Google Cloud Console):I was able to produce compatible urlsafe keys by specifying the location prefix only when creating the urlsafe keys. This PR contains that small change.