datastore: add EnableKeyConversion for compatibility with Cloud Datastore encoded keys #192

goog-lukemc · 2019-02-12T20:37:10Z

The main purpose of this change is to remove toil in migrating to the go111 runtime. This PR adds forward compatibility with cloud.google.com/go/datastore/key.Encode() so that customers can address migration to the go111 runtime in any order without concern for key encoding strings difference produced by the key.Encode() functions.

datastore/key.go

datastore/keycompatibility.go

Deleplace · 2019-02-18T17:25:50Z

CI fails for Go 1.8, as the new dependencies use type aliasing (and other things) added in 1.9.

This looks like another point in favor of more specific code duplication (though I ack the risk of future divergence).

datastore/keycompatibility.go

…tch.

googlebot · 2019-05-09T22:03:30Z

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 or co-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 cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-09T22:03:31Z

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 or co-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 cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

tbpg

Looks great overall!

tbpg · 2019-05-10T15:22:03Z

datastore/internal/cloudkey/cloudkey.go

+func protoToKey(p *cloudpb.Key) (*Key, error) {
+	var key *Key
+	var namespace string
+	if partition := p.PartitionId; partition != nil {


Could this be simplified to namespace := p.GetPartitionId().GetNamespaceId()? Normally, the Get function for protos handles nil values nicely.

Everything under internal/cloudkey and internal/cloudpb is copied/pasted from cloud.google.com/go/datastore, we should keep as-is.

Can you make that more clear? I wasn't sure where the copy/paste started & stopped.

Hopefully better now. Added package-level doc, too.

tbpg · 2019-05-10T15:22:31Z

datastore/internal/cloudpb/entity.pb.go

+	"github.com/golang/protobuf/proto"
+)
+
+// Code below is copied from google.golang.org/genproto/googleapis/datastore/v1


Maybe comment on why it was copied?

PTAL at package-level doc.

tbpg · 2019-05-10T15:23:24Z

datastore/key.go

@@ -254,6 +254,9 @@ func DecodeKey(encoded string) (*Key, error) {

 	ref := new(pb.Reference)
 	if err := proto.Unmarshal(b, ref); err != nil {
+		if k := decodeCloudKey(encoded); k != nil {


Might be worth a comment explaining why this is done this way.

tbpg · 2019-05-10T15:24:37Z

datastore/keycompat.go

+	keyConversion.mu.RLock()
+	project := keyConversion.project
+	keyConversion.mu.RUnlock()
+	return project


Seems weird to R lock the mutex and return the value... Sounds like it would be easy to have a race condition when the value is used.

Once set, it's immutable and will always return the same value. Will add a comment.

tbpg · 2019-05-10T15:25:10Z

datastore/keycompat.go

+	return project
+}
+
+// decodeCloudKey attempts to decode the given encoded key generated by the Cloud Datastore client library, returning nil if the key couldn't be decoded.


Reference the full package name rather than "Cloud Datastore client library"?

tbpg · 2019-05-10T15:26:09Z

datastore/keycompat.go

+}
+
+// decodeCloudKey attempts to decode the given encoded key generated by the Cloud Datastore client library, returning nil if the key couldn't be decoded.
+func decodeCloudKey(encoded string) *Key {


This should probably return an error. Good not to rely on special return values to indicate success/failure (other than an error).

This was a simplification - we don't use the error value. I think the contract of "non-nil if valid" is fine?

SGTM. Similar to how protos work in general, I suppose.

datastore/keycompat.go

tbpg · 2019-05-10T15:29:57Z

datastore/keycompat_test.go

+	"testing"
+)
+
+var testCasesKeyCompat = []struct {


Not a big deal, but I usually prefer putting the test cases directly in the test where they're used. Then the slice can just be named tests because it's clear what they're for.

tbpg · 2019-05-10T15:31:15Z

datastore/keycompat_test.go

+			t.Fatalf("%v", err.Error())
+		}
+		if !reflect.DeepEqual(dk, tc.key) {
+			t.Errorf("%s: got %+v, want %+v", tc.desc, dk, tc.key)


Please identify the function that failed. https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures

Looks like tc.desc is logged. What would make it clearer?

I prefer both (function name + desc), but OK with me to only do tc.desc.

sbuss · 2019-05-10T20:37:53Z

datastore/internal/cloudkey/cloudkey.go

+func protoToKey(p *cloudpb.Key) (*Key, error) {
+	var key *Key
+	var namespace string
+	if partition := p.PartitionId; partition != nil {


Can you make that more clear? I wasn't sure where the copy/paste started & stopped.

sbuss · 2019-05-10T20:50:53Z

datastore/keycompat.go

+// The context provided must be an App Engine context if running in first generation App Engine.
+// This can be called in the /_ah/start handler. It is safe to call multiple times, and is cheap to call, so can also be inserted as middleware.
+//
+// Enabling key compatibility not affect the encoding format used by Key.Encode, it only expands the type of keys that are able to be decoded with DecodeKey.


nit: grammar. Should be "does not affect"?

sbuss · 2019-05-13T21:40:29Z

datastore/keycompat.go

+//
+// Enabling key compatibility not affect the encoding format used by Key.Encode, it only expands the type of keys that are able to be decoded with DecodeKey.
+func EnableKeyConversion(ctx context.Context) {
+	if getKeyConversionAppID() == "" {


Am I misreading this? Won't getKeyConversionAppID() always return "" until the code below this runs? And so this effectively prevents keyConversion.project from ever getting set?

Oops. Expression was inverted. PTAL. Good catch.

datastore/keycompat.go

googlebot · 2019-05-14T17:20:29Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-14T17:20:31Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

If a key can't be decoded, try to decode it with the encoding scheme from google.golang.org/appengine/datastore. Along with golang/appengine#192, this allows for cross-compatibility for encoded keys across the two libraries. Co-authored-by: Luke Mccoy <[email protected]> Co-authored-by: Chris Broadfoot <[email protected]> Change-Id: I7e091b37b184a12c30998c3c22baffb964018a61 Reviewed-on: https://code-review.googlesource.com/c/gocloud/+/41370 Reviewed-by: kokoro <[email protected]> Reviewed-by: Tyler Bui-Palsulich <[email protected]> Reviewed-by: Jean de Klerk <[email protected]> Reviewed-by: Steven Buss <[email protected]>

goog-lukemc added 8 commits January 30, 2019 20:14

Adding foward compatibility for key encoding

bce4541

splitting logic into a new file

8d736f0

Added implemetnation check

71b3369

split code into seperate files and completed code review comments

1a7bf35

Add additional return to fix nil pointer exception

74ebe0f

added key parent testing and as well as control tests

3b5c8ef

Fix pointer preference per review comments

5721432

Change variable name per comment

ee4f33d

goog-lukemc changed the title ~~Adds forward compatibility for with cloud.google.com/go/datastore/key.Encode() strings~~ Adds forward compatibility with cloud.google.com/go/datastore/key.Encode() strings Feb 12, 2019

sbuss suggested changes Feb 12, 2019

View reviewed changes

goog-lukemc added 3 commits February 15, 2019 12:46

Added documentation for datastore key conversation

8e1c38a

fixed various comment nits

5d06d01

fixed various comment nits and typos

f4a9440

sbuss requested a review from broady February 15, 2019 17:53

broady reviewed Feb 20, 2019

View reviewed changes

datastore/keycompatibility.go Outdated Show resolved Hide resolved

broady reviewed Feb 20, 2019

View reviewed changes

datastore/keycompatibility.go Outdated Show resolved Hide resolved

goog-lukemc added 13 commits February 25, 2019 16:19

Updated variable name to make it more readable

7789107

Removed \n and spacing issue

7d7c327

Update lib alias name to newnds so that the comments and the alias ma…

ea111b8

…tch.

Update lib alias name to newnds so that the comments and the alias ma…

300b945

…tch.

Fixed various greammer issues and string spacing

dc956f7

passing unit tests

35a8464

removed dependancies on new libs

68002c3

code duplication file - code copied from cloud.google.com/go/datastore

0030609

Spacing and format changes

a66c279

adjusted formating

cf0b497

added sync.Once

adf4e0f

Updated Key conversion documentation in readme.md

0fbebe6

corrected spelling

abdd3c6

googlebot added cla: no and removed cla: yes labels May 9, 2019

broady changed the title ~~Adds forward compatibility with cloud.google.com/go/datastore/key.Encode() strings~~ datastore: add EnableKeyConversion for compatibility with Cloud Datastore encoded keys May 9, 2019

broady added the cla: yes label May 9, 2019

googlebot removed the cla: no label May 9, 2019

README

2c45d7e

googlebot added cla: no and removed cla: yes labels May 9, 2019

golang deleted a comment from googlebot May 9, 2019

keycompat -> cloudkey

d3c996c

tbpg suggested changes May 10, 2019

View reviewed changes

sbuss reviewed May 13, 2019

View reviewed changes

review comments

98afd7e

broady approved these changes May 13, 2019

View reviewed changes

tbpg approved these changes May 14, 2019

View reviewed changes

sbuss approved these changes May 14, 2019

View reviewed changes

broady added the cla: yes label May 14, 2019

googlebot removed the cla: no label May 14, 2019

broady merged commit 4c25cac into golang:master May 14, 2019

sg0hsmt mentioned this pull request May 11, 2024

update proto library to google.golang.org/protobuf #327

Open

datastore: add EnableKeyConversion for compatibility with Cloud Datastore encoded keys #192

datastore: add EnableKeyConversion for compatibility with Cloud Datastore encoded keys #192

Uh oh!

Conversation

goog-lukemc commented Feb 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Deleplace commented Feb 18, 2019

Uh oh!

Uh oh!

Uh oh!

googlebot commented May 9, 2019

Uh oh!

googlebot commented May 9, 2019

Uh oh!

tbpg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!