impl/memory: Disallow empty namespace.

impl/memory: datastore and memcache namespacing

Update GetNamespace() to return an additional boolean, which will be
true if the namespace was set and false if it was not. The AppEngine SDK
defaults to having no namespace installed ("", false), but will allow
the user to install any namespace. This allows a user to differentiate
between no namespace and an empty namespace ("", true).

Make impl/memory's datastore query operation to error if the namespace has
been set to the empty namespace (""). This reflects observed production
behavior.

BUG=

Committed: https://github.com/luci/gae/commit/76e946629e8d7f5e68963ff48fbaabe5ac2c2157

[dnj, 2016-04-22 19:02:22]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2016-04-22 19:02:23]

PTAL

[iannucci, 2016-04-22 19:11:36]

Description was changed from

==========
impl/memory: Disallow empty namespace.

Disallow setting the empty namespace. In production, the empty namespace
is invalid, and will result in a server error if used.

This could be better handled if luci/gae #23 is implemented.

BUG=
==========

to

==========
impl/memory: Disallow empty namespace.

Disallow setting the empty namespace. In production, the empty namespace
is invalid, and will result in a server error if used.

This could be better handled if luci/gae#23 is implemented.

BUG=
==========

[iannucci, 2016-04-22 19:13:49]

lgtm (and I edited the description to use the correct github issue syntax)

https://chromiumcodereview.appspot.com/1916463004/diff/1/impl/memory/datastor...
File impl/memory/datastore_query_execution_test.go (right):

https://chromiumcodereview.appspot.com/1916463004/diff/1/impl/memory/datastor...
impl/memory/datastore_query_execution_test.go:88: pmap("$key",
ds.MakeKey("dev~app", "jim", "Kind", "id")),
s/jim/kat

https://chromiumcodereview.appspot.com/1916463004/diff/1/impl/memory/datastor...
impl/memory/datastore_query_execution_test.go:354: pmap("$key",
ds.MakeKey("dev~app", "", "__namespace__", "bob")),
lost test for default namespace :/. Could we add a quick ad-hoc one?

[Vadim Sh., 2016-04-22 19:31:22]

vadimsh@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2016-04-22 19:31:23]

wait, what? 

Code in luci-go does switches to empty namespace and it has worked fine... e.g.
https://github.com/luci/luci-go/blob/master/appengine/gaesettings/gaesettings...

[Vadim Sh., 2016-04-22 19:31:31]

vadimsh@chromium.org changed reviewers:
- vadimsh@chromium.org

[Vadim Sh., 2016-04-22 19:36:18]

On 2016/04/22 19:31:23, Vadim Sh. wrote:
> wait, what? 
> 
> Code in luci-go does switches to empty namespace and it has worked fine...
e.g.
>
https://github.com/luci/luci-go/blob/master/appengine/gaesettings/gaesettings...

If it is indeed broken, we'd need to BS workarounds for getting non-namespaces
context, since many libs in luci-go/appengine need single namespace (since they
store data global to the service, such as configs, users groups, etc).

[dnj, 2016-04-22 19:44:39]

On 2016/04/22 19:36:18, Vadim Sh. wrote:
> On 2016/04/22 19:31:23, Vadim Sh. wrote:
> > wait, what? 
> > 
> > Code in luci-go does switches to empty namespace and it has worked fine...
> e.g.
> >
>
https://github.com/luci/luci-go/blob/master/appengine/gaesettings/gaesettings...
> 
> If it is indeed broken, we'd need to BS workarounds for getting non-namespaces
> context, since many libs in luci-go/appengine need single namespace (since
they
> store data global to the service, such as configs, users groups, etc).

The error that prompted this occurred when:
c = info.Get(c).MustNamespace("")
ds.Get(c).Run(query, ...)

Failure to query. :: {"error":"API error 1 (datastore_v3: BAD_REQUEST):
Namespace (name_space) field is invalid: name_space may not be present and
empty.", "clientID":"1461351604_9", "key":"tumble.9.lock", "namespace":"",
"query":"SELECT DISTINCT `ExpandedShard`, `TargetRoot` FROM `tumble.Mutation`
WHERE `ExpandedShard` >= -4035225266123964425 AND `ExpandedShard` <=
-3458764513820540938 ORDER BY `ExpandedShard`, `TargetRoot`, `__key__` LIMIT
128"}

Perhaps this only happens for queries, not Get/Put?

[dnj, 2016-04-22 23:27:25]

Description was changed from

==========
impl/memory: Disallow empty namespace.

Disallow setting the empty namespace. In production, the empty namespace
is invalid, and will result in a server error if used.

This could be better handled if luci/gae#23 is implemented.

BUG=
==========

to

==========
impl/memory: datastore and memcache namespacing

Update GetNamespace() to return an additional boolean, which will be
true if the namespace was set and false if it was not. The AppEngine SDK
defaults to having no namespace installed ("", false), but will allow
the user to install any namespace. This allows a user to differentiate
between no namespace and an empty namespace ("", true).

Make impl/memory's datastore query operation to error if the namespace has
been set to the empty namespace (""). This reflects observed production
behavior.

BUG=
==========

[dnj, 2016-04-22 23:28:00]

Updated to now allow empty namespaces basically everywhere except datastore
queries.

[iannucci, 2016-04-23 19:19:08]

lgtm

https://chromiumcodereview.appspot.com/1916463004/diff/20001/impl/memory/data...
File impl/memory/datastore.go (right):

https://chromiumcodereview.appspot.com/1916463004/diff/20001/impl/memory/data...
impl/memory/datastore.go:255: // for queries.
link to bug

[dnj, 2016-04-24 06:29:49]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-04-24 06:29:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1916463004/#ps60001
(title: "Rbase.")

[commit-bot: I haz the power, 2016-04-24 06:29:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916463004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916463004/60001

[commit-bot: I haz the power, 2016-04-24 06:34:39]

Description was changed from

==========
impl/memory: datastore and memcache namespacing

Update GetNamespace() to return an additional boolean, which will be
true if the namespace was set and false if it was not. The AppEngine SDK
defaults to having no namespace installed ("", false), but will allow
the user to install any namespace. This allows a user to differentiate
between no namespace and an empty namespace ("", true).

Make impl/memory's datastore query operation to error if the namespace has
been set to the empty namespace (""). This reflects observed production
behavior.

BUG=
==========

to

==========
impl/memory: datastore and memcache namespacing

Update GetNamespace() to return an additional boolean, which will be
true if the namespace was set and false if it was not. The AppEngine SDK
defaults to having no namespace installed ("", false), but will allow
the user to install any namespace. This allows a user to differentiate
between no namespace and an empty namespace ("", true).

Make impl/memory's datastore query operation to error if the namespace has
been set to the empty namespace (""). This reflects observed production
behavior.

BUG=

Committed:
https://github.com/luci/gae/commit/76e946629e8d7f5e68963ff48fbaabe5ac2c2157
==========

[commit-bot: I haz the power, 2016-04-24 06:34:40]

Committed patchset #4 (id:60001) as
https://github.com/luci/gae/commit/76e946629e8d7f5e68963ff48fbaabe5ac2c2157