Re-add metadata passthrough on Get operations

Re-add metadata passthrough on Get operations

It got lost in translation during the RawInterface conversion. This will be used
to e.g. allow SomeStruct to set a default dscache.enable metadatum when doing
e.g. Interface.GetMulti([]SomeStruct{...}).

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

Committed: https://github.com/luci/gae/commit/00d1a80545d14379fbe4b851ff638f7e9be85e84

[iannucci, 2015-08-04 18:18:22]

rebased, ptal

[iannucci, 2015-08-04 19:12:06]

actually hang on a sec, this interface is a bit kludgey. Fix coming shortly.

[iannucci, 2015-08-04 21:58:47]

Ok, PTAL again. This makes the metadata passthrough a bit more typeful and less
ambiguous (e.g. no "what should I do with this PropertyMap?!"). It also defines
a new GetMetaDefault method for PLS which is quite useful when all you want to
do is typecheck and provide a default value (which is 100% of the GetMeta
invocations I have so far)

[dnj, 2015-08-04 22:16:06]

lgtm

https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
File service/datastore/raw_interface.go (right):

https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
service/datastore/raw_interface.go:113: if idx >= len(m) {
What's the use case for tolerating anything other than perfect parity here?

If it's explicitly for empty construction, perhaps you want to more explicitly
say:
if len(m) == 0 {
  return nil, ErrMetaFieldUnset
}
if idx >= len(m) {
  panic("ArrayIndexOutOfBoundsException")
}

[iannucci, 2015-08-04 23:41:06]

On 2015/08/04 22:16:06, dnj wrote:
> lgtm
> 
>
https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
> File service/datastore/raw_interface.go (right):
> 
>
https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
> service/datastore/raw_interface.go:113: if idx >= len(m) {
> What's the use case for tolerating anything other than perfect parity here?
> 
> If it's explicitly for empty construction, perhaps you want to more explicitly
> say:
> if len(m) == 0 {
>   return nil, ErrMetaFieldUnset
> }
> if idx >= len(m) {
>   panic("ArrayIndexOutOfBoundsException")
> }

WDYT about

if m == nil {
  return nil, ErrMetaFieldUnset
}
return m[idx].GetMeta(key)  // will panic on its own

[iannucci, 2015-08-04 23:41:55]

On 2015/08/04 23:41:06, iannucci wrote:
> On 2015/08/04 22:16:06, dnj wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
> > File service/datastore/raw_interface.go (right):
> > 
> >
>
https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
> > service/datastore/raw_interface.go:113: if idx >= len(m) {
> > What's the use case for tolerating anything other than perfect parity here?
> > 
> > If it's explicitly for empty construction, perhaps you want to more
explicitly
> > say:
> > if len(m) == 0 {
> >   return nil, ErrMetaFieldUnset
> > }
> > if idx >= len(m) {
> >   panic("ArrayIndexOutOfBoundsException")
> > }
> 
> WDYT about
> 
> if m == nil {

er, I mean len(m) == 0

>   return nil, ErrMetaFieldUnset
> }
> return m[idx].GetMeta(key)  // will panic on its own

[iannucci, 2015-08-04 23:53:49]

On 2015/08/04 23:41:55, iannucci wrote:
> On 2015/08/04 23:41:06, iannucci wrote:
> > On 2015/08/04 22:16:06, dnj wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
> > > File service/datastore/raw_interface.go (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1270113002/diff/60001/service/datastore/raw_i...
> > > service/datastore/raw_interface.go:113: if idx >= len(m) {
> > > What's the use case for tolerating anything other than perfect parity
here?
> > > 
> > > If it's explicitly for empty construction, perhaps you want to more
> explicitly
> > > say:
> > > if len(m) == 0 {
> > >   return nil, ErrMetaFieldUnset
> > > }
> > > if idx >= len(m) {
> > >   panic("ArrayIndexOutOfBoundsException")
> > > }
> > 
> > WDYT about
> > 
> > if m == nil {
> 
> er, I mean len(m) == 0
> 
> >   return nil, ErrMetaFieldUnset
> > }
> > return m[idx].GetMeta(key)  // will panic on its own

Mer... I don't like the asymmetry. MultiMetaGetter is supposed to be a really
safe object. Getting an error back (ErrMetaFieldUnset) is fine, IMO.

[dnj, 2015-08-05 00:02:21]

> Mer... I don't like the asymmetry. MultiMetaGetter is supposed to be a really
> safe object. Getting an error back (ErrMetaFieldUnset) is fine, IMO.

*shrug*. If it's only really for internal API, then I would not be upset
asserting that the internal API correctly allocated sizes, as it would be a bug
if it didn't. If so, I like your construction :)

[iannucci, 2015-08-05 00:04:52]

On 2015/08/05 00:02:21, dnj wrote:
> > Mer... I don't like the asymmetry. MultiMetaGetter is supposed to be a
really
> > safe object. Getting an error back (ErrMetaFieldUnset) is fine, IMO.
> 
> *shrug*. If it's only really for internal API, then I would not be upset
> asserting that the internal API correctly allocated sizes, as it would be a
bug
> if it didn't. If so, I like your construction :)

The 'users' of this package are filter implementors... technically internal, but
I want to minimize the amount of boilerplate. I don't think that this would be
masking a real error, since they have to loop over keys anyway... if they're out
of bounds then who knows what they're doing (and that'll panic by itself).

PTAL @ the last patchset. I added tests and reduced the amount of copypaste.

[dnj, 2015-08-05 00:38:41]

Nice boilerplate crushing.

https://codereview.chromium.org/1270113002/diff/100001/service/datastore/prop...
File service/datastore/properties.go (right):

https://codereview.chromium.org/1270113002/diff/100001/service/datastore/prop...
service/datastore/properties.go:227: func UpconvertUnderlyingType(o interface{},
t reflect.Type) (interface{}, reflect.Type) {
Is this used elsewhere? What's the point of taking "t" as a separate parameter
and not just performing "reflect.TypeOf" in here?

https://codereview.chromium.org/1270113002/diff/100001/service/datastore/prop...
service/datastore/properties.go:487: dflt, typ := UpconvertUnderlyingType(dflt,
reflect.TypeOf(dflt))
Since you're always calling this anyway, do it above the "gm" call so the call
and its error handling are next to each other.

[iannucci, 2015-08-05 00:58:15]

ptal

https://codereview.chromium.org/1270113002/diff/100001/service/datastore/prop...
File service/datastore/properties.go (right):

https://codereview.chromium.org/1270113002/diff/100001/service/datastore/prop...
service/datastore/properties.go:227: func UpconvertUnderlyingType(o interface{},
t reflect.Type) (interface{}, reflect.Type) {
On 2015/08/05 00:38:41, dnj wrote:
> Is this used elsewhere? What's the point of taking "t" as a separate parameter
> and not just performing "reflect.TypeOf" in here?

Yes, it's used in Property.SetValue. That said, the type parameter isn't really
needed anymore.

https://codereview.chromium.org/1270113002/diff/100001/service/datastore/prop...
service/datastore/properties.go:487: dflt, typ := UpconvertUnderlyingType(dflt,
reflect.TypeOf(dflt))
On 2015/08/05 00:38:41, dnj wrote:
> Since you're always calling this anyway, do it above the "gm" call so the call
> and its error handling are next to each other.

Done.

[dnj, 2015-08-05 00:59:47]

Missing patch set?

[iannucci, 2015-08-05 01:02:54]

On 2015/08/05 00:59:47, dnj wrote:
> Missing patch set?

d'oh.. uploaded to the wrong cl. PTAL

[dnj (Google), 2015-08-05 01:23:31]

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

[dnj (Google), 2015-08-05 01:23:31]

lgtm

[iannucci, 2015-08-05 01:24:06]

Committed patchset #8 (id:140001) manually as
00d1a80545d14379fbe4b851ff638f7e9be85e84 (presubmit successful).