Make rawdatastore API safer for writing filters.

Make rawdatastore API safer for writing filters.

This removes Get/Put/Delete, since they always end up being implemented
as wrappers around the Multi versions anyway, it just caused unnecessary noise.
They'll come back with the 'user friendly' datastore interface, but there's no
reason to have them at every layer of the cake.

It moves the *Multi methods to be callback-based, which makes interacting with
them easier (since you don't have to do lots of slice allocations).

It introduces 'checkFilter' to rawdatastore which guarantees that filters and
backend implementations don't need to do lots of redundant checks to sanitize
their inputs.

It adds a new API to the Info service to retrieve the current namespace.

R=dnj@google.com, estaab@chromium.org, dnj@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/gae/commit/5de5392ae282348d6c7f4df74fea36bfccd48e09

[iannucci, 2015-07-27 15:21:34]

On 2015/07/26 23:07:20, iannucci wrote:

wait on this one for a bit, have a refactor.

[iannucci, 2015-07-28 05:26:41]

PTAL, this is in decent shape now (though the diff is a bit dumb :/)

[iannucci, 2015-07-28 05:27:02]

On 2015/07/28 05:26:41, iannucci wrote:
> PTAL, this is in decent shape now (though the diff is a bit dumb :/)

(check the description for what to look for)

[estaab, 2015-07-28 21:52:40]

lgtm with the caveat that I don't have a strong grasp of this code.

https://codereview.chromium.org/1253263002/diff/40001/filter/count/count_test.go
File filter/count/count_test.go (right):

https://codereview.chromium.org/1253263002/diff/40001/filter/count/count_test...
filter/count/count_test.go:24: pnil := func(_ rawdatastore.Key, err error) {
Can this have a more descriptive name like putAssertNoError or something along
those lines?

https://codereview.chromium.org/1253263002/diff/40001/filter/featureBreaker/f...
File filter/featureBreaker/featurebreaker_test.go (right):

https://codereview.chromium.org/1253263002/diff/40001/filter/featureBreaker/f...
filter/featureBreaker/featurebreaker_test.go:22: cbe := func(expect string)
func(rawdatastore.PropertyMap, error) {
better name here, maybe errorSubstrMatch?

[dnj (Google), 2015-07-28 23:11:12]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-07-28 23:11:13]

(Sorry for the delay, will review later this evening).

[dnj (Google), 2015-07-29 01:44:59]

LGTM w/ nits.

Now that i see the usage more directly, I think it would be useful to have
Interface methods return an error if any of the callback methods returned an
error, maybe even a MultiError, instead of nil. This would make large-scale "did
something go wrong?" checks really easy to implement.

This is somewhat redundant with the ability to retain an error via the callback,
but from a user interface perspective might present the best of both worlds.
WDYT?

https://codereview.chromium.org/1253263002/diff/40001/impl/memory/raw_datasto...
File impl/memory/raw_datastore.go (right):

https://codereview.chromium.org/1253263002/diff/40001/impl/memory/raw_datasto...
impl/memory/raw_datastore.go:71: /*
Why disabled / commented out?

https://codereview.chromium.org/1253263002/diff/40001/impl/prod/raw_datastore.go
File impl/prod/raw_datastore.go (right):

https://codereview.chromium.org/1253263002/diff/40001/impl/prod/raw_datastore...
impl/prod/raw_datastore.go:84: } else {
Remove "else" clause, return nil at the end of the initial "if".

https://codereview.chromium.org/1253263002/diff/40001/impl/prod/raw_datastore...
impl/prod/raw_datastore.go:150: keepGoing = cb(dsR2F(k), tf.pm, cfunc)
Consider just:
for {
  ... 
  if !cb(...) {
    return nil
  }

[iannucci, 2015-07-29 07:35:40]

On 2015/07/29 01:44:59, dnj (Google) wrote:
> LGTM w/ nits.
> 
> Now that i see the usage more directly, I think it would be useful to have
> Interface methods return an error if any of the callback methods returned an
> error, maybe even a MultiError, instead of nil. This would make large-scale
"did
> something go wrong?" checks really easy to implement.
> 
> This is somewhat redundant with the ability to retain an error via the
callback,
> but from a user interface perspective might present the best of both worlds.
> WDYT?

I think you'll be really happy with the next CL :)

> 
>
https://codereview.chromium.org/1253263002/diff/40001/impl/memory/raw_datasto...
> File impl/memory/raw_datastore.go (right):
> 
>
https://codereview.chromium.org/1253263002/diff/40001/impl/memory/raw_datasto...
> impl/memory/raw_datastore.go:71: /*
> Why disabled / commented out?
> 
>
https://codereview.chromium.org/1253263002/diff/40001/impl/prod/raw_datastore.go
> File impl/prod/raw_datastore.go (right):
> 
>
https://codereview.chromium.org/1253263002/diff/40001/impl/prod/raw_datastore...
> impl/prod/raw_datastore.go:84: } else {
> Remove "else" clause, return nil at the end of the initial "if".
> 
>
https://codereview.chromium.org/1253263002/diff/40001/impl/prod/raw_datastore...
> impl/prod/raw_datastore.go:150: keepGoing = cb(dsR2F(k), tf.pm, cfunc)
> Consider just:
> for {
>   ... 
>   if !cb(...) {
>     return nil
>   }

[iannucci, 2015-07-29 07:35:48]

https://chromiumcodereview.appspot.com/1253263002/diff/40001/filter/count/cou...
File filter/count/count_test.go (right):

https://chromiumcodereview.appspot.com/1253263002/diff/40001/filter/count/cou...
filter/count/count_test.go:24: pnil := func(_ rawdatastore.Key, err error) {
On 2015/07/28 21:52:40, estaab wrote:
> Can this have a more descriptive name like putAssertNoError or something along
> those lines?

yeah... I actually take all these out completely in the next CL :)

https://chromiumcodereview.appspot.com/1253263002/diff/40001/filter/featureBr...
File filter/featureBreaker/featurebreaker_test.go (right):

https://chromiumcodereview.appspot.com/1253263002/diff/40001/filter/featureBr...
filter/featureBreaker/featurebreaker_test.go:22: cbe := func(expect string)
func(rawdatastore.PropertyMap, error) {
On 2015/07/28 21:52:40, estaab wrote:
> better name here, maybe errorSubstrMatch?

nuked in the next cl

https://chromiumcodereview.appspot.com/1253263002/diff/40001/impl/memory/raw_...
File impl/memory/raw_datastore.go (right):

https://chromiumcodereview.appspot.com/1253263002/diff/40001/impl/memory/raw_...
impl/memory/raw_datastore.go:71: /*
On 2015/07/29 01:44:59, dnj (Google) wrote:
> Why disabled / commented out?

all this query stuff is placeholder right now.

https://chromiumcodereview.appspot.com/1253263002/diff/40001/impl/prod/raw_da...
File impl/prod/raw_datastore.go (right):

https://chromiumcodereview.appspot.com/1253263002/diff/40001/impl/prod/raw_da...
impl/prod/raw_datastore.go:84: } else {
On 2015/07/29 01:44:59, dnj (Google) wrote:
> Remove "else" clause, return nil at the end of the initial "if".

Done.

https://chromiumcodereview.appspot.com/1253263002/diff/40001/impl/prod/raw_da...
impl/prod/raw_datastore.go:150: keepGoing = cb(dsR2F(k), tf.pm, cfunc)
On 2015/07/29 01:44:59, dnj (Google) wrote:
> Consider just:
> for {
>   ... 
>   if !cb(...) {
>     return nil
>   }

Done.

[iannucci, 2015-07-29 07:36:21]

Committed patchset #4 (id:60001) manually as
5de5392ae282348d6c7f4df74fea36bfccd48e09 (presubmit successful).