Fix miscellaneous prod bugs.

Fix miscellaneous prod bugs.

- Fix bug where "nil" Parent field was being translated into non-nil
  empty key.
- Use fully qualified AppID for key AppID in checkfilter.
- Use a context-installed singleton to probe AppEngine runtime parameters.
- Bump Task field count to match gae/AppEngine count.
- Fix UTC time check when Locations are the same, but have different
  pointers.
- Pass PropertyLoadSaver value array (rather than source) in
  prod/PutMulti :)
- Use struct name as default even when it implements PropertyLoadSaver.

R=iannucci@chromium.org

Committed: https://github.com/luci/gae/commit/fcb2b4639aa8014c6de0f7f0c33d4260371e945e

[dnj (Google), 2015-08-14 16:00:25]

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

[dnj (Google), 2015-08-14 16:00:25]

PTAL

[dnj (Google), 2015-08-14 16:08:32]

https://codereview.chromium.org/1289323002/diff/1/impl/prod/info.go
File impl/prod/info.go (right):

https://codereview.chromium.org/1289323002/diff/1/impl/prod/info.go#newcode37
impl/prod/info.go:37: id = "dev~" + id
This primarily affected the key's namespace, which was being derived from and
checked against AppID. On dev_appserver, the key namespace didn't match the
native datastore library's assigned namespace.

Now that I think about it, though, this is probably not correct. AppID is still
the same; just the "aid" value for the key is wrong. I'll update to apply this
logic in the "aid" generation.

[dnj (Google), 2015-08-15 01:51:26]

Now with moar fixez!

[iannucci, 2015-08-15 02:23:53]

https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
File service/datastore/multiarg.go (right):

https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
service/datastore/multiarg.go:271: return pls, v.Type().Name()
This looks really wrong to me. We already see if it's a *structPLS above. This
is for implementors of just the PropertyLoadSaver interface (e.g. PropertyMap),
which won't have .Type()

[dnj (Google), 2015-08-15 02:32:33]

https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
File service/datastore/multiarg.go (right):

