fix(datastore): remove namespace from Key.String() (#10684)#10685
fix(datastore): remove namespace from Key.String() (#10684)#10685sgp wants to merge 4 commits intogoogleapis:mainfrom
Conversation
Datastore namespaces may be sensitive, and it's best not to emit them. Provide a `datastore.Key.StringWithNamespace()` as an alternative, and use this version internally, which preserves the fix from googleapis#7829.
|
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). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Please create a bug describing the issue. |
datastore/integration_test.go
Outdated
| databasesStr, ok := os.LookupEnv(envDatabases) | ||
| if ok { | ||
| databaseIDs = append(databaseIDs, strings.Split(databasesStr, ",")...) | ||
| databaseIDs = strings.Split(databasesStr, ",") |
There was a problem hiding this comment.
Removing the append removes the default database from the list.
There was a problem hiding this comment.
Oops. This was not intended to be committed; I had done this because testing in the default database wasn't easy without polluting a ton of stuff in existing projects here. Nice catch.
datastore/key.go
Outdated
|
|
||
| // marshal marshals the key's string representation to the buffer. | ||
| func (k *Key) marshal(b *bytes.Buffer) { | ||
| func (k *Key) marshal(b *bytes.Buffer, includeNamespace bool) { |
There was a problem hiding this comment.
Rename includeNamespace to includeSensitive.
datastore/key.go
Outdated
| } | ||
|
|
||
| // StringWithNamespace is identical to String(), but appends the namespace. | ||
| func (k *Key) StringWithNamespace() string { |
There was a problem hiding this comment.
Better name for StringWithNamespace.
May be StringifyInternal or StringInternal or something else
There was a problem hiding this comment.
If it's truly meant to be "internal", then should we just make it non-exported and call it stringInternal?
datastore/key.go
Outdated
| return k._string(true) | ||
| } | ||
|
|
||
| func (k *Key) _string(includeNamespace bool) string { |
There was a problem hiding this comment.
Lose the _
Using lowercase letters for unexported identifiers tends to be more readable and aligns with the overall Go coding style in this repository
There was a problem hiding this comment.
Yeah, I nearly didn't do this, but redefining string as a func identifier when it clashes with a builtin type felt bad, too. I guess since it takes a receiver it's fine to call it Key.string, though. Will change.
I did, didn't I? It's #10684. |
|
I've addressed your comments, @bhshkh -- let me know what you think. |
Thanks! LGTM |
|
This branch is out-of-date with the base branch. |
|
@bhshkh --
Sadly, I created this fork from inside our organization, and apparently according to Github:
I could close this PR, fork using my personal repository, and then recreate the PR there. That's a pain, and we would lose the conversation here. In the meantime, can you force Kokoro to run so this will merge? Everything else has passed. |
|
Ok, apparently this is a losing battle, as I cannot continually fight the progress of the monorepo changing every day. I'm going to close this PR and reopen it using a personal fork where I can give appropriate permissions. |
Datastore namespaces may be sensitive, and it's best not to emit them.
Provide a
datastore.Key.StringWithNamespace()as an alternative, and use this version internally, which preserves the fix from #7829.