Fix error message from KeyForObj when passing an invalid struct.

Clean up PropertyLoadSaver interface.

This removes GetMetaDefault and Problem from the interfaces, and makes GetPLS 
errors into panics, on the assumption that if you pass it a bad datatype, you 
can't really do anything about it without recompiling, and failing fast is better.

This also fixes a bug where doing any operations on an invalid (non datastore-
compatible) struct would return the unhelpful "unable to extract $kind" error.

R=dnj@chromium.org, hinoka@chromium.org, martiniss@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/gae/commit/043deeee59a8e1203b3350fb9d315b3d0d7db076

[iannucci, 2015-12-11 05:45:45]

OK, PTAL. This was supposed to just be a fix for the stupid error message, but
it ended up being something... much more... *ominous music*

This now fixes the GetMetaDefault stupidity (sorry everyone, it won't happen
again (probably)).

[dnj, 2015-12-11 06:36:17]

https://chromiumcodereview.appspot.com/1516173002/diff/20001/service/datastor...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1516173002/diff/20001/service/datastor...
service/datastore/pls.go:161: Problem() error
Since Problem() is only used by tests, why export it here? WDYT about either:

(a) making it a private interface (Problem -> problem)?
(b) removing it here and just type-asserting the test struct to "*structPLS"?

[iannucci, 2015-12-12 00:20:12]

OK, PTAL pls. This fixes all of the problems.

[iannucci, 2015-12-12 00:49:23]

Description was changed from

==========
Fix error message from KeyForObj when passing an invalid struct.

Previously doing any operations on an invalid (non datastore-compatible) struct
would return the unhelpful "unable to extract $kind" error.

R=dnj@chromium.org, hinoka@chromium.org, martiniss@chromium.org,
vadimsh@chromium.org
BUG=
==========

to

==========
Clean up PropertyLoadSaver interface.

This removes GetMetaDefault and Problem from the interfaces, and makes GetPLS
errors into panics, on the assumption that if you pass it a bad datatype, you
can't really do anything about it without recompiling, and failing fast is
better.

This also fixes a bug where doing any operations on an invalid (non
datastore-compatible) struct would return the unhelpful "unable to extract
$kind" error.

R=dnj@chromium.org, hinoka@chromium.org, martiniss@chromium.org,
vadimsh@chromium.org
BUG=
==========

[iannucci, 2015-12-12 01:22:58]

Description was changed from

==========
Clean up PropertyLoadSaver interface.

This removes GetMetaDefault and Problem from the interfaces, and makes GetPLS
errors into panics, on the assumption that if you pass it a bad datatype, you
can't really do anything about it without recompiling, and failing fast is
better.

This also fixes a bug where doing any operations on an invalid (non
datastore-compatible) struct would return the unhelpful "unable to extract
$kind" error.

R=dnj@chromium.org, hinoka@chromium.org, martiniss@chromium.org,
vadimsh@chromium.org
BUG=
==========

to

==========
Clean up PropertyLoadSaver interface.

This removes GetMetaDefault and Problem from the interfaces, and makes GetPLS 
errors into panics, on the assumption that if you pass it a bad datatype, you 
can't really do anything about it without recompiling, and failing fast is
better.

This also fixes a bug where doing any operations on an invalid (non datastore-
compatible) struct would return the unhelpful "unable to extract $kind" error.

R=dnj@chromium.org, hinoka@chromium.org, martiniss@chromium.org,
vadimsh@chromium.org
BUG=
==========

[dnj, 2015-12-12 02:47:50]

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
service/datastore/pls.go:21: // property types), this function will panic Other
problems include duplicate
nit: panic<period>.

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
File service/datastore/pls_impl.go (right):

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
service/datastore/pls_impl.go:478: return
I think you should panic when the problem is identified, such that it is
impossible to create a structPLS from an invalid structure. It seems unintuitive
to me to have the panic be delayed until the structPLS is actually used.

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
File service/datastore/properties.go (left):

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
service/datastore/properties.go:664: GetMetaDefault(key string, dflt
interface{}) interface{}
w00t

[iannucci, 2015-12-12 03:52:22]

PTALP

https://chromiumcodereview.appspot.com/1516173002/diff/20001/filter/dscache/d...
File filter/dscache/ds.go (right):

https://chromiumcodereview.appspot.com/1516173002/diff/20001/filter/dscache/d...
filter/dscache/ds.go:93: // TODO(riannucci): Not sure what to do with this.
as in; this should basically never happen, but if it does happen, we should know
about it. Plumbing it through in this function as it's written right now is
going to be a pain in the butt :/

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

