Merge "x/ref/services/syncbase: Update Glob implementation"
diff --git a/x/ref/services/syncbase/server/app.go b/x/ref/services/syncbase/server/app.go
index 957f4ed..d7103e9 100644
--- a/x/ref/services/syncbase/server/app.go
+++ b/x/ref/services/syncbase/server/app.go
@@ -15,6 +15,7 @@
"v.io/syncbase/x/ref/services/syncbase/server/util"
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/v23/context"
+ "v.io/v23/glob"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/verror"
@@ -82,17 +83,17 @@
return data.Perms, util.FormatVersion(data.Version), nil
}
-func (a *app) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
+func (a *app) GlobChildren__(ctx *context.T, call rpc.GlobChildrenServerCall, matcher *glob.Element) error {
if !a.exists {
- return nil, verror.New(verror.ErrNoExist, ctx, a.name)
+ return verror.New(verror.ErrNoExist, ctx, a.name)
}
// Check perms.
sn := a.s.st.NewSnapshot()
if err := util.GetWithAuth(ctx, call, sn, a.stKey(), &appData{}); err != nil {
sn.Abort()
- return nil, err
+ return err
}
- return util.Glob(ctx, call, "*", sn, sn.Abort, util.JoinKeyParts(util.DbInfoPrefix, a.name))
+ return util.Glob(ctx, call, matcher, sn, sn.Abort, util.JoinKeyParts(util.DbInfoPrefix, a.name))
}
////////////////////////////////////////
diff --git a/x/ref/services/syncbase/server/nosql/database.go b/x/ref/services/syncbase/server/nosql/database.go
index d4875c0..7b9a470 100644
--- a/x/ref/services/syncbase/server/nosql/database.go
+++ b/x/ref/services/syncbase/server/nosql/database.go
@@ -22,6 +22,7 @@
"v.io/syncbase/x/ref/services/syncbase/server/watchable"
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/v23/context"
+ "v.io/v23/glob"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/services/watch"
@@ -339,20 +340,20 @@
return nil, verror.NewErrNotImplemented(ctx)
}
-func (d *databaseReq) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
+func (d *databaseReq) GlobChildren__(ctx *context.T, call rpc.GlobChildrenServerCall, matcher *glob.Element) error {
if !d.exists {
- return nil, verror.New(verror.ErrNoExist, ctx, d.name)
+ return verror.New(verror.ErrNoExist, ctx, d.name)
}
if d.batchId != nil {
- return nil, wire.NewErrBoundToBatch(ctx)
+ return wire.NewErrBoundToBatch(ctx)
}
// Check perms.
sn := d.st.NewSnapshot()
if err := util.GetWithAuth(ctx, call, sn, d.stKey(), &databaseData{}); err != nil {
sn.Abort()
- return nil, err
+ return err
}
- return util.Glob(ctx, call, "*", sn, sn.Abort, util.TablePrefix)
+ return util.Glob(ctx, call, matcher, sn, sn.Abort, util.TablePrefix)
}
////////////////////////////////////////
diff --git a/x/ref/services/syncbase/server/nosql/table.go b/x/ref/services/syncbase/server/nosql/table.go
index 558a1c1..a3ed4b7 100644
--- a/x/ref/services/syncbase/server/nosql/table.go
+++ b/x/ref/services/syncbase/server/nosql/table.go
@@ -11,6 +11,7 @@
"v.io/syncbase/x/ref/services/syncbase/server/util"
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/v23/context"
+ "v.io/v23/glob"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/verror"
@@ -312,15 +313,15 @@
}
}
-func (t *tableReq) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
- impl := func(sntx store.SnapshotOrTransaction, closeSntx func() error) (<-chan string, error) {
+func (t *tableReq) GlobChildren__(ctx *context.T, call rpc.GlobChildrenServerCall, matcher *glob.Element) error {
+ impl := func(sntx store.SnapshotOrTransaction, closeSntx func() error) error {
// Check perms.
if err := t.checkAccess(ctx, call, sntx, ""); err != nil {
closeSntx()
- return nil, err
+ return err
}
// TODO(rogulenko): Check prefix permissions for children.
- return util.Glob(ctx, call, "*", sntx, closeSntx, util.JoinKeyParts(util.RowPrefix, t.name))
+ return util.Glob(ctx, call, matcher, sntx, closeSntx, util.JoinKeyParts(util.RowPrefix, t.name))
}
if t.d.batchId != nil {
return impl(t.d.batchReader(), func() error {
diff --git a/x/ref/services/syncbase/server/service.go b/x/ref/services/syncbase/server/service.go
index 5b87252..bafa388 100644
--- a/x/ref/services/syncbase/server/service.go
+++ b/x/ref/services/syncbase/server/service.go
@@ -19,6 +19,7 @@
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/syncbase/x/ref/services/syncbase/vsync"
"v.io/v23/context"
+ "v.io/v23/glob"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/verror"
@@ -160,14 +161,14 @@
return data.Perms, util.FormatVersion(data.Version), nil
}
-func (s *service) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
+func (s *service) GlobChildren__(ctx *context.T, call rpc.GlobChildrenServerCall, matcher *glob.Element) error {
// Check perms.
sn := s.st.NewSnapshot()
if err := util.GetWithAuth(ctx, call, sn, s.stKey(), &serviceData{}); err != nil {
sn.Abort()
- return nil, err
+ return err
}
- return util.Glob(ctx, call, "*", sn, sn.Abort, util.AppPrefix)
+ return util.Glob(ctx, call, matcher, sn, sn.Abort, util.AppPrefix)
}
////////////////////////////////////////
diff --git a/x/ref/services/syncbase/server/util/glob.go b/x/ref/services/syncbase/server/util/glob.go
index 2cbc3f9..8cf15a5 100644
--- a/x/ref/services/syncbase/server/util/glob.go
+++ b/x/ref/services/syncbase/server/util/glob.go
@@ -5,91 +5,36 @@
package util
import (
- "strings"
-
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/v23/context"
+ "v.io/v23/glob"
+ "v.io/v23/naming"
"v.io/v23/rpc"
- "v.io/v23/verror"
"v.io/x/lib/vlog"
- "v.io/x/ref/lib/glob"
)
// NOTE(nlacasse): Syncbase handles Glob requests by implementing
// GlobChildren__ at each level (service, app, database, table).
-//
-// Each GlobChildren__ method returns all the children of the name without
-// filtering them in any way. Any filtering that needs to happen based on the
-// glob pattern happens magically at a higher level. This results in a simple
-// GlobChildren__ implementation, but often an inefficient one, since we must
-// return every single child name, even though many will be filtered before
-// being sent to the client.
-//
-// We should consider changing GlobChildren__ so that it takes the pattern that
-// is being Globbed as an argument. Then, GlobChildren__ could be smarter about
-// what it returns, and only return names that already match the pattern. This
-// would prevent us from having to read all child names from the database every
-// time we Glob.
-
-// globPatternToPrefix takes "foo*" and returns "foo".
-// It assumes the input pattern is a valid glob pattern, and returns
-// verror.ErrBadArg if the input is not a valid prefix.
-func globPatternToPrefix(pattern string) (string, error) {
- if pattern == "" {
- return "", verror.NewErrBadArg(nil)
- }
- if pattern[len(pattern)-1] != '*' {
- return "", verror.NewErrBadArg(nil)
- }
- res := pattern[:len(pattern)-1]
- // Disallow chars and char sequences that have special meaning in glob, since
- // our Glob() does not support these.
- if strings.ContainsAny(res, "/*?[") {
- return "", verror.NewErrBadArg(nil)
- }
- if strings.Contains(res, "...") {
- return "", verror.NewErrBadArg(nil)
- }
- return res, nil
-}
// Glob performs a glob. It calls closeSntx to close sntx.
-// TODO(sadovsky): Why do we make developers implement Glob differently from
-// other streaming RPCs? It's confusing that Glob must return immediately and
-// write its results to a channel, while other streaming RPC handlers must block
-// and write their results to the output stream. See nlacasse's TODO below, too.
-func Glob(ctx *context.T, call rpc.ServerCall, pattern string, sntx store.SnapshotOrTransaction, closeSntx func() error, stKeyPrefix string) (<-chan string, error) {
- // TODO(sadovsky): Support glob with non-prefix pattern.
- if _, err := glob.Parse(pattern); err != nil {
- closeSntx()
- return nil, verror.New(verror.ErrBadArg, ctx, err)
- }
- prefix, err := globPatternToPrefix(pattern)
- if err != nil {
- closeSntx()
- if verror.ErrorID(err) == verror.ErrBadArg.ID {
- return nil, verror.NewErrNotImplemented(ctx)
- }
- return nil, verror.New(verror.ErrInternal, ctx, err)
- }
+func Glob(ctx *context.T, call rpc.GlobChildrenServerCall, matcher *glob.Element, sntx store.SnapshotOrTransaction, closeSntx func() error, stKeyPrefix string) error {
+ prefix, _ := matcher.FixedPrefix()
it := sntx.Scan(ScanPrefixArgs(stKeyPrefix, prefix))
- ch := make(chan string)
- go func() {
- defer closeSntx()
- defer close(ch)
- key := []byte{}
- for it.Advance() {
- key = it.Key(key)
- parts := SplitKeyParts(string(key))
- ch <- parts[len(parts)-1]
+ defer closeSntx()
+ key := []byte{}
+ for it.Advance() {
+ key = it.Key(key)
+ parts := SplitKeyParts(string(key))
+ name := parts[len(parts)-1]
+ if matcher.Match(name) {
+ if err := call.SendStream().Send(naming.GlobChildrenReplyName{name}); err != nil {
+ return err
+ }
}
- if err := it.Err(); err != nil {
- // TODO(nlacasse): We should do something with the error other than
- // just logging. Ideally we could send the error back to the
- // client, but the GlobChildren__ API doesn't really allow for that
- // since it uses a channel of strings rather than GlobResults.
- vlog.VI(1).Infof("Glob(%q) failed: %v", pattern, err)
- }
- }()
- return ch, nil
+ }
+ if err := it.Err(); err != nil {
+ vlog.VI(1).Infof("Glob() failed: %v", err)
+ call.SendStream().Send(naming.GlobChildrenReplyError{naming.GlobError{Error: err}})
+ }
+ return nil
}
diff --git a/x/ref/services/syncbase/server/util/glob_test.go b/x/ref/services/syncbase/server/util/glob_test.go
deleted file mode 100644
index d9c8b71..0000000
--- a/x/ref/services/syncbase/server/util/glob_test.go
+++ /dev/null
@@ -1,54 +0,0 @@
-// Copyright 2015 The Vanadium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-// Note, we use package util rather than util_test so that we can access the
-// unexported name util.globPatternToPrefix.
-
-package util
-
-import (
- "testing"
-
- "v.io/v23/verror"
-)
-
-var (
- errBadArg = string(verror.ErrBadArg.ID)
-)
-
-func TestGlobPatternToPrefix(t *testing.T) {
- tests := []struct {
- pattern, prefix string
- }{
- {"", errBadArg},
- {"*", ""},
- {"foo", errBadArg},
- {"foo*", "foo"},
- {"foo/", errBadArg},
- {"foo/*", errBadArg},
- {"foo/bar", errBadArg},
- {"foo/bar*", errBadArg},
- {".*", "."},
- {"..*", ".."},
- {"...*", errBadArg},
- {"...foo*", errBadArg},
- {"foo...foo*", errBadArg},
- {"..foo*", "..foo"},
- {"foo..foo*", "foo..foo"},
- {"...", errBadArg},
- {"*/...", errBadArg},
- {"foo/...", errBadArg},
- {"/", errBadArg},
- {"/*", errBadArg},
- }
- for _, test := range tests {
- prefix, err := globPatternToPrefix(test.pattern)
- if err != nil {
- prefix = string(verror.ErrorID(err))
- }
- if prefix != test.prefix {
- t.Errorf("%q: got %q, want %q", test.pattern, prefix, test.prefix)
- }
- }
-}
