impl/memory: Make queries self-validate

impl/memory: Make queries self-validate

This moves validation and normalization logic into the query construction
process itself. This makes it so that queries are either valid and normalized,
or they contain an error (and query building 'sticks' on the first error
encountered).

This replaces the previous build-a-questionable-query-and-then-try-to-make-it-sensible
logic (which was copied from dev_appserver for the most part).

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

Committed: https://github.com/luci/gae/commit/9f570ae76af2ec00cbc915f8c2200d5339607273

[dnj (Google), 2015-08-14 21:01:34]

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

[dnj (Google), 2015-08-14 21:01:35]

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore.go
File impl/memory/datastore.go (right):

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore.g...
impl/memory/datastore.go:73: if done || err != nil {
What's the value of "done"? From what I can see, you might as well just have
"valid" return nil if done.

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
File impl/memory/datastore_query.go (right):

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:17: const MaxQueryComponents = 100
Is this a hard limit, or one you chose? Either way, should have some
documentation explaining it.

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:74: lastIdx := len(bstr) - 1
Can we have an empty bstr?

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:208: nq.order = append([]ds.IndexColumn(nil),
q.order...)
I used to like doing this; however, "append" will likely overallocate capacity.
Since we know the size, we should probably just do a make/copy.

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:261: func() (err error) {
No point in a named variable that you only use it once!

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:276: return err
If still using named return variables, just "return".

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:285: buf := &bytes.Buffer{}
Might as well do this down near return statement, since that's the only time you
write to it.

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:357: } else if strings.HasPrefix(prop, "+") {
Why specifically "+"? Just catch a common user error? Maybe update the error
message to, "datastore: cannot use +, but the default ordering is ascending".

https://codereview.chromium.org/1286943002/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query.go:396: func() error {
Test for duplicate project clauses, or is that cool?

[iannucci, 2015-08-14 22:25:48]

done

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
File impl/memory/datastore.go (right):

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore.go:73: if done || err != nil {
On 2015/08/14 21:01:34, dnj (Google) wrote:
> What's the value of "done"? From what I can see, you might as well just have
> "valid" return nil if done.

it's in case the user specified a query with zero results. this code will change
when queries are fully implemented.

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:17: const MaxQueryComponents = 100
On 2015/08/14 21:01:35, dnj (Google) wrote:
> Is this a hard limit, or one you chose? Either way, should have some
> documentation explaining it.

It has the same amount of documentation as the constant in dev_appserver :)

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:74: lastIdx := len(bstr) - 1
On 2015/08/14 21:01:35, dnj (Google) wrote:
> Can we have an empty bstr?

nope

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:208: nq.order = append([]ds.IndexColumn(nil),
q.order...)
On 2015/08/14 21:01:35, dnj (Google) wrote:
> I used to like doing this; however, "append" will likely overallocate
capacity.
> Since we know the size, we should probably just do a make/copy.

done

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:261: func() (err error) {
On 2015/08/14 21:01:35, dnj (Google) wrote:
> No point in a named variable that you only use it once!

*shrug* changed

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:276: return err
On 2015/08/14 21:01:35, dnj (Google) wrote:
> If still using named return variables, just "return".

Acknowledged.

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:285: buf := &bytes.Buffer{}
On 2015/08/14 21:01:35, dnj (Google) wrote:
> Might as well do this down near return statement, since that's the only time
you
> write to it.

Done.

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:357: } else if strings.HasPrefix(prop, "+") {
On 2015/08/14 21:01:35, dnj (Google) wrote:
> Why specifically "+"? Just catch a common user error? Maybe update the error
> message to, "datastore: cannot use +, but the default ordering is ascending".

this was copied from the sdk.

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:396: func() error {
On 2015/08/14 21:01:35, dnj (Google) wrote:
> Test for duplicate project clauses, or is that cool?

they're fine, they're just ignored. order doesn't matter for projected fields.

[dnj (Google), 2015-08-14 22:58:13]

lgtm

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
File impl/memory/datastore.go (right):

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore.go:73: if done || err != nil {
On 2015/08/14 22:25:47, iannucci wrote:
> On 2015/08/14 21:01:34, dnj (Google) wrote:
> > What's the value of "done"? From what I can see, you might as well just have
> > "valid" return nil if done.
> 
> it's in case the user specified a query with zero results. this code will
change
> when queries are fully implemented.

Acknowledged.

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1286943002/diff/40001/impl/memory/data...
impl/memory/datastore_query.go:261: func() (err error) {
On 2015/08/14 22:25:47, iannucci wrote:
> On 2015/08/14 21:01:35, dnj (Google) wrote:
> > No point in a named variable that you only use it once!
> 
> *shrug* changed

maruel@ cares :P

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

Committed patchset #6 (id:100001) manually as
9f570ae76af2ec00cbc915f8c2200d5339607273 (presubmit successful).