Issue 2277: Add strict schema mode #2863
Conversation
…Don't create data directory that's not used.
…dd mutations mode tests.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @codexnull)
dgraph/cmd/alpha/run.go, line 141 at r1 (raw file):
flag.Bool("debugmode", false, "Enable debug mode for more debug information.") flag.String("mutations", "",
You can remove the nomutations flag above.
Also, you can set the default here to "allow", in the second argument.
dgraph/cmd/alpha/run.go, line 142 at r1 (raw file):
"Enable debug mode for more debug information.") flag.String("mutations", "", "Set mutation mode to allow, disallow, or strict. (default \"allow\")")
No need to mention this. --help would mention the default automatically.
dgraph/cmd/alpha/run.go, line 405 at r1 (raw file):
WALDir: Alpha.Conf.GetString("wal"), MutationsMode: edgraph.AllowMutations,
Alpha.Conf.GetString("mutations")
dgraph/cmd/alpha/run.go, line 425 at r1 (raw file):
if Alpha.Conf.GetBool("nomutations") { glog.Warning("--nomutations is deprecated in favor of --mutations=disallow")
It's alright to break that flag by removing it. It would only affect a few users.
dgraph/cmd/alpha/run.go, line 428 at r1 (raw file):
opts.MutationsMode = edgraph.DisallowMutations } if mutations := Alpha.Conf.GetString("mutations"); mutations != "" {
Don't check for empty strings. Empty string should be an error.
dgraph/cmd/alpha/run.go, line 431 at r1 (raw file):
switch strings.ToLower(mutations) { case "allow": if Alpha.Conf.GetBool("nomutations") {
remove.
dgraph/cmd/alpha/run.go, line 439 at r1 (raw file):
opts.MutationsMode = edgraph.DisallowMutations case "strict": if Alpha.Conf.GetBool("nomutations") {
remove.
edgraph/server.go, line 377 at r1 (raw file):
typ, err := schema.State().TypeOf(nquad.Predicate) if typ == types.UndefinedID { return resp, err
Hmm... This might not work. Because, in a multi-group system, each group owns certain predicates and has the schema for those predicates. If this endpoint receiving server does not serve that predicate, then it would return error here.
Can you test for that, by running a multi-group Dgraph cluster?
In that case, you'd need to enforce this in the worker package.
systest/_mutations_mode/docker-compose-mutations-mode.yml, line 4 at r1 (raw file):
# not add any new ones so that stopCluster still stops and deletes all containers. version: "3.5" services:
Why does the directory start with an underscore?
systest/_mutations_mode?
systest/_mutations_mode/mutations_mode_test.go, line 154 at r1 (raw file):
} func TestMutationsStrict(t *testing.T) {
These unit tests can be moved to within edgraph package. They don't look like heavy integration tests, worth putting into systest.
There was a problem hiding this comment.
Reviewable status: 8 of 9 files reviewed, 10 unresolved discussions (waiting on @manishrjain and @codexnull)
dgraph/cmd/alpha/run.go, line 141 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
You can remove the
nomutationsflag above.Also, you can set the default here to "allow", in the second argument.
Done.
dgraph/cmd/alpha/run.go, line 142 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
No need to mention this.
--helpwould mention the default automatically.
Done.
dgraph/cmd/alpha/run.go, line 405 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Alpha.Conf.GetString("mutations")
The config option value is a human-friendly string (i.e. "strict") but the MutationsMode setting is a computer-friendly int (e.g. 2 for "strict"). It seems better do to an int rather than string compare when checking weather to allow a mutation.
dgraph/cmd/alpha/run.go, line 425 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
It's alright to break that flag by removing it. It would only affect a few users.
Done.
dgraph/cmd/alpha/run.go, line 428 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Don't check for empty strings. Empty string should be an error.
I allowed an empty string so I could tell if the value was coming from the default given in flag.String() or a user-supplied setting so I could detect conflicts with --nomutations, but since that flag is going away this can too.
dgraph/cmd/alpha/run.go, line 431 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
remove.
Done.
dgraph/cmd/alpha/run.go, line 439 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
remove.
Done.
edgraph/server.go, line 377 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Hmm... This might not work. Because, in a multi-group system, each group owns certain predicates and has the schema for those predicates. If this endpoint receiving server does not serve that predicate, then it would return error here.
Can you test for that, by running a multi-group Dgraph cluster?
In that case, you'd need to enforce this in the worker package.
Working on it...
systest/_mutations_mode/docker-compose-mutations-mode.yml, line 4 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Why does the directory start with an underscore?
systest/_mutations_mode?
The runAll function in test.sh runs all the tests it finds after restarting the cluster, but this test requires a different cluster configuration (that's using the --mutations flag), so I changed runAll to ignore directories that start with '_' (there was already a directory named '_customtok'). These tests are run separately after restarting the cluster with the required settings.
I'm open to other ideas for excluding tests that can't be run against the cluster defined in docker-compose.yml.
systest/_mutations_mode/mutations_mode_test.go, line 154 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
These unit tests can be moved to within edgraph package. They don't look like heavy integration tests, worth putting into
systest.
I didn't realize that's what systest was for. I'll move them out.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @codexnull and @manishrjain)
worker/mutation.go
Outdated
| @@ -507,6 +503,25 @@ func CommitOverNetwork(ctx context.Context, tc *api.TxnContext) (uint64, error) | |||
| return tctx.CommitTs, nil | |||
| } | |||
|
|
|||
| func (w *grpcWorker) proposeAndWait(ctx context.Context, txnCtx *api.TxnContext, m *pb.Mutations) (*api.TxnContext, error) { | |||
| @@ -507,6 +505,22 @@ func CommitOverNetwork(ctx context.Context, tc *api.TxnContext) (uint64, error) | |||
| return tctx.CommitTs, nil | |||
| } | |||
|
|
|||
| func (w *grpcWorker) proposeAndWait(ctx context.Context, txnCtx *api.TxnContext, | |||
| m *pb.Mutations) error { | |||
There was a problem hiding this comment.
File is not goimports-ed (from goimports)
There was a problem hiding this comment.
Reviewable status: 3 of 12 files reviewed, 6 unresolved discussions (waiting on @manishrjain, @codexnull, and @golangcibot)
worker/mutation.go, line 512 at r4 (raw file):
if Config.StrictMutations { for _, edge := range m.Edges { if typ, err := schema.State().TypeOf(edge.Attr); typ == types.UndefinedID {
Seems like this could return a nil error. Even if TypeOf would always return an error if typ == UndefinedID, it still is more robust to check for err being not nil, then check for typ being undefined (if that's a possibility).
typ, err := ...
if err != nil { ... }
if typ ==
Fixes hypermodeinc#2277. * Add --mutations option. * Check strict mode and schema before allowing mutation. * Remove --nomutations option. * Add tests of strict schema with multi-group cluster. * Refactor out code duplicated in proposeOrSend() and Mutate(). * Log message when server is ready to accept requests. * Check for error instead of predicate type to detect undefined predicates. * Replace nomutations test with mutations_mode test.
Add a "strict" mode to dgraph to allow mutations only on predicates defined in the schema.
This change is