Decouple PLS from MGS.

Decouple PLS from MGS.

Decouple PropertyLoadSaver from MetaGetterSetter. This allows
implementing structs to override either one of those interfaces without
depending on spooky the interaction between them implemented by
"structPLS".

"structPLS" no longer magic-detects if its type implements
MetaGetterSetter and defers to it. Instead, a struct that implements
MetaGetterSetter and wants to use "structPLS" cool meta defaults
can call GetPLS(this) to get a handle on that base functionality.

BUG=

Committed: https://github.com/luci/gae/commit/3932cd48f52a6fd2051c48c183cd3c0a17ae4913

[dnj, 2015-10-30 07:47:55]

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

[dnj, 2015-10-30 07:47:55]

PTAL, hard to tell if I'm missing anything, but it passes all tests, so there's
that.

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
File service/datastore/pls_impl.go (right):

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
service/datastore/pls_impl.go:221: if !p.o.IsValid() {
This is a bug I stumbled on: if an ErrInvalidType structPLS is instantiated and
methods are called on it, this can be invoked when "p.o" is not valid.

[iannucci, 2015-10-30 20:21:07]

lgtm (certainly better than before) + comment/question.

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
service/datastore/pls.go:112: func getMGS(obj interface{}) MetaGetterSetter {
So it's a bit weird. For MGS overriding, it's kind of 'merged' between the
struct fields and the overridden methods. For PLS overriding, you have to call
GetPLS in your PLS methods. WDYT about the ergonomics of that asymmetry? I'm
still forming an opinion.

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
File service/datastore/pls_impl.go (right):

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
service/datastore/pls_impl.go:221: if !p.o.IsValid() {
On 2015/10/30 at 07:47:55, dnj wrote:
> This is a bug I stumbled on: if an ErrInvalidType structPLS is instantiated
and methods are called on it, this can be invoked when "p.o" is not valid.

Ah, ok. Good catch. Did you add a test for it?

[dnj, 2015-10-30 20:34:48]

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
service/datastore/pls.go:112: func getMGS(obj interface{}) MetaGetterSetter {
On 2015/10/30 20:21:06, iannucci wrote:
> So it's a bit weird. For MGS overriding, it's kind of 'merged' between the
> struct fields and the overridden methods. For PLS overriding, you have to call
> GetPLS in your PLS methods. WDYT about the ergonomics of that asymmetry? I'm
> still forming an opinion.

I think from a user's perspective, there are four options:
1) Do nothing, in which case stuctPLS is used and it Just Works!
2) Implement PropertyLoadSaver only, in which case getMGS uses structPLS
MetaGetterSetter and it Just Works!
3) Implement MetaGetterSetter only, in which case structPLS does all of its
stuff, but you have to be explicitly aware that you completely own the MGS
interface. If you want any of the default behavior, you have to explicitly ask
for it. This is the same as (2) for PLS though, so I think it's consistent.
4) Implement both, in which case you own everything consistently (see (2), (3)).

I think it's sensible. There's less magic and slightly more awareness needed,
but it's also consistent with how other Go overloading works, so it feels
reasonable. WDYT?

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
File service/datastore/pls_impl.go (right):

https://chromiumcodereview.appspot.com/1427933002/diff/1/service/datastore/pl...
service/datastore/pls_impl.go:221: if !p.o.IsValid() {
On 2015/10/30 20:21:06, iannucci wrote:
> On 2015/10/30 at 07:47:55, dnj wrote:
> > This is a bug I stumbled on: if an ErrInvalidType structPLS is instantiated
> and methods are called on it, this can be invoked when "p.o" is not valid.
> 
> Ah, ok. Good catch. Did you add a test for it?

Done.

[iannucci, 2015-10-30 21:29:29]

https://chromiumcodereview.appspot.com/1427933002/diff/40001/service/datastor...
File service/datastore/pls_impl.go (left):

https://chromiumcodereview.appspot.com/1427933002/diff/40001/service/datastor...
service/datastore/pls_impl.go:338: ret = p.getMGS().GetAllMeta()
This is the code that I was wondering about. I'm not sure why it's gone now?

[dnj, 2015-10-30 21:40:04]

https://chromiumcodereview.appspot.com/1427933002/diff/40001/service/datastor...
File service/datastore/pls_impl.go (left):

https://chromiumcodereview.appspot.com/1427933002/diff/40001/service/datastor...
service/datastore/pls_impl.go:338: ret = p.getMGS().GetAllMeta()
On 2015/10/30 21:29:29, iannucci wrote:
> This is the code that I was wondering about. I'm not sure why it's gone now?

Gone b/c:
1) No fallthroughs, structPLS is pure now!
2) This logic was for friendly fuzziness: you can implement GetAllMeta, and if
you don't export $kind, we'll do it for you for free!

RE (2), the new mentality is, `if you don't export $kind, you should call
GetPLS(me).GetMeta("$kind") and that's on you`. I added a test to assert that
datastore has an issue with this failure higher up.

[iannucci, 2015-10-30 21:47:07]

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

[iannucci, 2015-10-30 21:47:07]

ok, I understand this better now. still lgtm :)

I don't think any code in luci-go used the other behavior yet, but I'll need to
tweak dm (not a big deal). Vadim may have used the old flow too, so adding him.

TL;DR: now when you override MetaGetterSetter, your "I'm not handling this meta
property" case is to call GetPLS(self).GetMeta(...), instead of returning
ErrMetaFieldUnset and having structPLS magically take care of it.

https://chromiumcodereview.appspot.com/1427933002/diff/60001/service/datastor...
File service/datastore/pls_test.go (right):

https://chromiumcodereview.appspot.com/1427933002/diff/60001/service/datastor...
service/datastore/pls_test.go:1719: pls := GetPLS(&Simple{})
may be clearer to just put `type Simple struct{}` in this function. *shrug*

[Vadim Sh., 2015-10-30 21:54:20]

I don't think my code depended on the difference. If it did, unit tests should
break in some helpful way.

[Vadim Sh., 2015-10-30 21:56:50]

lgtm

https://codereview.chromium.org/1427933002/diff/80001/service/datastore/inter...
File service/datastore/interface.go (right):

https://codereview.chromium.org/1427933002/diff/80001/service/datastore/inter...
service/datastore/interface.go:79: // implements GetMeta to make some minor
tweaks can evoke the defualt behavior
typo: defualt

[iannucci, 2015-10-30 22:02:54]

https://chromiumcodereview.appspot.com/1427933002/diff/100001/service/datasto...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1427933002/diff/100001/service/datasto...
service/datastore/pls.go:113: //     return GetPLS(props)
I think this should be `GetPLS(s).Load(props)`

https://chromiumcodereview.appspot.com/1427933002/diff/100001/service/datasto...
service/datastore/pls.go:117: //     props, err := GetPLS(withMeta)
GetPLS(s).Save(withMeta)

[iannucci, 2015-10-30 22:12:08]

lgtm

[dnj, 2015-10-30 22:12:26]

The CQ bit was checked by dnj@chromium.org

[dnj, 2015-10-30 22:12:26]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1427933002/#ps120001 (title: "Derp")

[commit-bot: I haz the power, 2015-10-30 22:12:39]

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

[commit-bot: I haz the power, 2015-10-30 22:15:55]

Committed patchset #7 (id:120001) as
https://github.com/luci/gae/commit/3932cd48f52a6fd2051c48c183cd3c0a17ae4913