https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
service/datastore/multiarg.go:239: return nil, fmt.Errorf("unable to extract
$kind from %v", src)
(this error)

https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
service/datastore/multiarg.go:271: return pls, v.Type().Name()
On 2015/08/15 02:23:53, iannucci wrote:
> This looks really wrong to me. We already see if it's a *structPLS above. This
> is for implementors of just the PropertyLoadSaver interface (e.g.
PropertyMap),
> which won't have .Type()

The problem occurs when I implement a custom PropertyLoadSaver. That
PropertyLoadSaver doesn't set the $kind meta, so newKeyObjErr gets back "" for
the name. It then runs GetMetaDefault("kind", ""), and since my PLS doesn't
declare/export $kind, the name is "" and I hit the ensuing error.

[iannucci, 2015-08-15 16:20:20]

On 2015/08/15 02:32:33, dnj (Google) wrote:
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> File service/datastore/multiarg.go (right):
> 
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> service/datastore/multiarg.go:239: return nil, fmt.Errorf("unable to extract
> $kind from %v", src)
> (this error)
> 
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> service/datastore/multiarg.go:271: return pls, v.Type().Name()
> On 2015/08/15 02:23:53, iannucci wrote:
> > This looks really wrong to me. We already see if it's a *structPLS above.
This
> > is for implementors of just the PropertyLoadSaver interface (e.g.
> PropertyMap),
> > which won't have .Type()
> 
> The problem occurs when I implement a custom PropertyLoadSaver. That
> PropertyLoadSaver doesn't set the $kind meta, so newKeyObjErr gets back "" for
> the name. It then runs GetMetaDefault("kind", ""), and since my PLS doesn't
> declare/export $kind, the name is "" and I hit the ensuing error.

Just add $kind as part of your PLS implementation?

[iannucci, 2015-08-15 16:20:32]

On 2015/08/15 16:20:20, iannucci wrote:
> On 2015/08/15 02:32:33, dnj (Google) wrote:
> >
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> > File service/datastore/multiarg.go (right):
> > 
> >
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> > service/datastore/multiarg.go:239: return nil, fmt.Errorf("unable to extract
> > $kind from %v", src)
> > (this error)
> > 
> >
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> > service/datastore/multiarg.go:271: return pls, v.Type().Name()
> > On 2015/08/15 02:23:53, iannucci wrote:
> > > This looks really wrong to me. We already see if it's a *structPLS above.
> This
> > > is for implementors of just the PropertyLoadSaver interface (e.g.
> > PropertyMap),
> > > which won't have .Type()
> > 
> > The problem occurs when I implement a custom PropertyLoadSaver. That
> > PropertyLoadSaver doesn't set the $kind meta, so newKeyObjErr gets back ""
for
> > the name. It then runs GetMetaDefault("kind", ""), and since my PLS doesn't
> > declare/export $kind, the name is "" and I hit the ensuing error.
> 
> Just add $kind as part of your PLS implementation?

Or $key

[iannucci, 2015-08-15 16:21:50]

On 2015/08/15 16:20:32, iannucci wrote:
> On 2015/08/15 16:20:20, iannucci wrote:
> > On 2015/08/15 02:32:33, dnj (Google) wrote:
> > >
> >
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> > > File service/datastore/multiarg.go (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> > > service/datastore/multiarg.go:239: return nil, fmt.Errorf("unable to
extract
> > > $kind from %v", src)
> > > (this error)
> > > 
> > >
> >
>
https://codereview.chromium.org/1289323002/diff/40001/service/datastore/multi...
> > > service/datastore/multiarg.go:271: return pls, v.Type().Name()
> > > On 2015/08/15 02:23:53, iannucci wrote:
> > > > This looks really wrong to me. We already see if it's a *structPLS
above.
> > This
> > > > is for implementors of just the PropertyLoadSaver interface (e.g.
> > > PropertyMap),
> > > > which won't have .Type()
> > > 
> > > The problem occurs when I implement a custom PropertyLoadSaver. That
> > > PropertyLoadSaver doesn't set the $kind meta, so newKeyObjErr gets back ""
> for
> > > the name. It then runs GetMetaDefault("kind", ""), and since my PLS
doesn't
> > > declare/export $kind, the name is "" and I hit the ensuing error.
> > 
> > Just add $kind as part of your PLS implementation?
> 
> Or $key

Actually, I think you're doing something wrong. If you do
GetPLS(self).Save(true), you'll get a PropertyMap which includes all the
kind/id/etc information, assuming you're using annotated fields. These should
work just fine?

[dnj (Google), 2015-08-15 17:50:36]

> > > Just add $kind as part of your PLS implementation?
> > Or $key

I could do that; however, it seems like the wrong approach. The struct
auto-describes itself in other circumstances. Implementing a PLS shouldn't lose
that perk.

> Actually, I think you're doing something wrong. If you do
> GetPLS(self).Save(true), you'll get a PropertyMap which includes all the
> kind/id/etc information, assuming you're using annotated fields. These should
> work just fine?

I'm calling:
func (s *LogStream) Save(withMeta bool) (ds.PropertyMap, error) {
  pmap, err := ds.GetPLS(s).Save(withMeta)

  // Custom fields...
}

"withMeta" is true, so I think I'm doing it right. The problem here is that I am
not setting "$kind", so even with "withMeta=true" the PropertyMap isn't
including $kind and, consequently, the default (invalid) name, "", is being
used.

[dnj (Google), 2015-08-15 17:53:04]

> "withMeta" is true, so I think I'm doing it right. The problem here is that I
am
> not setting "$kind", so even with "withMeta=true" the PropertyMap isn't
> including $kind and, consequently, the default (invalid) name, "", is being
> used.

Would a better solution be to have structPLS always add "$kind" metadata,
defaulting to struct name if missing?

[iannucci, 2015-08-15 17:55:00]

On 2015/08/15 17:53:04, dnj (Google) wrote:
> > "withMeta" is true, so I think I'm doing it right. The problem here is that
I
> am
> > not setting "$kind", so even with "withMeta=true" the PropertyMap isn't
> > including $kind and, consequently, the default (invalid) name, "", is being
> > used.
> 
> Would a better solution be to have structPLS always add "$kind" metadata,
> defaulting to struct name if missing?

Ah... yeah that makes sense to me. It's kind of an implied metadata. That may
actually just be all-around cleaner than the current 'get name thing'. Then it
would just be .GetMeta("kind") for every implementor. I like that a lot better.

[dnj (Google), 2015-08-15 18:48:45]

Yo dawg, I did some updates.

[iannucci, 2015-08-15 21:50:54]

lgtm

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/check...
File service/datastore/checkfilter.go (right):

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/check...
service/datastore/checkfilter.go:123: }
This is the wrong place to apply this. memory.info.AppID should return this
directly.

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/multi...
File service/datastore/multiarg.go (right):

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/multi...
service/datastore/multiarg.go:237: kindIface, err := pls.GetMeta("kind")
I think you could GetMetaDefault("kind", "").(string), and just return the first
error if it == ""

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/pls_t...
File service/datastore/pls_test.go (right):

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/pls_t...
service/datastore/pls_test.go:1717: "$kind": {mp("OKDefaults")},
technically when you SetMeta on a PropertyMap, it sets them to 'noindex'. Not
that it matters, because they'll never be serialized. But that's why the other
lines are mpNI

[dnj (Google), 2015-08-16 06:11:07]

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/check...
File service/datastore/checkfilter.go (right):

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/check...
service/datastore/checkfilter.go:123: }
On 2015/08/15 21:50:54, iannucci wrote:
> This is the wrong place to apply this. memory.info.AppID should return this
> directly.

The actual AppID (as returned by appengine.AppID()) does _not_ have the "dev~"
prefix. It's just prepended to the keys generated by "appengine". Adjusting the
formal AppID would be wrong in this case.

What's happening is:
1) GAE is asking "appengine" layer to create a key. "appengine" does so, using
AppID and prepending "dev~".
2) Fine so far, GAE translates that key into a valid GAE key, cool.
3) checkfilter says, "hey key, you're supposed to have info.AppID(). You fail
teh chex."

