diff --git a/cmd/test.go b/cmd/test.go index a60fd5edf1..cdf8fe5f63 100644 --- a/cmd/test.go +++ b/cmd/test.go @@ -302,7 +302,7 @@ func processWatcherUpdate(ctx context.Context, testParams testCommandParams, pat err := pathwatcher.ProcessWatcherUpdateForRegoVersion(ctx, testParams.RegoVersion(), paths, removed, store, filter, testParams.bundleMode, false, func(ctx context.Context, txn storage.Transaction, loaded *initload.LoadPathsResult) error { if len(loaded.Files.Documents) > 0 || removed != "" { - if err := store.Write(ctx, txn, storage.AddOp, storage.Path{}, loaded.Files.Documents); err != nil { + if err := store.Write(ctx, txn, storage.AddOp, storage.RootPath, loaded.Files.Documents); err != nil { return fmt.Errorf("storage error: %w", err) } } diff --git a/internal/bundle/utils.go b/internal/bundle/utils.go index 836aa586b9..98093b774e 100644 --- a/internal/bundle/utils.go +++ b/internal/bundle/utils.go @@ -72,7 +72,7 @@ func LoadWasmResolversFromStore(ctx context.Context, store storage.Store, txn st var resolvers []*wasm.Resolver if len(resolversToLoad) > 0 { // Get a full snapshot of the current data (including any from "outside" the bundles) - data, err := store.Read(ctx, txn, storage.Path{}) + data, err := store.Read(ctx, txn, storage.RootPath) if err != nil { return nil, fmt.Errorf("failed to initialize wasm runtime: %s", err) } diff --git a/internal/presentation/presentation_test.go b/internal/presentation/presentation_test.go index 28f44139d7..3f8504ad4c 100644 --- a/internal/presentation/presentation_test.go +++ b/internal/presentation/presentation_test.go @@ -126,7 +126,7 @@ func TestOutputJSONErrorStructuredASTErr(t *testing.T) { func TestOutputJSONErrorStructuredStorageErr(t *testing.T) { store := inmem.New() txn := storage.NewTransactionOrDie(t.Context(), store) - err := store.Write(t.Context(), txn, storage.AddOp, storage.Path{}, map[string]any{"foo": 1}) + err := store.Write(t.Context(), txn, storage.AddOp, storage.RootPath, map[string]any{"foo": 1}) expected := `{ "errors": [ { diff --git a/internal/runtime/init/init.go b/internal/runtime/init/init.go index 530deadb9a..de8ef87401 100644 --- a/internal/runtime/init/init.go +++ b/internal/runtime/init/init.go @@ -42,7 +42,7 @@ type InsertAndCompileResult struct { // store contents. func InsertAndCompile(ctx context.Context, opts InsertAndCompileOptions) (*InsertAndCompileResult, error) { if len(opts.Files.Documents) > 0 { - if err := opts.Store.Write(ctx, opts.Txn, storage.AddOp, storage.Path{}, opts.Files.Documents); err != nil { + if err := opts.Store.Write(ctx, opts.Txn, storage.AddOp, storage.RootPath, opts.Files.Documents); err != nil { return nil, fmt.Errorf("storage error: %w", err) } } diff --git a/v1/debug/thread.go b/v1/debug/thread.go index b688de503c..76ae72fba3 100644 --- a/v1/debug/thread.go +++ b/v1/debug/thread.go @@ -471,7 +471,7 @@ func (t *thread) inputVars(e *topdown.Event) VarRef { func (t *thread) dataVars() VarRef { return t.varManager.addVars(func() []namedVar { ctx := context.Background() - d, err := storage.ReadOne(ctx, t.store, storage.Path{}) + d, err := storage.ReadOne(ctx, t.store, storage.RootPath) if err != nil { return nil } diff --git a/v1/plugins/bundle/plugin_test.go b/v1/plugins/bundle/plugin_test.go index 121ca5797e..6e6f5bf0b0 100644 --- a/v1/plugins/bundle/plugin_test.go +++ b/v1/plugins/bundle/plugin_test.go @@ -109,7 +109,7 @@ func TestPluginOneShot(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) expData := util.MustUnmarshalJSON([]byte(`{ "foo": {"bar": 1, "baz": "qux"}, "system": { @@ -163,7 +163,7 @@ func TestPluginOneShotWithAstStore(t *testing.T) { txn := storage.NewTransactionOrDie(ctx, manager.Store) defer manager.Store.Abort(ctx, txn) - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) expData := ast.MustParseTerm(`{"foo": {"bar": 1, "baz": "qux"}, "system": {"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}}}}`) if err != nil { t.Fatal(err) @@ -993,7 +993,7 @@ func TestPluginStartLazyLoadInMem(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) if err != nil { t.Fatal(err) } @@ -1118,7 +1118,7 @@ func TestPluginOneShotDiskStorageMetrics(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) expData := util.MustUnmarshalJSON([]byte(`{ "foo": {"bar": 1, "baz": "qux"}, "system": { @@ -1227,7 +1227,7 @@ func TestPluginOneShotDeltaBundle(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) if err != nil { t.Fatal(err) } @@ -1338,7 +1338,7 @@ func TestPluginOneShotDeltaBundleWithAstStore(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) if err != nil { t.Fatal(err) } @@ -1535,7 +1535,7 @@ func TestPluginOneShotBundlePersistence(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) expData := util.MustUnmarshalJSON([]byte(`{ "foo": {"bar": 1, "baz": "qux"}, "system": { @@ -1731,7 +1731,7 @@ corge contains 1 if { } }`)) - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) if err != nil { t.Fatal(err) } else if !reflect.DeepEqual(data, expData) { @@ -2017,7 +2017,7 @@ corge contains 1 if { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) var manifestRegoVersion string var moduleRegoVersion string @@ -2143,7 +2143,7 @@ func TestPluginOneShotSignedBundlePersistence(t *testing.T) { t.Fatal("Expected no policy") } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) if err != nil { t.Fatal(err) } @@ -2228,7 +2228,7 @@ func TestLoadAndActivateBundlesFromDisk(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) expData := util.MustUnmarshalJSON([]byte(`{ "foo": {"bar": 1, "baz": "qux"}, "system": { @@ -2314,7 +2314,7 @@ func TestLoadAndActivateBundlesFromDiskReservedChars(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) expData := util.MustUnmarshalJSON([]byte(`{ "foo": {"bar": 1, "baz": "qux"}, "system": { @@ -2570,7 +2570,7 @@ corge contains 2 if { } }`)) - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) if err != nil { fatal(err) } else if !reflect.DeepEqual(data, expData) { @@ -2781,7 +2781,7 @@ corge contains 1 if { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) manifestRegoVersionStr := "" if tc.bundleRegoVersion != nil { @@ -3231,7 +3231,7 @@ func TestPluginOneShotActivationRemovesOld(t *testing.T) { } else if !slices.Equal([]string{filepath.Join(bundleName, "example2.rego")}, ids) { return errors.New("expected updated policy ids") } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) // remove system key to make comparison simpler delete(data.(map[string]any), "system") if err != nil { @@ -3884,7 +3884,7 @@ func TestPluginActivateScopedBundle(t *testing.T) { if err := storage.Txn(ctx, manager.Store, storage.WriteParams, func(txn storage.Transaction) error { externalData := map[string]any{"a": map[string]any{"a1": "x1", "a3": "x2", "a5": "x3"}} - if err := manager.Store.Write(ctx, txn, storage.AddOp, storage.Path{}, externalData); err != nil { + if err := manager.Store.Write(ctx, txn, storage.AddOp, storage.RootPath, externalData); err != nil { return err } if err := manager.Store.UpsertPolicy(ctx, txn, "some/id1", []byte(`package a.a2`)); err != nil { @@ -7137,7 +7137,7 @@ func TestPluginManualTriggerMultipleDiskStorage(t *testing.T) { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) + data, err := manager.Store.Read(ctx, txn, storage.RootPath) expData := util.MustUnmarshalJSON([]byte(`{ "p": "x1", "q": "x2", "system": { diff --git a/v1/rego/example_test.go b/v1/rego/example_test.go index 5b01a6f7c4..76ab995c08 100644 --- a/v1/rego/example_test.go +++ b/v1/rego/example_test.go @@ -372,7 +372,7 @@ func ExampleRego_Eval_persistent_storage() { // Handle error. } - err = storage.WriteOne(ctx, store, storage.AddOp, storage.Path{}, json) + err = storage.WriteOne(ctx, store, storage.AddOp, storage.RootPath, json) if err != nil { // Handle error } diff --git a/v1/rego/rego.go b/v1/rego/rego.go index 8a47d90a93..2c4d8a8d91 100644 --- a/v1/rego/rego.go +++ b/v1/rego/rego.go @@ -1798,7 +1798,7 @@ func (r *Rego) PrepareForEval(ctx context.Context, opts ...PrepareOption) (Prepa } // nolint: staticcheck // SA4006 false positive - data, err := r.store.Read(ctx, r.txn, storage.Path{}) + data, err := r.store.Read(ctx, r.txn, storage.RootPath) if err != nil { _ = txnClose(ctx, err) // Ignore error return PreparedEvalQuery{}, err @@ -2020,7 +2020,7 @@ func (r *Rego) loadFiles(ctx context.Context, txn storage.Transaction, m metrics } if len(result.Documents) > 0 { - err = r.store.Write(ctx, txn, storage.AddOp, storage.Path{}, result.Documents) + err = r.store.Write(ctx, txn, storage.AddOp, storage.RootPath, result.Documents) if err != nil { return err } diff --git a/v1/repl/repl.go b/v1/repl/repl.go index 1becbfabd9..0fdc1e308a 100644 --- a/v1/repl/repl.go +++ b/v1/repl/repl.go @@ -1410,7 +1410,7 @@ func newCommand(line string) *command { } func dumpStorage(ctx context.Context, store storage.Store, txn storage.Transaction, w io.Writer) error { - data, err := store.Read(ctx, txn, storage.Path{}) + data, err := store.Read(ctx, txn, storage.RootPath) if err != nil { return err } diff --git a/v1/storage/inmem/inmem.go b/v1/storage/inmem/inmem.go index 742d6c167f..1ee084e411 100644 --- a/v1/storage/inmem/inmem.go +++ b/v1/storage/inmem/inmem.go @@ -71,7 +71,7 @@ func NewFromObjectWithOpts(data map[string]any, opts ...Opt) storage.Store { if err != nil { panic(err) } - if err := db.Write(ctx, txn, storage.AddOp, storage.Path{}, data); err != nil { + if err := db.Write(ctx, txn, storage.AddOp, storage.RootPath, data); err != nil { panic(err) } if err := db.Commit(ctx, txn); err != nil { @@ -120,19 +120,23 @@ type handle struct { } func (db *store) NewTransaction(_ context.Context, params ...storage.TransactionParams) (storage.Transaction, error) { - var write bool - var ctx *storage.Context - if len(params) > 0 { - write = params[0].Write - ctx = params[0].Context + txn := &transaction{ + xid: atomic.AddUint64(&db.xid, uint64(1)), + db: db, } - xid := atomic.AddUint64(&db.xid, uint64(1)) - if write { + + if len(params) > 0 { + txn.write = params[0].Write + txn.context = params[0].Context + } + + if txn.write { db.wmu.Lock() } else { db.rmu.RLock() } - return newTransaction(xid, write, ctx, db), nil + + return txn, nil } // Truncate implements the storage.Store interface. This method must be called within a transaction. @@ -193,11 +197,7 @@ func (db *store) Truncate(ctx context.Context, txn storage.Transaction, params s // For backwards compatibility, check if `RootOverwrite` was configured. if params.RootOverwrite { - newPath, ok := storage.ParsePathEscaped("/") - if !ok { - return fmt.Errorf("storage path invalid: %v", newPath) - } - return underlying.Write(storage.AddOp, newPath, mergedData) + return underlying.Write(storage.AddOp, storage.RootPath, mergedData) } for _, root := range params.BasePaths { diff --git a/v1/storage/inmem/inmem_bench_test.go b/v1/storage/inmem/inmem_bench_test.go new file mode 100644 index 0000000000..0d4c72d344 --- /dev/null +++ b/v1/storage/inmem/inmem_bench_test.go @@ -0,0 +1,130 @@ +package inmem_test + +import ( + "testing" + + "github.com/open-policy-agent/opa/v1/storage" + "github.com/open-policy-agent/opa/v1/storage/inmem" +) + +type ( + txnType bool + target struct { + name string + store storage.Store + } + targets []target + + withTxnFn func(b *testing.B, target storage.Store, txn storage.Transaction) + noTxnFn func(b *testing.B, target storage.Store) +) + +const ( + readTxn txnType = false + writeTxn txnType = true +) + +func BenchmarkNewTransaction(b *testing.B) { + store := inmem.NewFromObject(map[string]any{}) + + for name, typ := range map[string]txnType{"Read": readTxn, "Write": writeTxn} { + b.Run(name, func(b *testing.B) { + for b.Loop() { + txn, err := store.NewTransaction(b.Context(), transactionParams(typ)...) + if err != nil { + b.Fatal(err) + } + store.Abort(b.Context(), txn) + } + }) + } +} + +func BenchmarkReadOne(b *testing.B) { + data := map[string]any{"foo": "bar"} + path := storage.Path{"foo"} + + AllStores(data).Bench(b, func(b *testing.B, store storage.Store) { + if _, err := storage.ReadOne(b.Context(), store, path); err != nil { + b.Fatal(err) + } + }) +} + +func BenchmarkRead(b *testing.B) { + data := map[string]any{"foo": "bar"} + path := storage.Path{"foo"} + + AllStores(data).BenchWithTxn(b, readTxn, func(b *testing.B, s storage.Store, txn storage.Transaction) { + if _, err := s.Read(b.Context(), txn, path); err != nil { + b.Fatal(err) + } + }) +} + +func BenchmarkWriteOne(b *testing.B) { + data := map[string]any{} + path := storage.Path{"foo"} + + AllStores(data).Bench(b, func(b *testing.B, store storage.Store) { + if err := storage.WriteOne(b.Context(), store, storage.AddOp, path, "bar"); err != nil { + b.Fatal(err) + } + }) +} + +func transactionParams(mode txnType) (params []storage.TransactionParams) { + if mode == writeTxn { + params = append(params, storage.WriteParams) + } + return params +} + +func AllStores(data map[string]any) targets { + return []target{ + { + "Go store (roundtrip)", + inmem.NewFromObjectWithOpts(data, inmem.OptRoundTripOnWrite(true)), + }, + { + "Go store (no roundtrip)", + inmem.NewFromObjectWithOpts(data, inmem.OptRoundTripOnWrite(false)), + }, + { + "AST store (roundtrip)", + inmem.NewFromObjectWithOpts(data, inmem.OptReturnASTValuesOnRead(true), inmem.OptRoundTripOnWrite(true)), + }, + { + "AST store (no roundtrip)", + inmem.NewFromObjectWithOpts(data, inmem.OptReturnASTValuesOnRead(true), inmem.OptRoundTripOnWrite(false)), + }, + } +} + +func (t targets) Bench(b *testing.B, fn noTxnFn) { + b.Helper() + + for _, target := range t { + b.Run(target.name, func(b *testing.B) { + for b.Loop() { + fn(b, target.store) + } + }) + } +} + +func (t targets) BenchWithTxn(b *testing.B, mode txnType, fn withTxnFn) { + b.Helper() + + for _, target := range t { + b.Run(target.name, func(b *testing.B) { + txn := storage.NewTransactionOrDie(b.Context(), target.store, transactionParams(mode)...) + + for b.Loop() { + fn(b, target.store, txn) + } + + target.store.Abort(b.Context(), txn) + }) + } +} diff --git a/v1/storage/inmem/inmem_test.go b/v1/storage/inmem/inmem_test.go index 341a6905b5..c3e9c1bf60 100644 --- a/v1/storage/inmem/inmem_test.go +++ b/v1/storage/inmem/inmem_test.go @@ -195,7 +195,7 @@ func TestInMemoryWrite(t *testing.T) { store := NewFromObjectWithOpts(data, OptReturnASTValuesOnRead(rvt.ast)) // Perform patch and check result - value := loadExpectedSortedResult(tc.value) + value := loadExpectedResult(tc.value) var op storage.PatchOp switch tc.op { @@ -304,7 +304,7 @@ func TestInMemoryWriteOfStruct(t *testing.T) { t.Fatal(err) } - expected := loadExpectedSortedResult(tc.expected) + expected := loadExpectedResult(tc.expected) if !reflect.DeepEqual(expected, actual) { t.Errorf("expected %v, got %v", tc.expected, actual) } @@ -1235,21 +1235,8 @@ func loadExpectedResult(input string) any { if len(input) == 0 { return nil } - var data any - if err := util.UnmarshalJSON([]byte(input), &data); err != nil { - panic(err) - } - return data -} -func loadExpectedSortedResult(input string) any { - data := loadExpectedResult(input) - switch data := data.(type) { - case []any: - return data - default: - return data - } + return util.MustUnmarshalJSON([]byte(input)) } func loadSmallTestData() map[string]any { diff --git a/v1/storage/inmem/txn.go b/v1/storage/inmem/txn.go index 28e68c20f2..eec32c45a6 100644 --- a/v1/storage/inmem/txn.go +++ b/v1/storage/inmem/txn.go @@ -34,13 +34,13 @@ import ( // Read transactions do not require any special handling and simply passthrough // to the underlying store. Read transactions do not support upgrade. type transaction struct { + db *store + updates *list.List + context *storage.Context + policies map[string]policyUpdate xid uint64 write bool stale bool - db *store - updates *list.List - policies map[string]policyUpdate - context *storage.Context } type policyUpdate struct { @@ -48,28 +48,17 @@ type policyUpdate struct { remove bool } -func newTransaction(xid uint64, write bool, context *storage.Context, db *store) *transaction { - return &transaction{ - xid: xid, - write: write, - db: db, - policies: map[string]policyUpdate{}, - updates: list.New(), - context: context, - } -} - func (txn *transaction) ID() uint64 { return txn.xid } func (txn *transaction) Write(op storage.PatchOp, path storage.Path, value any) error { - if !txn.write { - return &storage.Error{ - Code: storage.InvalidTransactionErr, - Message: "data write during read transaction", - } + return &storage.Error{Code: storage.InvalidTransactionErr, Message: "data write during read transaction"} + } + + if txn.updates == nil { + txn.updates = list.New() } if len(path) == 0 { @@ -145,7 +134,7 @@ func (txn *transaction) updateRoot(op storage.PatchOp, value any) error { } update = &updateAST{ - path: storage.Path{}, + path: storage.RootPath, remove: false, value: valueAST, } @@ -155,7 +144,7 @@ func (txn *transaction) updateRoot(op storage.PatchOp, value any) error { } update = &updateRaw{ - path: storage.Path{}, + path: storage.RootPath, remove: false, value: value, } @@ -163,20 +152,23 @@ func (txn *transaction) updateRoot(op storage.PatchOp, value any) error { txn.updates.Init() txn.updates.PushFront(update) + return nil } func (txn *transaction) Commit() (result storage.TriggerEvent) { result.Context = txn.context - for curr := txn.updates.Front(); curr != nil; curr = curr.Next() { - action := curr.Value.(dataUpdate) - txn.db.data = action.Apply(txn.db.data) + if txn.updates != nil { + for curr := txn.updates.Front(); curr != nil; curr = curr.Next() { + action := curr.Value.(dataUpdate) + txn.db.data = action.Apply(txn.db.data) - result.Data = append(result.Data, storage.DataEvent{ - Path: action.Path(), - Data: action.Value(), - Removed: action.Remove(), - }) + result.Data = append(result.Data, storage.DataEvent{ + Path: action.Path(), + Data: action.Value(), + Removed: action.Remove(), + }) + } } for id, upd := range txn.policies { if upd.remove { @@ -218,8 +210,7 @@ func deepcpy(v any) any { } func (txn *transaction) Read(path storage.Path) (any, error) { - - if !txn.write { + if !txn.write || txn.updates == nil { return pointer(txn.db.data, path) } @@ -260,8 +251,7 @@ func (txn *transaction) Read(path storage.Path) (any, error) { return cpy, nil } -func (txn *transaction) ListPolicies() []string { - var ids []string +func (txn *transaction) ListPolicies() (ids []string) { for id := range txn.db.policies { if _, ok := txn.policies[id]; !ok { ids = append(ids, id) @@ -276,11 +266,13 @@ func (txn *transaction) ListPolicies() []string { } func (txn *transaction) GetPolicy(id string) ([]byte, error) { - if update, ok := txn.policies[id]; ok { - if !update.remove { - return update.value, nil + if txn.policies != nil { + if update, ok := txn.policies[id]; ok { + if !update.remove { + return update.value, nil + } + return nil, errors.NewNotFoundErrorf("policy id %q", id) } - return nil, errors.NewNotFoundErrorf("policy id %q", id) } if exist, ok := txn.db.policies[id]; ok { return exist, nil @@ -289,24 +281,24 @@ func (txn *transaction) GetPolicy(id string) ([]byte, error) { } func (txn *transaction) UpsertPolicy(id string, bs []byte) error { - if !txn.write { - return &storage.Error{ - Code: storage.InvalidTransactionErr, - Message: "policy write during read transaction", - } - } - txn.policies[id] = policyUpdate{bs, false} - return nil + return txn.updatePolicy(id, policyUpdate{bs, false}) } func (txn *transaction) DeletePolicy(id string) error { + return txn.updatePolicy(id, policyUpdate{nil, true}) +} + +func (txn *transaction) updatePolicy(id string, update policyUpdate) error { if !txn.write { - return &storage.Error{ - Code: storage.InvalidTransactionErr, - Message: "policy write during read transaction", - } + return &storage.Error{Code: storage.InvalidTransactionErr, Message: "policy write during read transaction"} } - txn.policies[id] = policyUpdate{nil, true} + + if txn.policies == nil { + txn.policies = map[string]policyUpdate{id: update} + } else { + txn.policies[id] = update + } + return nil } diff --git a/v1/storage/path.go b/v1/storage/path.go index f774d2eeda..16bb3e42c5 100644 --- a/v1/storage/path.go +++ b/v1/storage/path.go @@ -8,40 +8,40 @@ import ( "errors" "fmt" "net/url" + "slices" "strconv" "strings" "github.com/open-policy-agent/opa/v1/ast" ) +// RootPath refers to the root document in storage. +var RootPath = Path{} + // Path refers to a document in storage. type Path []string // ParsePath returns a new path for the given str. func ParsePath(str string) (path Path, ok bool) { - if len(str) == 0 { - return nil, false - } - if str[0] != '/' { + if len(str) == 0 || str[0] != '/' { return nil, false } if len(str) == 1 { return Path{}, true } - parts := strings.Split(str[1:], "/") - return parts, true + + return strings.Split(str[1:], "/"), true } // ParsePathEscaped returns a new path for the given escaped str. func ParsePathEscaped(str string) (path Path, ok bool) { - path, ok = ParsePath(str) - if !ok { - return - } - for i := range path { - segment, err := url.PathUnescape(path[i]) - if err == nil { - path[i] = segment + if path, ok = ParsePath(str); ok { + for i := range path { + if segment, err := url.PathUnescape(path[i]); err == nil { + path[i] = segment + } else { + return nil, false + } } } return @@ -49,7 +49,6 @@ func ParsePathEscaped(str string) (path Path, ok bool) { // NewPathForRef returns a new path for the given ref. func NewPathForRef(ref ast.Ref) (path Path, err error) { - if len(ref) == 0 { return nil, errors.New("empty reference (indicates error in caller)") } @@ -85,36 +84,17 @@ func NewPathForRef(ref ast.Ref) (path Path, err error) { // is less than other, 0 if p is equal to other, or 1 if p is greater than // other. func (p Path) Compare(other Path) (cmp int) { - for i := range min(len(p), len(other)) { - if cmp := strings.Compare(p[i], other[i]); cmp != 0 { - return cmp - } - } - if len(p) < len(other) { - return -1 - } - if len(p) == len(other) { - return 0 - } - return 1 + return slices.Compare(p, other) } // Equal returns true if p is the same as other. func (p Path) Equal(other Path) bool { - return p.Compare(other) == 0 + return slices.Equal(p, other) } // HasPrefix returns true if p starts with other. func (p Path) HasPrefix(other Path) bool { - if len(other) > len(p) { - return false - } - for i := range other { - if p[i] != other[i] { - return false - } - } - return true + return len(other) <= len(p) && p[:len(other)].Equal(other) } // Ref returns a ref that represents p rooted at head. diff --git a/v1/storage/path_test.go b/v1/storage/path_test.go index 3a19e77374..31820d51b7 100644 --- a/v1/storage/path_test.go +++ b/v1/storage/path_test.go @@ -23,7 +23,7 @@ func TestNewPathForString(t *testing.T) { }{ {"", nil, false}, {"foo", nil, false}, - {"/", Path{}, true}, + {"/", RootPath, true}, {"/", nil, true}, {"/foo", Path{"foo"}, true}, {"/foo/bar", Path{"foo", "bar"}, true}, @@ -53,7 +53,7 @@ func TestNewPathForRef(t *testing.T) { {ast.MustParseRef("data.foo[{1, 2}]"), nil, fmt.Errorf("composites cannot be base document keys: %v", ast.MustParseRef("data.foo[{1, 2}]"))}, {ast.MustParseRef(`data.foo[{"foo": 2}]`), nil, fmt.Errorf("composites cannot be base document keys: %v", ast.MustParseRef(`data.foo[{"foo": 2}]`))}, - {ast.MustParseRef("data"), Path{}, nil}, + {ast.MustParseRef("data"), RootPath, nil}, {ast.MustParseRef("data.foo"), Path{"foo"}, nil}, {ast.MustParseRef("data.foo[1]"), Path{"foo", "1"}, nil}, {ast.MustParseRef("data.foo.bar"), Path{"foo", "bar"}, nil}, @@ -70,12 +70,15 @@ func TestNewPathForRef(t *testing.T) { } func TestNewPathForStringEscaped(t *testing.T) { - tests := []struct { input string result Path ok bool }{ + { + input: "", + ok: false, + }, { input: "/foo/bar", // no escaping result: Path{"foo", "bar"}, @@ -91,6 +94,11 @@ func TestNewPathForStringEscaped(t *testing.T) { result: Path{"foo//bar", "baz"}, ok: true, }, + { + input: "/foo%%%%bar", + result: nil, // invalid escaping + ok: false, + }, } for _, tc := range tests { @@ -107,9 +115,9 @@ func TestPathCompare(t *testing.T) { b Path result int }{ - {Path{}, Path{}, 0}, - {Path{}, Path{"x"}, -1}, - {Path{"x"}, Path{}, 1}, + {RootPath, RootPath, 0}, + {RootPath, Path{"x"}, -1}, + {Path{"x"}, RootPath, 1}, {Path{"x"}, Path{"x"}, 0}, {Path{"x"}, Path{"y"}, -1}, {Path{"x"}, Path{"w"}, 1}, @@ -133,9 +141,9 @@ func TestPathEqual(t *testing.T) { b Path result bool }{ - {Path{}, Path{}, true}, - {Path{}, Path{"foo"}, false}, - {Path{"foo"}, Path{}, false}, + {RootPath, RootPath, true}, + {RootPath, Path{"foo"}, false}, + {Path{"foo"}, RootPath, false}, {Path{"foo", "bar"}, Path{"foo"}, false}, {Path{"foo", "bar"}, Path{"foo", "bar"}, true}, } @@ -153,15 +161,15 @@ func TestPathHasPrefix(t *testing.T) { b Path result bool }{ - {Path{}, Path{}, true}, - {Path{}, Path{"foo"}, false}, - {Path{"foo"}, Path{}, true}, + {RootPath, RootPath, true}, + {RootPath, Path{"foo"}, false}, + {Path{"foo"}, RootPath, true}, {Path{"foo"}, Path{"bar"}, false}, {Path{"bar"}, Path{"foo"}, false}, {Path{"foo", "bar"}, Path{"foo"}, true}, {Path{"foo", "bar"}, Path{"foo", "bar"}, true}, {Path{"foo", "bar"}, Path{"foo", "bar", "baz"}, false}, - {Path{"foo", "bar", "baz"}, Path{}, true}, + {Path{"foo", "bar", "baz"}, RootPath, true}, } for _, tc := range tests { result := tc.a.HasPrefix(tc.b) diff --git a/v1/test/authz/authz_bench_test.go b/v1/test/authz/authz_bench_test.go index 59eecad9c3..15f665528f 100644 --- a/v1/test/authz/authz_bench_test.go +++ b/v1/test/authz/authz_bench_test.go @@ -61,7 +61,7 @@ func runAuthzBenchmark(b *testing.B, mode InputMode, numPaths int, extras ...boo b.Fatal(err) } - if err = storage.WriteOne(ctx, store, storage.AddOp, storage.Path{}, data); err != nil { + if err = storage.WriteOne(ctx, store, storage.AddOp, storage.RootPath, data); err != nil { b.Fatal(err) } } else { diff --git a/v1/tester/runner.go b/v1/tester/runner.go index e6aaaa8de7..de95859eb5 100644 --- a/v1/tester/runner.go +++ b/v1/tester/runner.go @@ -1172,10 +1172,12 @@ func LoadWithRegoVersion(args []string, filter loader.Filter, regoVersion ast.Re } var store storage.Store + ctx := context.Background() + if bundle.BundleExtStore != nil { store = bundle.BundleExtStore() // inline'd NewFromObject - if err := storage.WriteOne(context.Background(), store, storage.AddOp, storage.Path{}, loaded.Documents); err != nil { + if err := storage.WriteOne(ctx, store, storage.AddOp, storage.RootPath, loaded.Documents); err != nil { return nil, nil, err } } else { @@ -1183,7 +1185,7 @@ func LoadWithRegoVersion(args []string, filter loader.Filter, regoVersion ast.Re } modules := make(map[string]*ast.Module, len(loaded.Modules)) - ctx := context.Background() + err = storage.Txn(ctx, store, storage.WriteParams, func(txn storage.Transaction) error { for _, loadedModule := range loaded.Modules { modules[loadedModule.Name] = loadedModule.Parsed @@ -1213,12 +1215,15 @@ func LoadWithParserOptions(args []string, filter loader.Filter, popts ast.Parser if err != nil { return nil, nil, err } + var store storage.Store + ctx := context.Background() + // Plumb in storage for external bundle activation plugin, if registered with bundle.RegisterStore. if bundle.BundleExtStore != nil { store = bundle.BundleExtStore() // inline'd NewFromObject - if err := storage.WriteOne(context.Background(), store, storage.AddOp, storage.Path{}, loaded.Documents); err != nil { + if err := storage.WriteOne(ctx, store, storage.AddOp, storage.RootPath, loaded.Documents); err != nil { return nil, nil, err } } else { @@ -1226,7 +1231,6 @@ func LoadWithParserOptions(args []string, filter loader.Filter, popts ast.Parser } modules := make(map[string]*ast.Module, len(loaded.Modules)) - ctx := context.Background() err = storage.Txn(ctx, store, storage.WriteParams, func(txn storage.Transaction) error { for _, loadedModule := range loaded.Modules { modules[loadedModule.Name] = loadedModule.Parsed @@ -1234,8 +1238,7 @@ func LoadWithParserOptions(args []string, filter loader.Filter, popts ast.Parser // Add the policies to the store to ensure that any future bundle // activations will preserve them and re-compile the module with // the bundle modules. - err := store.UpsertPolicy(ctx, txn, loadedModule.Name, loadedModule.Raw) - if err != nil { + if err := store.UpsertPolicy(ctx, txn, loadedModule.Name, loadedModule.Raw); err != nil { return err } }