Annotate ErrInvalidKey.
The ErrInvalidKey error is annoying when encountered, becuase it
provides no context about why the key is invalid and can occur in some
obscure places (checkfilter, query finalization, implementations, etc.).
To address this, replace ErrInvalidKey with a test function,
IsErrInvalidKey. This will return true for any invalid key errors.
Replace instances of ErrInvalidKey with errors derived from
MakeErrInvalidKey, annotated with some context and useful information.
This should be relatively minor change upstream, since no audited code
actually checks for this error case.
BUG=None
TEST=unit
Review-Url: https://codereview.chromium.org/2620043002
diff --git a/impl/cloud/datastore.go b/impl/cloud/datastore.go
index ca7085f..d0d1dc6 100644
--- a/impl/cloud/datastore.go
+++ b/impl/cloud/datastore.go
@@ -583,7 +583,7 @@
case datastore.ErrConcurrentTransaction:
return ds.ErrConcurrentTransaction
case datastore.ErrInvalidKey:
- return ds.ErrInvalidKey
+ return ds.MakeErrInvalidKey().Err()
default:
return err
}
diff --git a/impl/memory/datastore_test.go b/impl/memory/datastore_test.go
index 0d884a9..c309e10 100644
--- a/impl/memory/datastore_test.go
+++ b/impl/memory/datastore_test.go
@@ -104,7 +104,7 @@
})
})
Convey("Deleteing with a bogus key is bad", func() {
- So(ds.Delete(c, ds.NewKey(c, "Foo", "wat", 100, nil)), ShouldEqual, ds.ErrInvalidKey)
+ So(ds.IsErrInvalidKey(ds.Delete(c, ds.NewKey(c, "Foo", "wat", 100, nil))), ShouldBeTrue)
})
Convey("Deleteing a DNE entity is fine", func() {
So(ds.Delete(c, ds.NewKey(c, "Foo", "wat", 0, nil)), ShouldBeNil)
diff --git a/service/datastore/checkfilter.go b/service/datastore/checkfilter.go
index 04945c0..452d770 100644
--- a/service/datastore/checkfilter.go
+++ b/service/datastore/checkfilter.go
@@ -45,8 +45,17 @@
}
lme := errors.NewLazyMultiError(len(keys))
for i, k := range keys {
- if k.IsIncomplete() || !k.Valid(true, tcf.kc) {
- lme.Assign(i, ErrInvalidKey)
+ var err error
+ switch {
+ case k.IsIncomplete():
+ err = MakeErrInvalidKey().Reason("key [%(key)s] is incomplete").
+ D("key", k).Err()
+ case !k.Valid(true, tcf.kc):
+ err = MakeErrInvalidKey().Reason("key [%(key)s] is not valid in context %(context)s").
+ D("key", k).D("context", tcf.kc).Err()
+ }
+ if err != nil {
+ lme.Assign(i, err)
}
}
if me := lme.Get(); me != nil {
@@ -71,7 +80,8 @@
lme := errors.NewLazyMultiError(len(keys))
for i, k := range keys {
if !k.PartialValid(tcf.kc) {
- lme.Assign(i, ErrInvalidKey)
+ lme.Assign(i, MakeErrInvalidKey().Reason("key [%(key)s] is not partially valid in context %(context)s").
+ D("key", k).D("context", tcf.kc).Err())
continue
}
v := vals[i]
@@ -98,8 +108,17 @@
}
lme := errors.NewLazyMultiError(len(keys))
for i, k := range keys {
- if k.IsIncomplete() || !k.Valid(false, tcf.kc) {
- lme.Assign(i, ErrInvalidKey)
+ var err error
+ switch {
+ case k.IsIncomplete():
+ err = MakeErrInvalidKey().Reason("key [%(key)s] is incomplete").
+ D("key", k).Err()
+ case !k.Valid(false, tcf.kc):
+ err = MakeErrInvalidKey().Reason("key [%(key)s] is not valid in context %(context)s").
+ D("key", k).D("context", tcf.kc).Err()
+ }
+ if err != nil {
+ lme.Assign(i, err)
}
}
if me := lme.Get(); me != nil {
diff --git a/service/datastore/checkfilter_test.go b/service/datastore/checkfilter_test.go
index 56382e4..d70ba54 100644
--- a/service/datastore/checkfilter_test.go
+++ b/service/datastore/checkfilter_test.go
@@ -67,7 +67,7 @@
keys := []*Key{MkKeyContext("wut", "wrong").MakeKey("Kind", 1)}
So(rds.GetMulti(keys, nil, func(pm PropertyMap, err error) error {
So(pm, ShouldBeNil)
- So(err, ShouldEqual, ErrInvalidKey)
+ So(IsErrInvalidKey(err), ShouldBeTrue)
return nil
}), ShouldBeNil)
@@ -94,7 +94,7 @@
So(rds.PutMulti(keys, vals, func(k *Key, err error) error {
So(k, ShouldBeNil)
- So(err, ShouldEqual, ErrInvalidKey)
+ So(IsErrInvalidKey(err), ShouldBeTrue)
return nil
}), ShouldBeNil)
@@ -121,7 +121,7 @@
So(rds.DeleteMulti(nil, nil), ShouldBeNil)
So(rds.DeleteMulti([]*Key{mkKey("", "", "", "")}, nil).Error(), ShouldContainSubstring, "is nil")
So(rds.DeleteMulti([]*Key{mkKey("", "", "", "")}, func(err error) error {
- So(err, ShouldEqual, ErrInvalidKey)
+ So(IsErrInvalidKey(err), ShouldBeTrue)
return nil
}), ShouldBeNil)
diff --git a/service/datastore/datastore_test.go b/service/datastore/datastore_test.go
index 9edf17d..a97902b 100644
--- a/service/datastore/datastore_test.go
+++ b/service/datastore/datastore_test.go
@@ -612,7 +612,7 @@
}
// having an Incomplete parent makes an invalid key
bp := &BadParent{ID: 1, Parent: MakeKey(c, "Something", 0)}
- So(Put(c, bp), ShouldErrLike, ErrInvalidKey)
+ So(IsErrInvalidKey(Put(c, bp)), ShouldBeTrue)
})
Convey("vararg with errors", func() {
diff --git a/service/datastore/errors.go b/service/datastore/errors.go
index 60c1196..65e35f7 100644
--- a/service/datastore/errors.go
+++ b/service/datastore/errors.go
@@ -9,12 +9,14 @@
"reflect"
"github.com/luci/gae"
+
+ "github.com/luci/luci-go/common/errors"
+
"google.golang.org/appengine/datastore"
)
// These errors are returned by various datastore.Interface methods.
var (
- ErrInvalidKey = datastore.ErrInvalidKey
ErrNoSuchEntity = datastore.ErrNoSuchEntity
ErrConcurrentTransaction = datastore.ErrConcurrentTransaction
@@ -22,6 +24,17 @@
Stop = gae.Stop
)
+// MakeErrInvalidKey returns an errors.Annotator instance that wraps an invalid
+// key error. Calling IsErrInvalidKey on this Annotator or its derivatives will
+// return true.
+func MakeErrInvalidKey() *errors.Annotator {
+ return errors.Annotate(datastore.ErrInvalidKey)
+}
+
+// IsErrInvalidKey tests if a given error is a wrapped datastore.ErrInvalidKey
+// error.
+func IsErrInvalidKey(err error) bool { return errors.Unwrap(err) == datastore.ErrInvalidKey }
+
// ErrFieldMismatch is returned when a field is to be loaded into a different
// type than the one it was stored from, or when a field is missing or
// unexported in the destination struct.
diff --git a/service/datastore/finalized_query.go b/service/datastore/finalized_query.go
index 0f9d755..1b7e598 100644
--- a/service/datastore/finalized_query.go
+++ b/service/datastore/finalized_query.go
@@ -323,16 +323,30 @@
// have values of type PTKey (but don't filter on the magic '__key__' field).
func (q *FinalizedQuery) Valid(kc KeyContext) error {
anc := q.Ancestor()
- if anc != nil && (!anc.Valid(false, kc) || anc.IsIncomplete()) {
- return ErrInvalidKey
+ if anc != nil {
+ switch {
+ case !anc.Valid(false, kc):
+ return MakeErrInvalidKey().Reason("ancestor [%(key)s] is not valid in context %(context)s").
+ D("key", anc).D("context", kc).Err()
+ case anc.IsIncomplete():
+ return MakeErrInvalidKey().Reason("ancestor [%(key)s] is incomplete").D("key", anc).Err()
+ }
}
if q.ineqFiltProp == "__key__" {
- if q.ineqFiltLowSet && !q.ineqFiltLow.Value().(*Key).Valid(false, kc) {
- return ErrInvalidKey
+ if q.ineqFiltLowSet {
+ if k := q.ineqFiltLow.Value().(*Key); !k.Valid(false, kc) {
+ return MakeErrInvalidKey().
+ Reason("low inequality filter key [%(key)s] is not valid in context %(context)s").
+ D("key", k).D("context", kc).Err()
+ }
}
- if q.ineqFiltHighSet && !q.ineqFiltHigh.Value().(*Key).Valid(false, kc) {
- return ErrInvalidKey
+ if q.ineqFiltHighSet {
+ if k := q.ineqFiltHigh.Value().(*Key); !k.Valid(false, kc) {
+ return MakeErrInvalidKey().
+ Reason("high inequality filter key [%(key)s] is not valid in context %(context)s").
+ D("key", k).D("context", kc).Err()
+ }
}
}
return nil
diff --git a/service/datastore/interface.go b/service/datastore/interface.go
index 7b93698..a8933dc 100644
--- a/service/datastore/interface.go
+++ b/service/datastore/interface.go
@@ -124,7 +124,7 @@
}
if !mat.setKey(v, key) {
- return ErrInvalidKey
+ return MakeErrInvalidKey().Reason("failed to export key [%(key)s]").D("key", key).Err()
}
return nil
})
diff --git a/service/datastore/query_test.go b/service/datastore/query_test.go
index 842eff1..1a3e53b 100644
--- a/service/datastore/query_test.go
+++ b/service/datastore/query_test.go
@@ -9,6 +9,7 @@
"testing"
"github.com/luci/luci-go/common/sync/parallel"
+
. "github.com/luci/luci-go/common/testing/assertions"
. "github.com/smartystreets/goconvey/convey"
)
@@ -61,8 +62,9 @@
// gql is the expected generated GQL.
gql string
- // err is the error to expect after prepping the query (error, string or nil)
- err interface{}
+ // assertion is the error to expect after prepping the query, or nil if the
+ // error should be nil.
+ assertion func(err error)
// equivalentQuery is another query which ShouldResemble q. This is useful to
// see the effects of redundancy pruning on e.g. filters.
@@ -85,21 +87,32 @@
return MkKeyContext("s~aid", "ns").MakeKey(elems...)
}
+func errString(v string) func(error) {
+ return func(err error) {
+ So(err, ShouldErrLike, v)
+ }
+}
+
+func shouldBeErrInvalidKey(err error) {
+ So(IsErrInvalidKey(err), ShouldBeTrue)
+}
+
var queryTests = []queryTest{
{"only one inequality",
nq().Order("bob", "wat").Gt("bob", 10).Lt("wat", 29),
"",
- "inequality filters on multiple properties", nil},
+ errString("inequality filters on multiple properties"),
+ nil},
{"bad order",
nq().Order("+Bob"),
"",
- "invalid order", nil},
+ errString("invalid order"), nil},
{"empty order",
nq().Order(""),
"",
- "empty order", nil},
+ errString("empty order"), nil},
{"negative offset disables Offset",
nq().Offset(100).Offset(-20),
@@ -109,22 +122,22 @@
{"projecting a keys-only query",
nq().Project("hello").KeysOnly(true),
"",
- "cannot project a keysOnly query", nil},
+ errString("cannot project a keysOnly query"), nil},
{"projecting a keys-only query (reverse)",
nq().KeysOnly(true).Project("hello"),
"",
- "cannot project a keysOnly query", nil},
+ errString("cannot project a keysOnly query"), nil},
{"projecting an empty field",
nq().Project("hello", ""),
"",
- "cannot filter/project on: \"\"", nil},
+ errString("cannot filter/project on: \"\""), nil},
{"projecting __key__",
nq().Project("hello", "__key__"),
"",
- "cannot project on \"__key__\"", nil},
+ errString("cannot project on \"__key__\""), nil},
{"getting all the keys",
nq("").KeysOnly(true),
@@ -154,7 +167,7 @@
{"bad ancestors",
nq().Ancestor(mkKey("goop", 0)),
"",
- ErrInvalidKey, nil},
+ shouldBeErrInvalidKey, nil},
{"nil ancestors",
nq().Ancestor(nil),
@@ -164,57 +177,57 @@
{"Bad key filters",
nq().Gt("__key__", mkKey("goop", 0)),
"",
- ErrInvalidKey, nil},
+ shouldBeErrInvalidKey, nil},
{"filters for __key__ that aren't keys",
nq().Gt("__key__", 10),
"",
- "filters on \"__key__\" must have type *Key", nil},
+ errString("filters on \"__key__\" must have type *Key"), nil},
{"multiple inequalities",
nq().Gt("bob", 19).Lt("charlie", 20),
"",
- "inequality filters on multiple properties", nil},
+ errString("inequality filters on multiple properties"), nil},
{"inequality must be first sort order",
nq().Gt("bob", 19).Order("-charlie"),
"",
- "first sort order", nil},
+ errString("first sort order"), nil},
{"inequality must be first sort order (reverse)",
nq().Order("-charlie").Gt("bob", 19),
"",
- "first sort order", nil},
+ errString("first sort order"), nil},
{"equality filter projected field",
nq().Project("foo").Eq("foo", 10),
"",
- "cannot project", nil},
+ errString("cannot project"), nil},
{"equality filter projected field (reverse)",
nq().Eq("foo", 10).Project("foo"),
"",
- "cannot project", nil},
+ errString("cannot project"), nil},
{"kindless with non-__key__ filters",
nq("").Lt("face", 25.3),
"",
- "kindless queries can only filter on __key__", nil},
+ errString("kindless queries can only filter on __key__"), nil},
{"kindless with non-__key__ orders",
nq("").Order("face"),
"",
- "invalid order for kindless query", nil},
+ errString("invalid order for kindless query"), nil},
{"kindless with descending-__key__ order",
nq("").Order("-__key__"),
"",
- "invalid order for kindless query", nil},
+ errString("invalid order for kindless query"), nil},
{"kindless with equality filters",
nq("").Eq("hello", 1),
"",
- "may not have any equality", nil},
+ errString("may not have any equality"), nil},
{"kindless with ancestor filter",
nq("").Ancestor(mkKey("Parent", 1)),
@@ -234,7 +247,7 @@
{"chained errors return the first",
nq().Eq("__reserved__", 100).Eq("hello", "wurld").Order(""),
"",
- "__reserved__", nil},
+ errString("__reserved__"), nil},
{"multiple ancestors",
nq().Ancestor(mkKey("something", "correct")).Ancestor(mkKey("something", "else")),
@@ -246,7 +259,7 @@
{"filter with illegal type",
nq().Eq("something", complex(1, 2)),
"",
- "bad type complex", nil},
+ errString("bad type complex"), nil},
{"sort orders used for equality are ignored",
nq().Order("a", "b", "c").Eq("b", 2, 2),
@@ -274,7 +287,7 @@
{"Filtering on a reserved property is forbidden",
nq().Gte("__special__", 10),
"",
- "cannot filter/project on reserved property: \"__special__\"",
+ errString("cannot filter/project on reserved property: \"__special__\""),
nil},
{"in-bound key filters with ancestor OK",
@@ -300,17 +313,20 @@
{"ineq on __key__ with ancestor must be an ancestor of __ancestor__!",
nq().Ancestor(mkKey("Hello", 10)).Lt("__key__", mkKey("Hello", 8)),
"",
- "inequality filters on __key__ must be descendants of the __ancestor__", nil},
+ errString("inequality filters on __key__ must be descendants of the __ancestor__"),
+ nil},
{"ineq on __key__ with ancestor must be an ancestor of __ancestor__! (2)",
nq().Ancestor(mkKey("Hello", 10)).Gt("__key__", mkKey("Hello", 8)),
"",
- "inequality filters on __key__ must be descendants of the __ancestor__", nil},
+ errString("inequality filters on __key__ must be descendants of the __ancestor__"),
+ nil},
{"can build an empty query",
nq().Lt("hello", 10).Gt("hello", 50),
"",
- ErrNullQuery, nil},
+ func(err error) { So(err, ShouldEqual, ErrNullQuery) },
+ nil},
}
func TestQueries(t *testing.T) {
@@ -323,7 +339,11 @@
if err == nil {
err = fq.Valid(MkKeyContext("s~aid", "ns"))
}
- So(err, ShouldErrLike, tc.err)
+ if tc.assertion != nil {
+ tc.assertion(err)
+ } else {
+ So(err, ShouldBeNil)
+ }
if tc.gql != "" {
So(fq.GQL(), ShouldEqual, tc.gql)