So everything is great except for checkFilter's idea of what the AppID should
be. That's why I put this logic here.

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/multi...
File service/datastore/multiarg.go (right):

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/multi...
service/datastore/multiarg.go:237: kindIface, err := pls.GetMeta("kind")
On 2015/08/15 21:50:54, iannucci wrote:
> I think you could GetMetaDefault("kind", "").(string), and just return the
first
> error if it == ""

Done.

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/pls_t...
File service/datastore/pls_test.go (right):

https://codereview.chromium.org/1289323002/diff/80001/service/datastore/pls_t...
service/datastore/pls_test.go:1717: "$kind": {mp("OKDefaults")},
On 2015/08/15 21:50:54, iannucci wrote:
> technically when you SetMeta on a PropertyMap, it sets them to 'noindex'. Not
> that it matters, because they'll never be serialized. But that's why the other
> lines are mpNI

Switched to NI.

[iannucci, 2015-08-16 07:57:50]

Oh gross... I suspect that behavior will require changes in impl/memory as
well.

On Sat, Aug 15, 2015, 23:11  <dnj@google.com> wrote:

>
>
>
https://codereview.chromium.org/1289323002/diff/80001/service/datastore/check...
> File service/datastore/checkfilter.go (right):
>
>
>
https://codereview.chromium.org/1289323002/diff/80001/service/datastore/check...
> service/datastore/checkfilter.go:123
>
<https://codereview.chromium.org/1289323002/diff/80001/service/datastore/check...>:
> }
> On 2015/08/15 21:50:54, iannucci wrote:
> > This is the wrong place to apply this. memory.info.AppID should return
> this
> > directly.
>
> The actual AppID (as returned by appengine.AppID()) does _not_ have the
> "dev~" prefix. It's just prepended to the keys generated by "appengine".
> Adjusting the formal AppID would be wrong in this case.
>
> What's happening is:
> 1) GAE is asking "appengine" layer to create a key. "appengine" does so,
> using AppID and prepending "dev~".
> 2) Fine so far, GAE translates that key into a valid GAE key, cool.
> 3) checkfilter says, "hey key, you're supposed to have info.AppID(). You
> fail teh chex."
>
> So everything is great except for checkFilter's idea of what the AppID
> should be. That's why I put this logic here.
>
>
>
https://codereview.chromium.org/1289323002/diff/80001/service/datastore/multi...
> File service/datastore/multiarg.go (right):
>
>
>
https://codereview.chromium.org/1289323002/diff/80001/service/datastore/multi...
> service/datastore/multiarg.go:237
>
<https://codereview.chromium.org/1289323002/diff/80001/service/datastore/multi...>:
> kindIface, err := pls.GetMeta("kind")
> On 2015/08/15 21:50:54, iannucci wrote:
> > I think you could GetMetaDefault("kind", "").(string), and just return
> the first
> > error if it == ""
>
> Done.
>
>
>
https://codereview.chromium.org/1289323002/diff/80001/service/datastore/pls_t...
> File service/datastore/pls_test.go (right):
>
>
>
https://codereview.chromium.org/1289323002/diff/80001/service/datastore/pls_t...
> service/datastore/pls_test.go:1717
>
<https://codereview.chromium.org/1289323002/diff/80001/service/datastore/pls_t...>:
> "$kind": {mp("OKDefaults")},
> On 2015/08/15 21:50:54, iannucci wrote:
> > technically when you SetMeta on a PropertyMap, it sets them to
> 'noindex'. Not
> > that it matters, because they'll never be serialized. But that's why
> the other
> > lines are mpNI
>
> Switched to NI.
>
> https://codereview.chromium.org/1289323002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dnj (Google), 2015-08-17 17:43:27]