https://chromiumcodereview.appspot.com/1516173002/diff/20001/service/datastor...
service/datastore/pls_test.go:1624: })
This silliness is because of the way the tests are structured. No one should
ever need to do this for real.

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
service/datastore/pls.go:21: // property types), this function will panic Other
problems include duplicate
On 2015/12/12 at 02:47:50, dnj wrote:
> nit: panic<period>.

done

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
File service/datastore/pls_impl.go (right):

https://chromiumcodereview.appspot.com/1516173002/diff/80001/service/datastor...
service/datastore/pls_impl.go:478: return
On 2015/12/12 at 02:47:50, dnj wrote:
> I think you should panic when the problem is identified, such that it is
impossible to create a structPLS from an invalid structure. It seems unintuitive
to me to have the panic be delayed until the structPLS is actually used.

Ok, now we panic harder (in getCodec).

[dnj, 2015-12-12 04:04:53]

Nice! LGTM (w/ comment)

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

https://chromiumcodereview.appspot.com/1516173002/diff/140001/service/datasto...
service/datastore/pls.go:193: if c.problem != nil {
IMO: While I find the effort to avoid recomputing an invalid codec noble, I
think the complication of retaining "problem" is more complicated than
necessary. The only use case is when a user computes the struct in a
"panic/recover" loop, else they'd just be dead, and those folk have more
problems than some reflection inefficiency. I think you should remove the
"problem" member and just panic in "getStructCodecLocked" if the struct is bad.

I'm okay either way, just wanted to put this out there :)

[iannucci, 2015-12-12 04:34:00]

as i mentioned elsewhere, it makes detecting recursion really easy and I
already spent too much time on this patch :P

On Fri, Dec 11, 2015, 20:04  <dnj@chromium.org> wrote:

> Nice! LGTM (w/ comment)
>
>
>
>
https://chromiumcodereview.appspot.com/1516173002/diff/140001/service/datasto...
> File service/datastore/pls.go (right):
>
>
>
https://chromiumcodereview.appspot.com/1516173002/diff/140001/service/datasto...
> service/datastore/pls.go:193
>
<https://chromiumcodereview.appspot.com/1516173002/diff/140001/service/datasto...>:
> if c.problem != nil {
> IMO: While I find the effort to avoid recomputing an invalid codec
> noble, I think the complication of retaining "problem" is more
> complicated than necessary. The only use case is when a user computes
> the struct in a "panic/recover" loop, else they'd just be dead, and
> those folk have more problems than some reflection inefficiency. I think
> you should remove the "problem" member and just panic in
> "getStructCodecLocked" if the struct is bad.
>
> I'm okay either way, just wanted to put this out there :)
>
> https://chromiumcodereview.appspot.com/1516173002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[iannucci, 2015-12-12 17:15:03]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-12-12 17:15:14]

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

[commit-bot: I haz the power, 2015-12-12 17:23:05]

Description was changed from

==========
Clean up PropertyLoadSaver interface.

This removes GetMetaDefault and Problem from the interfaces, and makes GetPLS 
errors into panics, on the assumption that if you pass it a bad datatype, you 
can't really do anything about it without recompiling, and failing fast is
better.

This also fixes a bug where doing any operations on an invalid (non datastore-
compatible) struct would return the unhelpful "unable to extract $kind" error.

R=dnj@chromium.org, hinoka@chromium.org, martiniss@chromium.org,
vadimsh@chromium.org
BUG=
==========

to

==========
Clean up PropertyLoadSaver interface.

This removes GetMetaDefault and Problem from the interfaces, and makes GetPLS 
errors into panics, on the assumption that if you pass it a bad datatype, you 
can't really do anything about it without recompiling, and failing fast is
better.

This also fixes a bug where doing any operations on an invalid (non datastore-
compatible) struct would return the unhelpful "unable to extract $kind" error.

R=dnj@chromium.org, hinoka@chromium.org, martiniss@chromium.org,
vadimsh@chromium.org
BUG=

Committed:
https://github.com/luci/gae/commit/043deeee59a8e1203b3350fb9d315b3d0d7db076
==========

[commit-bot: I haz the power, 2015-12-12 17:23:06]

Committed patchset #8 (id:140001) as
https://github.com/luci/gae/commit/043deeee59a8e1203b3350fb9d315b3d0d7db076