veyron/services/store: Objects can no longer be accessed by id.
Objects can no longer be accessed via the path "/uid/<id>".
"/uid" is not a special name, and can be used as the store
admin pleases.
Change-Id: I28aef1f788e98586b97b23955942ccd4d8e99a02
diff --git a/runtimes/google/vsync/watcher_test.go b/runtimes/google/vsync/watcher_test.go
index b9d8a79..7804409 100644
--- a/runtimes/google/vsync/watcher_test.go
+++ b/runtimes/google/vsync/watcher_test.go
@@ -149,7 +149,7 @@
batch.Changes = []watch.Change{
// 1st transaction: create "/" and "/a" and "/a/b" as 3 new objects (prior versions are 0).
watch.Change{
- Name: "/uid/4c6db51aa740d8c62b90df874503e285",
+ Name: "",
State: 0,
Value: &raw.Mutation{
ID: storage.ID{0x4c, 0x6d, 0xb5, 0x1a, 0xa7, 0x40, 0xd8, 0xc6,
@@ -170,7 +170,7 @@
Continued: true,
},
watch.Change{
- Name: "/uid/082bc42e15af4fcf611d7f19a8d7831f",
+ Name: "",
State: 0,
Value: &raw.Mutation{
ID: storage.ID{0x8, 0x2b, 0xc4, 0x2e, 0x15, 0xaf, 0x4f, 0xcf,
@@ -191,7 +191,7 @@
Continued: true,
},
watch.Change{
- Name: "/uid/6e4a327c297d76fb5142b1b1d95b2d07",
+ Name: "",
State: 0,
Value: &raw.Mutation{
ID: storage.ID{0x6e, 0x4a, 0x32, 0x7c, 0x29, 0x7d, 0x76, 0xfb,
@@ -208,7 +208,7 @@
// 2nd transaction: create "/a/c" as a new object, which also updates "a" (its "Dir" field).
watch.Change{
- Name: "/uid/082bc42e15af4fcf611d7f19a8d7831f",
+ Name: "",
State: 0,
Value: &raw.Mutation{
ID: storage.ID{0x8, 0x2b, 0xc4, 0x2e, 0x15, 0xaf, 0x4f, 0xcf,
@@ -234,7 +234,7 @@
Continued: true,
},
watch.Change{
- Name: "/uid/70ff65ec0f825f44b69f895eea759d71",
+ Name: "",
State: 0,
Value: &raw.Mutation{
ID: storage.ID{0x70, 0xff, 0x65, 0xec, 0xf, 0x82, 0x5f, 0x44,
diff --git a/services/store/memstore/blackbox/graph_test.go b/services/store/memstore/blackbox/graph_test.go
index 6f2770e..6afbb4b 100644
--- a/services/store/memstore/blackbox/graph_test.go
+++ b/services/store/memstore/blackbox/graph_test.go
@@ -1,7 +1,6 @@
package blackbox
import (
- "fmt"
"testing"
_ "veyron/lib/testutil"
@@ -54,8 +53,10 @@
// Verify that all the nodes still exist.
st.GC()
- for _, id := range ids {
- ExpectExists(t, st, id)
+ for i, node := range nodes {
+ path := node.Label
+ id := ids[i]
+ ExpectExists(t, st, path, id)
}
// Truncate the graph to length 3.
@@ -68,11 +69,13 @@
}
st.GC()
- for i, id := range ids {
+ for i, node := range nodes {
+ path := node.Label
+ id := ids[i]
if i < 3 {
- ExpectExists(t, st, id)
+ ExpectExists(t, st, path, id)
} else {
- ExpectNotExists(t, st, id)
+ ExpectNotExists(t, st, path, id)
}
}
}
@@ -97,17 +100,18 @@
}
id := stat.ID
- // Create a linked list using /uid/xxx names.
- tr := memstore.NewTransaction()
+ // Create a linked list.
+ path := ""
var nodes [linearNodeCount]*Node
var ids [linearNodeCount]storage.ID
+ tr := memstore.NewTransaction()
for i := 0; i != linearNodeCount; i++ {
- node := &Node{Label: fmt.Sprintf("Node[%d]", i)}
+ path = path + "/Children/a"
+ node := &Node{Label: path}
nodes[i] = node
- name := fmt.Sprintf("/uid/%s/Children/a", id)
- stat, err := st.Bind(name).Put(rootPublicID, tr, node)
+ stat, err := st.Bind(path).Put(rootPublicID, tr, node)
if err != nil || stat == nil {
- t.Errorf("Unexpected error: %s: %s", name, err)
+ t.Errorf("Unexpected error: %s: %s", path, err)
}
id = stat.ID
ids[i] = id
@@ -116,9 +120,8 @@
// Add a back-loop.
{
node := nodes[linearNodeCount-1]
- id := ids[linearNodeCount-1]
node.Children = map[string]storage.ID{"a": ids[loopNodeIndex]}
- if _, err := st.Bind(fmt.Sprintf("/uid/%s", id)).Put(rootPublicID, tr, node); err != nil {
+ if _, err := st.Bind(node.Label).Put(rootPublicID, tr, node); err != nil {
t.Errorf("Unexpected error: %s", err)
}
}
@@ -126,26 +129,29 @@
// Verify that all the nodes still exist.
st.GC()
- for _, id := range ids {
- ExpectExists(t, st, id)
+ for i, node := range nodes {
+ path := node.Label
+ id := ids[i]
+ ExpectExists(t, st, path, id)
}
// Truncate part of the loop.
{
node := nodes[cutNodeIndex]
- id := ids[cutNodeIndex]
node.Children = nil
- if _, err := st.Bind(fmt.Sprintf("/uid/%s", id)).Put(rootPublicID, nil, node); err != nil {
+ if _, err := st.Bind(node.Label).Put(rootPublicID, nil, node); err != nil {
t.Errorf("Unexpected error: %s", err)
}
}
st.GC()
- for i, id := range ids {
+ for i, node := range nodes {
+ path := node.Label
+ id := ids[i]
if i <= cutNodeIndex {
- ExpectExists(t, st, id)
+ ExpectExists(t, st, path, id)
} else {
- ExpectNotExists(t, st, id)
+ ExpectNotExists(t, st, path, id)
}
}
}
diff --git a/services/store/memstore/blackbox/sync_integration_test.go b/services/store/memstore/blackbox/sync_integration_test.go
index 6e56574..06139f4 100644
--- a/services/store/memstore/blackbox/sync_integration_test.go
+++ b/services/store/memstore/blackbox/sync_integration_test.go
@@ -17,7 +17,7 @@
tr := memstore.NewTransaction()
id1 := Put(t, st, tr, "/", "val1")
id2 := Put(t, st, tr, "/a", "val2")
- Put(t, st, tr, "/a/b", "val3")
+ id3 := Put(t, st, tr, "/a/b", "val3")
Commit(t, tr)
// Remove /a/b
@@ -53,9 +53,10 @@
PutMutations(t, target, Mutations(cb.Changes))
GC(t, target)
- // Expect that the target contains id1 and id2
- ExpectExists(t, target, id1)
- ExpectExists(t, target, id2)
+ // Expect that the target contains id1 and id2 but not id3
+ ExpectExists(t, target, "/", id1)
+ ExpectExists(t, target, "/a", id2)
+ ExpectNotExists(t, target, "/a/b", id3)
}
func TestSyncTransaction(t *testing.T) {
@@ -87,9 +88,9 @@
GC(t, target)
// Expect that the target contains id1, id2, id3
- ExpectExists(t, target, id1)
- ExpectExists(t, target, id2)
- ExpectExists(t, target, id3)
+ ExpectExists(t, target, "/", id1)
+ ExpectExists(t, target, "/a", id2)
+ ExpectExists(t, target, "/a/b", id3)
// Next transaction, remove /a/b
tr = memstore.NewTransaction()
@@ -106,7 +107,7 @@
GC(t, target)
// Expect that the target contains id1, id2, but not id3
- ExpectExists(t, target, id1)
- ExpectExists(t, target, id2)
- ExpectNotExists(t, target, id3)
+ ExpectExists(t, target, "/", id1)
+ ExpectExists(t, target, "/a", id2)
+ ExpectNotExists(t, target, "/a/b", id3)
}
diff --git a/services/store/memstore/blackbox/util.go b/services/store/memstore/blackbox/util.go
index 569c504..d432ad5 100644
--- a/services/store/memstore/blackbox/util.go
+++ b/services/store/memstore/blackbox/util.go
@@ -1,7 +1,6 @@
package blackbox
import (
- "fmt"
"io/ioutil"
"os"
"runtime"
@@ -123,19 +122,22 @@
}
}
-func ExpectExists(t *testing.T, st *memstore.Store, id storage.ID) {
- _, err := st.Bind(fmt.Sprintf("/uid/%s", id)).Get(rootPublicID, nil)
+func ExpectExists(t *testing.T, st *memstore.Store, path string, id storage.ID) {
+ _, file, line, _ := runtime.Caller(1)
+ e, err := st.Bind(path).Get(rootPublicID, nil)
if err != nil {
- _, file, line, _ := runtime.Caller(1)
t.Errorf("%s(%d): Expected value for ID: %x", file, line, id)
}
+ if e.Stat.ID != id {
+ t.Errorf("%s(%d): expected id to be %v, but was %v", file, line, id, e.Stat.ID)
+ }
}
-func ExpectNotExists(t *testing.T, st *memstore.Store, id storage.ID) {
- x, err := st.Bind(fmt.Sprintf("/uid/%s", id)).Get(rootPublicID, nil)
+func ExpectNotExists(t *testing.T, st *memstore.Store, path string, id storage.ID) {
+ _, file, line, _ := runtime.Caller(1)
+ e, err := st.Bind(path).Get(rootPublicID, nil)
if err == nil {
- _, file, line, _ := runtime.Caller(1)
- t.Errorf("%s(%d): Unexpected value: %v", file, line, x)
+ t.Errorf("%s(%d): Unexpected value: %v", file, line, e)
}
}
diff --git a/services/store/memstore/state/mutable_snapshot.go b/services/store/memstore/state/mutable_snapshot.go
index c5a8b11..0845fcd 100644
--- a/services/store/memstore/state/mutable_snapshot.go
+++ b/services/store/memstore/state/mutable_snapshot.go
@@ -105,7 +105,6 @@
var (
errBadPath = verror.BadArgf("malformed path")
errBadRef = verror.BadArgf("value has dangling references")
- errCantUnlinkByID = verror.BadArgf("can't unlink entries by ID")
errDuplicatePutMutation = verror.BadArgf("duplicate calls to PutMutation for the same ID")
errNotFound = verror.NotFoundf("not found")
errNotTagList = verror.BadArgf("not a TagList")
@@ -322,27 +321,6 @@
return &cp, nil
}
-// resolveCell performs a path-based lookup, traversing the state from the
-// root.
-//
-// Returns (cell, suffix, v), where cell contains the value, suffix is the path
-// to the value, v is the value itself. If the operation failed, the returned
-// cell is nil.
-func (sn *MutableSnapshot) resolveCell(checker *acl.Checker, path storage.PathName, mu *Mutations) (*Cell, storage.PathName, interface{}) {
- // Get the starting object.
- id, suffix, ok := path.GetID()
- if ok {
- path = suffix
- // Remove garbage so that only valid uids are visible.
- sn.gc()
- checker.Update(uidTagList)
- } else {
- id = sn.rootID
- }
-
- return sn.resolve(checker, id, path, mu)
-}
-
// Get returns the value for a path.
func (sn *MutableSnapshot) Get(pid security.PublicID, path storage.PathName) (*storage.Entry, error) {
checker := sn.newPermChecker(pid)
@@ -376,9 +354,6 @@
if path.IsRoot() {
return sn.putRoot(checker, v)
}
- if id, suffix, ok := path.GetID(); ok && len(suffix) == 0 {
- return sn.putValueByID(checker, id, v)
- }
return sn.putValue(checker, path, v)
}
@@ -535,17 +510,6 @@
return ncell, nil
}
-// putValueByID replaces a value referred to by ID.
-func (sn *MutableSnapshot) putValueByID(checker *acl.Checker, id storage.ID, v interface{}) (*Cell, error) {
- checker.Update(uidTagList)
- if !checker.IsAllowed(security.WriteLabel) {
- return nil, errPermissionDenied
- }
-
- sn.gc()
- return sn.add(checker, id, v)
-}
-
// Remove removes a value.
func (sn *MutableSnapshot) Remove(pid security.PublicID, path storage.PathName) error {
checker := sn.newPermChecker(pid)
@@ -559,9 +523,6 @@
sn.mutations.SetRootID = true
return nil
}
- if path.IsStrictID() {
- return errCantUnlinkByID
- }
// Split the names into directory and field parts.
cell, suffix, _ := sn.resolveCell(checker, path[:len(path)-1], sn.mutations)
diff --git a/services/store/memstore/state/mutable_snapshot_test.go b/services/store/memstore/state/mutable_snapshot_test.go
index 1930ff1..c65794c 100644
--- a/services/store/memstore/state/mutable_snapshot_test.go
+++ b/services/store/memstore/state/mutable_snapshot_test.go
@@ -1,7 +1,6 @@
package state
import (
- "fmt"
"runtime"
"testing"
@@ -48,13 +47,17 @@
return stat.ID, dir
}
-func expectExists(t *testing.T, sn *MutableSnapshot, id storage.ID) {
+func expectExists(t *testing.T, sn *MutableSnapshot, path string, id storage.ID) {
_, file, line, _ := runtime.Caller(1)
if !sn.idTable.Contains(&Cell{ID: id}) {
t.Errorf("%s(%d): does not exist: %s", file, line, id)
}
- if _, err := sn.Get(rootPublicID, storage.ParsePath(fmt.Sprintf("/uid/%s", id))); err != nil {
- t.Errorf("%s(%d): does not exist: %s", file, line, id)
+ e, err := sn.Get(rootPublicID, storage.ParsePath(path))
+ if err != nil {
+ t.Errorf("%s(%d): does not exist: %s", file, line, path)
+ }
+ if e.Stat.ID != id {
+ t.Errorf("%s(%d): expected id to be %v, but was %v", file, line, id, e.Stat.ID)
}
}
@@ -158,11 +161,11 @@
id3, d3 := mkdir(t, sn, "/Entries/a/Entries/b")
id4, d4 := mkdir(t, sn, "/Entries/a/Entries/b/Entries/c")
id5, d5 := mkdir(t, sn, "/Entries/a/Entries/b/Entries/d")
- expectExists(t, sn, id1)
- expectExists(t, sn, id2)
- expectExists(t, sn, id3)
- expectExists(t, sn, id4)
- expectExists(t, sn, id5)
+ expectExists(t, sn, "/", id1)
+ expectExists(t, sn, "/Entries/a", id2)
+ expectExists(t, sn, "/Entries/a/Entries/b", id3)
+ expectExists(t, sn, "/Entries/a/Entries/b/Entries/c", id4)
+ expectExists(t, sn, "/Entries/a/Entries/b/Entries/d", id5)
// Parent directory has to exist.
d := &Dir{}
@@ -182,8 +185,8 @@
t.Errorf("Unexpected error: %s", err)
}
sn.gc()
- expectExists(t, sn, id1)
- expectExists(t, sn, id2)
+ expectExists(t, sn, "/", id1)
+ expectExists(t, sn, "/Entries/a", id2)
expectNotExists(t, sn, id3)
expectNotExists(t, sn, id4)
expectNotExists(t, sn, id5)
@@ -209,7 +212,7 @@
if err != nil {
t.Errorf("Unexpected error: %s", err)
}
- expectExists(t, sn, stat.ID)
+ expectExists(t, sn, "/Entries/a", stat.ID)
checkInRefs(t, sn)
// Change the ref to refer to itself.
@@ -218,7 +221,7 @@
t.Errorf("Unexpected error: %s", err)
}
sn.gc()
- expectExists(t, sn, stat.ID)
+ expectExists(t, sn, "/Entries/a", stat.ID)
checkInRefs(t, sn)
// Remove it.
@@ -239,11 +242,11 @@
id3, d3 := mkdir(t, sn, "/a/b")
id4, d4 := mkdir(t, sn, "/a/b/c")
id5, d5 := mkdir(t, sn, "/a/b/c/d")
- expectExists(t, sn, id1)
- expectExists(t, sn, id2)
- expectExists(t, sn, id3)
- expectExists(t, sn, id4)
- expectExists(t, sn, id5)
+ expectExists(t, sn, "/", id1)
+ expectExists(t, sn, "/a", id2)
+ expectExists(t, sn, "/a/b", id3)
+ expectExists(t, sn, "/a/b/c", id4)
+ expectExists(t, sn, "/a/b/c/d", id5)
checkInRefs(t, sn)
// Parent directory has to exisn.
@@ -264,8 +267,8 @@
t.Errorf("Unexpected error: %s", err)
}
sn.gc()
- expectExists(t, sn, id1)
- expectExists(t, sn, id2)
+ expectExists(t, sn, "/", id1)
+ expectExists(t, sn, "/a", id2)
expectNotExists(t, sn, id3)
expectNotExists(t, sn, id4)
expectNotExists(t, sn, id5)
diff --git a/services/store/memstore/state/snapshot.go b/services/store/memstore/state/snapshot.go
index 4213a55..9c352f5 100644
--- a/services/store/memstore/state/snapshot.go
+++ b/services/store/memstore/state/snapshot.go
@@ -106,27 +106,13 @@
return e, nil
}
-// resolveCell performs a path-based lookup, traversing the state from the
-// root.
+// resolveCell performs a path-based lookup, traversing the state from the root.
//
// Returns (cell, suffix, v), where cell contains the value, suffix is the path
// to the value, v is the value itself. If the operation failed, the returned
// cell is nil.
func (sn *snapshot) resolveCell(checker *acl.Checker, path storage.PathName, mu *Mutations) (*Cell, storage.PathName, interface{}) {
- // Get the starting object.
- id, suffix, ok := path.GetID()
- if ok {
- path = suffix
- checker.Update(uidTagList)
- } else {
- id = sn.rootID
- }
-
- return sn.resolve(checker, id, path, mu)
-}
-
-func (sn *snapshot) resolve(checker *acl.Checker, id storage.ID, path storage.PathName, mu *Mutations) (*Cell, storage.PathName, interface{}) {
- cell := sn.Find(id)
+ cell := sn.Find(sn.rootID)
if cell == nil {
return nil, nil, nil
}
diff --git a/services/store/memstore/watch/raw_processor.go b/services/store/memstore/watch/raw_processor.go
index 5c8d76e..68dc10a 100644
--- a/services/store/memstore/watch/raw_processor.go
+++ b/services/store/memstore/watch/raw_processor.go
@@ -1,8 +1,6 @@
package watch
import (
- "fmt"
-
"veyron/services/store/memstore/refs"
"veyron/services/store/memstore/state"
"veyron/services/store/raw"
@@ -89,7 +87,6 @@
Dir: flattenDir(refs.FlattenDir(cell.Dir)),
}
change := watch.Change{
- Name: uidName(id),
State: watch.Exists,
Value: value,
}
@@ -125,7 +122,6 @@
}
// TODO(tilaks): don't clone value.
change := watch.Change{
- Name: uidName(p.rootID),
State: watch.DoesNotExist,
Value: value,
}
@@ -157,7 +153,6 @@
}
// TODO(tilaks): don't clone value.
change := watch.Change{
- Name: uidName(id),
State: watch.Exists,
Value: value,
}
@@ -180,7 +175,6 @@
}
// TODO(tilaks): don't clone value.
change := watch.Change{
- Name: uidName(id),
State: watch.DoesNotExist,
Value: value,
}
@@ -190,11 +184,6 @@
return changes, nil
}
-// uidName returns path "<uid dir name>/<id>" for the object.
-func uidName(id storage.ID) string {
- return fmt.Sprintf("/%s/%s", storage.UIDDirName, id)
-}
-
// TODO(tilaks): revisit when raw.Mutation.Dir is of type []*storage.DEntry
// (once we support optional structs in the idl).
func flattenDir(pdir []*storage.DEntry) []storage.DEntry {