On 2015/08/16 07:57:50, iannucci wrote:
> Oh gross... I suspect that behavior will require changes in impl/memory as
> well.

I don't know about this. The prefix is a function of "dev_appserver', which
impl/memory does not (necessarily) have to implement If you're okay with it, I'd
prefer to leave impl/memory out for now.

[iannucci, 2015-08-17 17:47:24]

On 2015/08/17 17:43:27, dnj (Google) wrote:
> On 2015/08/16 07:57:50, iannucci wrote:
> > Oh gross... I suspect that behavior will require changes in impl/memory as
> > well.
> 
> I don't know about this. The prefix is a function of "dev_appserver', which
> impl/memory does not (necessarily) have to implement If you're okay with it,
I'd
> prefer to leave impl/memory out for now.

I was meaning that this namespace is used for Key validation. It needs to match
in impl/memory, or it will consider all your keys to be invalid.

[dnj, 2015-08-17 20:33:15]

Updated once more, haven't touched "impl/memory", but this update should obviate
the need for it.

Datastore keys use the "fully qualified app ID", which includes prefixes (e.g.,
"dev~" for DevAppserver, "s~" for high-replication, etc.). Checkfilter now
understands this, and "info" service can now return it.

PTAL.

[iannucci, 2015-08-17 20:38:00]

lgtm thanks :)

[iannucci, 2015-08-17 21:16:22]

On 2015/08/17 20:38:00, iannucci wrote:
> lgtm thanks :)

Oh, could you update the description too?

[iannucci, 2015-08-17 21:17:39]

lgtm :-)

https://codereview.chromium.org/1289323002/diff/140001/impl/prod/info.go
File impl/prod/info.go (right):

https://codereview.chromium.org/1289323002/diff/140001/impl/prod/info.go#newc...
impl/prod/info.go:113: probeKey := datastore.NewKey(c, "", "", 0, nil)
I would make this a valid-looking key, just in case they decide to start
validating in the sdk. `NewKey("Kind", "id", 0, nil)`

[dnj, 2015-08-17 21:22:54]

Description updated, latest tested on prod, looks good.

https://codereview.chromium.org/1289323002/diff/140001/impl/prod/info.go
File impl/prod/info.go (right):

https://codereview.chromium.org/1289323002/diff/140001/impl/prod/info.go#newc...
impl/prod/info.go:113: probeKey := datastore.NewKey(c, "", "", 0, nil)
On 2015/08/17 21:17:39, iannucci wrote:
> I would make this a valid-looking key, just in case they decide to start
> validating in the sdk. `NewKey("Kind", "id", 0, nil)`

Done.

[iannucci, 2015-08-17 21:26:07]

On 2015/08/17 21:22:54, dnj wrote:
> Description updated, latest tested on prod, looks good.
> 

missiles -> fire them

> https://codereview.chromium.org/1289323002/diff/140001/impl/prod/info.go
> File impl/prod/info.go (right):
> 
>
https://codereview.chromium.org/1289323002/diff/140001/impl/prod/info.go#newc...
> impl/prod/info.go:113: probeKey := datastore.NewKey(c, "", "", 0, nil)
> On 2015/08/17 21:17:39, iannucci wrote:
> > I would make this a valid-looking key, just in case they decide to start
> > validating in the sdk. `NewKey("Kind", "id", 0, nil)`
> 
> Done.

[dnj, 2015-08-17 21:26:55]

Committed patchset #9 (id:160001) manually as
fcb2b4639aa8014c6de0f7f0c33d4260371e945e (presubmit successful).