storage/inmem: allow opt out of object roundtrip #5015

srenatus · 2022-08-15T14:50:35Z

This is a continuance of @willbeason's work in #4709.

It will allow everyone to use an in-memory store without roundtripping. Before, we had been roundtripping every object written to that store through json, to canonicalize its data type. However, for many code paths, this isn't necessary. Notably, when our own http handlers take in JSON through encoding/json and unmarshal it, it'll always be in the right format, and the extra roundtrip can be avoided.

This roundtrip would also create a deep copy of the object, and with the opt-out, that no longer happens.

Fixes #4708.

srenatus · 2022-08-16T12:13:29Z

@ashutosh-narkar I've requested your review because I had changed some things in the bundle plugin tests. I think we're clear -- whenever a bundle is read, it'll go through encoding/json, and thus only have "safe" types, like, using []interface{}{"string1", "string2"} and not []string{"string1", "string2"}. It's only the test setup that necessitated the changes.

srenatus

comments

srenatus · 2022-08-16T12:16:03Z

internal/presentation/presentation_test.go

@@ -18,7 +18,7 @@ import (
 	"github.com/open-policy-agent/opa/loader"
 	"github.com/open-policy-agent/opa/rego"
 	"github.com/open-policy-agent/opa/storage"
-	"github.com/open-policy-agent/opa/storage/inmem"
+	inmem "github.com/open-policy-agent/opa/storage/inmem/test"
 	"github.com/open-policy-agent/opa/util"


I think I've flicked all (or most) tests to use the non-roundtripping inmem store. It just goes through smoothly, so we're probably OK wrt how it works in OPA-as-server.

srenatus · 2022-08-16T12:16:25Z

loader/loader.go

@@ -55,7 +55,13 @@ func (l *Result) Compiler() (*ast.Compiler, error) {

 // Store returns a Store object with the documents from this loader result.
 func (l *Result) Store() (storage.Store, error) {


I couldn't find this one being used in our code bases, but I figured for completeness' sake, we could still add 👇

srenatus · 2022-08-16T12:19:33Z

plugins/bundle/plugin_test.go

@@ -402,7 +402,7 @@ func TestPluginOneShotDeltaBundle(t *testing.T) {
 	p2 := bundle.PatchOperation{
 		Op:    "upsert",
 		Path:  "/a/foo",
-		Value: []string{"hello", "world"},
+		Value: []interface{}{"hello", "world"},
 	}


Before, this would be roundtripped though json, and it would be turned into []interface{} (see playground). Now, we're feeding in the proper value types.

In real life, this shouldn't matter, since when bundles are read, their Values should come from a json string unmarshalled using encoding/json, so we'd only ever get []interface{}, never []string, there.

The Go Playground example you included nicely illustrates your point. This change makes sense.

philipaconrad · 2022-08-17T18:19:38Z

plugins/bundle/plugin_test.go

 	data, err := manager.Store.Read(ctx, txn, storage.Path{})
-	expData := util.MustUnmarshalJSON([]byte(`{"a": {"baz": "bux", "foo": ["hello", "world"]}, "system": {"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "delta", "roots": ["a"]}}}}}`))
-
 	if err != nil {
 		t.Fatal(err)
-	} else if !reflect.DeepEqual(data, expData) {
-		t.Fatalf("Bad data content. Exp:\n%v\n\nGot:\n\n%v", expData, data)
+	}
+	expData := util.MustUnmarshalJSON([]byte(`{"a": {"baz": "bux", "foo": ["hello", "world"]}, "system": {"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "delta", "roots": ["a"]}}}}}`))
+	if !reflect.DeepEqual(data, expData) {
+		t.Fatalf("Bad data content. Exp:\n%#v\n\nGot:\n\n%#v", expData, data)


This was a small, but good fix. 👍

The err check is for the Store a few lines up, and MustUnmarshalJSON doesn't return an error, so an independent check for its error condition (wrong result) is put in a few lines down. This is clearer than the "chained-if" style that was there before, I think.

I think that's something a linter should be able to catch, anyways. If the if's branch breaks the control flow (like, return, or t.Fatal or panic), then there's no need to wrap the other conditionals into else. But maybe it's just a pet peeve of mine.

philipaconrad · 2022-08-17T18:26:59Z

storage/inmem/opts.go

+// Callers should disable use this if they can guarantee all objects passed to
+// Write() are serializable to JSON. Failing to do so may result in undefined
+// behavior, including panics.
+//
+// If setting to false consider deep-copying any objects passed to Write unless
+// they can guarantee the objects will not be mutated after being written.


Suggested change

// Callers should disable use this if they can guarantee all objects passed to

// Write() are serializable to JSON. Failing to do so may result in undefined

// behavior, including panics.

//

// If setting to false consider deep-copying any objects passed to Write unless

// they can guarantee the objects will not be mutated after being written.

// Callers should disable this if they cannot guarantee all objects passed to

// Write() are serializable to JSON. Failing to do so may result in undefined

// behavior, including panics.

//

// If setting to false, callers should consider deep-copying any objects passed

// to Write, unless they can guarantee the objects will not be mutated after

// being written to the store.

I've updated the comment in a later commit -- what do you think about that one?

One second... I'll go see the latest revisions. 😄

I'm a little confused by the "they" on line 23. I assume it's referring to the caller, and that they have a responsibility to ensure no mutations are happening to objects going into the store?

They need to ensure that if the object are mutated (in the store) it won't matter for them. Basically "hand them over"... I'm grateful for all suggestions here 😃

I added some extra words in the suggested change block to make it clear it's the caller we're talking about, because when I hear "consider <action>" by itself, I usually parse that as a command directed at me, instead of another target.

I know I'm bikeshedding a bit there. 🚲 It's probably fine, but clarity is always preferred in comments that may be there for a bit. 😄

storage/inmem/test/testutil.go

srenatus · 2022-08-17T18:39:54Z

Ideally, the code examples in the inmem/example_test.go would be updated, too. But I didn't get to that, and didn't want to push this further 😅

philipaconrad · 2022-08-17T18:45:00Z

Looks like DCO blew up because of a change in email identity of the committer. 🙁

@srenatus I think your GMail and Styra emails got mixed somewhere in the commit history, and confused the bot.

philipaconrad · 2022-08-17T18:48:01Z

Ideally, the code examples in the inmem/example_test.go would be updated, too. But I didn't get to that, and didn't want to push this further sweat_smile

Understandable. I don't see anything worrying in these commits, but definitely ping @ashutosh-narkar again if they haven't had time to see these changes.

I'm good to mark Approve in a bit once tests go green. 👍

philipaconrad · 2022-08-17T18:50:47Z

storage/inmem/opts.go

+// If setting to false consider deep-copying any objects passed to Write unless
+// they can guarantee the objects will not be mutated after being written.


Suggested change

// If setting to false consider deep-copying any objects passed to Write unless

// they can guarantee the objects will not be mutated after being written.

// If setting to false, callers should consider deep-copying any objects passed to

// Write() unless they can guarantee the objects will not be mutated after being written.

[nit] The "they" or "consider" blocks feel a little ambiguous, unless we tether at least on of them to an obvious subject somewhere nearby in the sentence.

philipaconrad

I've only got [nit]-level suggestions on this PR. The rest looks great! 😄

Marking Approve, so that @srenatus can get it merged when they have time to look at it tomorrow.

srenatus · 2022-08-18T05:22:01Z

I'll update the comment and merge this!

Add option to inmem.store which allows disabling the round-tripping through JSON when adding data to the store. This option is intended for callers who can guarantee the objects they pass to Write are JSON objects, and have properly ensured the object will be only be accessed by store once added. Fixes open-policy-agent#4708. This is continuance of open-policy-agent#4709, adding these bits: * storage/inmem: backwards-compat nitpicks, test adaptations I might have overshot here, but adding variable-length function parameters is not a backwards-compatible move. Concretely, if you had been using code like var x func() storage.Store = inmem.New going from New() to New(...Opts) would break it. * storage/inmem: use it where possible without roundtrip * storage/inmem: deal with nil map It looks like this is something the roundtrip had guarded us from. Now, we'll explicitly check this. This came up when running the bundle tests with roundtripping disabled. * loader: add StoreWithOpts convenience method Co-authored-by: Will Beason <willbeason@google.com> Co-authored-by: Philip Conrad <conradp@chariot-chaser.net> Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus force-pushed the sr+wb/storage/opt-out-of-roundtrip branch 7 times, most recently from ca89e34 to 7128f18 Compare August 16, 2022 11:58

srenatus marked this pull request as ready for review August 16, 2022 12:12

srenatus requested a review from ashutosh-narkar August 16, 2022 12:12

srenatus commented Aug 16, 2022

View reviewed changes

philipaconrad reviewed Aug 17, 2022

View reviewed changes

storage/inmem/test/testutil.go Outdated Show resolved Hide resolved

philipaconrad reviewed Aug 17, 2022

View reviewed changes

philipaconrad previously approved these changes Aug 17, 2022

View reviewed changes

ashutosh-narkar previously approved these changes Aug 17, 2022

View reviewed changes

srenatus dismissed stale reviews from ashutosh-narkar and philipaconrad via 8362bb2 August 18, 2022 05:21

srenatus force-pushed the sr+wb/storage/opt-out-of-roundtrip branch from 6456a23 to 8362bb2 Compare August 18, 2022 05:21

srenatus force-pushed the sr+wb/storage/opt-out-of-roundtrip branch from 8362bb2 to 72d8d50 Compare August 18, 2022 06:30

srenatus merged commit 137d7b6 into open-policy-agent:main Aug 18, 2022

srenatus deleted the sr+wb/storage/opt-out-of-roundtrip branch August 18, 2022 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage/inmem: allow opt out of object roundtrip #5015

storage/inmem: allow opt out of object roundtrip #5015

srenatus commented Aug 15, 2022

srenatus commented Aug 16, 2022

srenatus left a comment

srenatus Aug 16, 2022

srenatus Aug 16, 2022

srenatus Aug 16, 2022

philipaconrad Aug 17, 2022

philipaconrad Aug 17, 2022

srenatus Aug 17, 2022

philipaconrad Aug 17, 2022

srenatus Aug 17, 2022

philipaconrad Aug 17, 2022

philipaconrad Aug 17, 2022

srenatus Aug 17, 2022

philipaconrad Aug 17, 2022

srenatus commented Aug 17, 2022

philipaconrad commented Aug 17, 2022

philipaconrad commented Aug 17, 2022

philipaconrad Aug 17, 2022 •

edited

Loading

philipaconrad left a comment •

edited

Loading

srenatus commented Aug 18, 2022

		@@ -55,7 +55,13 @@ func (l Result) Compiler() (ast.Compiler, error) {

		// Store returns a Store object with the documents from this loader result.
		func (l *Result) Store() (storage.Store, error) {

		// If setting to false consider deep-copying any objects passed to Write unless
		// they can guarantee the objects will not be mutated after being written.

storage/inmem: allow opt out of object roundtrip #5015

storage/inmem: allow opt out of object roundtrip #5015

Conversation

srenatus commented Aug 15, 2022

srenatus commented Aug 16, 2022

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus commented Aug 17, 2022

philipaconrad commented Aug 17, 2022

philipaconrad commented Aug 17, 2022

philipaconrad Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

philipaconrad left a comment • edited Loading

Choose a reason for hiding this comment

srenatus commented Aug 18, 2022

philipaconrad Aug 17, 2022 •

edited

Loading

philipaconrad left a comment •

edited

Loading