From 9f690dfa76e003a4a42e683196ea94ea6f1bdf27 Mon Sep 17 00:00:00 2001 From: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com> Date: Sat, 20 Jun 2020 02:05:41 +0530 Subject: [PATCH] fix(Export/Import): Live load exported schema (#5663) (#5696) This PR fixes the issue where live loader was not able to import the schema exported using export request. It does so, by allowing the alter schema request for pre-defined types to go through if the type in the request is same as the initial internal type. This issue was introduced as a result of #5185. This PR also disallows a pre-defined type from being dropped. Fixes #DGRAPH-1694. (cherry picked from commit 76ae60ba2a6d8dab68c3eef5637f7faa7198e5b2) --- dgraph/cmd/live/load-uids/load_test.go | 59 +++++++++++++++++++++++ dgraph/cmd/live/run.go | 1 + edgraph/server.go | 34 ++++++++----- ee/acl/acl_test.go | 67 +++++++++++++++++++++----- schema/schema.go | 59 ++++++++++++++++++++--- x/keys.go | 25 ++++++++++ 6 files changed, 214 insertions(+), 31 deletions(-) diff --git a/dgraph/cmd/live/load-uids/load_test.go b/dgraph/cmd/live/load-uids/load_test.go index d7e21334d72..e439ddba20b 100644 --- a/dgraph/cmd/live/load-uids/load_test.go +++ b/dgraph/cmd/live/load-uids/load_test.go @@ -24,6 +24,7 @@ import ( "path" "runtime" "testing" + "time" "github.com/dgraph-io/dgo/v200" "github.com/dgraph-io/dgo/v200/protos/api" @@ -41,6 +42,12 @@ var ( dg *dgo.Dgraph ) +const ( + alphaName = "alpha1" + alphaExportPath = alphaName + ":/data/" + alphaName + "/export" + localExportPath = "./export_copy" +) + func checkDifferentUid(t *testing.T, wantMap, gotMap map[string]interface{}) { require.NotEqual(t, gotMap["q"].([]interface{})[0].(map[string]interface{})["uid"], wantMap["q"].([]interface{})[0].(map[string]interface{})["uid"], @@ -171,6 +178,58 @@ func TestLiveLoadRdfUidDiscard(t *testing.T) { checkLoadedData(t, true) } +func TestLiveLoadExportedSchema(t *testing.T) { + testutil.DropAll(t, dg) + + // initiate export + params := &testutil.GraphQLParams{ + Query: ` + mutation { + export(input: {format: "rdf"}) { + response { + code + message + } + } + }`, + } + accessJwt, _ := testutil.GrootHttpLogin("http://" + testutil.SockAddrHttp + "/admin") + resp := testutil.MakeGQLRequestWithAccessJwt(t, params, accessJwt) + require.Nilf(t, resp.Errors, resp.Errors.Error()) + + // wait a bit to be sure export is complete + time.Sleep(time.Second) + + // copy the export files from docker + exportId := copyExportToLocalFs(t) + + // then loading the exported files should work + pipeline := [][]string{ + {testutil.DgraphBinaryPath(), "live", + "--schema", localExportPath + "/" + exportId + "/g01.schema.gz", + "--files", localExportPath + "/" + exportId + "/g01.rdf.gz", + "--encryption_key_file", testDataDir + "/../../../../ee/enc/test-fixtures/enc-key", + "--alpha", alphaService, "--zero", zeroService, "-u", "groot", "-p", "password"}, + } + err := testutil.Pipeline(pipeline) + require.NoError(t, err, "live loading exported schema exited with error") + + // cleanup copied export files + require.NoError(t, os.RemoveAll(localExportPath), "Error removing export copy directory") +} + +func copyExportToLocalFs(t *testing.T) string { + require.NoError(t, os.RemoveAll(localExportPath), "Error removing directory") + require.NoError(t, testutil.DockerCp(alphaExportPath, localExportPath), + "Error copying files from docker container") + + childDirs, err := ioutil.ReadDir(localExportPath) + require.NoError(t, err, "Couldn't read local export copy directory") + require.True(t, len(childDirs) > 0, "Local export copy directory is empty!!!") + + return childDirs[0].Name() +} + func TestMain(m *testing.M) { _, thisFile, _, _ := runtime.Caller(0) testDataDir = path.Dir(thisFile) diff --git a/dgraph/cmd/live/run.go b/dgraph/cmd/live/run.go index 2c6c7d65206..c986776acfe 100644 --- a/dgraph/cmd/live/run.go +++ b/dgraph/cmd/live/run.go @@ -118,6 +118,7 @@ func init() { Run: func(cmd *cobra.Command, args []string) { defer x.StartProfile(Live.Conf).Stop() if err := run(); err != nil { + x.Check2(fmt.Fprintf(os.Stderr, "%s", err.Error())) os.Exit(1) } }, diff --git a/edgraph/server.go b/edgraph/server.go index 98aa327e7e5..15c6b930dc9 100644 --- a/edgraph/server.go +++ b/edgraph/server.go @@ -207,11 +207,10 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er attr = op.DropValue } - // Reserved predicates cannot be dropped. - if x.IsReservedPredicate(attr) { - err := errors.Errorf("predicate %s is reserved and is not allowed to be dropped", - attr) - return empty, err + // Pre-defined predicates cannot be dropped. + if x.IsPreDefinedPredicate(attr) { + return empty, errors.Errorf("predicate %s is pre-defined and is not allowed to be"+ + " dropped", attr) } nq := &api.NQuad{ @@ -235,6 +234,12 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er return empty, errors.Errorf("If DropOp is set to TYPE, DropValue must not be empty") } + // Pre-defined types cannot be dropped. + if x.IsPreDefinedType(op.DropValue) { + return empty, errors.Errorf("type %s is pre-defined and is not allowed to be dropped", + op.DropValue) + } + m.DropOp = pb.Mutations_TYPE m.DropValue = op.DropValue _, err := query.ApplyMutations(ctx, m) @@ -254,10 +259,9 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er for _, update := range result.Preds { // Pre-defined predicates cannot be altered but let the update go through // if the update is equal to the existing one. - if schema.IsPreDefinedPredicateChanged(update.Predicate, update) { - err := errors.Errorf("predicate %s is reserved and is not allowed to be modified", - update.Predicate) - return nil, err + if schema.IsPreDefPredChanged(update) { + return nil, errors.Errorf("predicate %s is pre-defined and is not allowed to be"+ + " modified", update.Predicate) } if err := validatePredName(update.Predicate); err != nil { @@ -276,8 +280,16 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er } for _, typ := range result.Types { - // Users are not allowed to create types in reserved namespace. - if x.IsReservedType(typ.TypeName) { + // Pre-defined types cannot be altered but let the update go through + // if the update is equal to the existing one. + if schema.IsPreDefTypeChanged(typ) { + return nil, errors.Errorf("type %s is pre-defined and is not allowed to be modified", + typ.TypeName) + } + + // Users are not allowed to create types in reserved namespace. But, there are pre-defined + // types for which the update should go through if the update is equal to the existing one. + if x.IsReservedType(typ.TypeName) && !x.IsPreDefinedType(typ.TypeName) { return nil, errors.Errorf("Can't alter type `%s` as it is prefixed with `dgraph.` "+ "which is reserved as the namespace for dgraph's internal types/predicates.", typ.TypeName) diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index b0d6941da26..247268aea13 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -223,14 +223,22 @@ func resetUser(t *testing.T) { glog.Infof("created user") } -func TestReservedPredicates(t *testing.T) { - // This test uses the groot account to ensure that reserved predicates +func TestPreDefinedPredicates(t *testing.T) { + // This test uses the groot account to ensure that pre-defined predicates // cannot be altered even if the permissions allow it. dg1, err := testutil.DgraphClientWithGroot(testutil.SockAddr) - if err != nil { - t.Fatalf("Error while getting a dgraph client: %v", err) - } - alterReservedPredicates(t, dg1) + require.NoError(t, err, "Error while getting a dgraph client") + + alterPreDefinedPredicates(t, dg1) +} + +func TestPreDefinedTypes(t *testing.T) { + // This test uses the groot account to ensure that pre-defined types + // cannot be altered even if the permissions allow it. + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) + require.NoError(t, err, "Error while getting a dgraph client") + + alterPreDefinedTypes(t, dg) } func TestAuthorization(t *testing.T) { @@ -310,11 +318,11 @@ var query = fmt.Sprintf(` }`, predicateToRead, queryAttr) var schemaQuery = "schema {}" -func alterReservedPredicates(t *testing.T, dg *dgo.Dgraph) { +func alterPreDefinedPredicates(t *testing.T, dg *dgo.Dgraph) { ctx := context.Background() // Test that alter requests are allowed if the new update is the same as - // the initial update for a reserved predicate. + // the initial update for a pre-defined predicate. err := dg.Alter(ctx, &api.Operation{ Schema: "dgraph.xid: string @index(exact) @upsert .", }) @@ -325,22 +333,57 @@ func alterReservedPredicates(t *testing.T, dg *dgo.Dgraph) { }) require.Error(t, err) require.Contains(t, err.Error(), - "predicate dgraph.xid is reserved and is not allowed to be modified") + "predicate dgraph.xid is pre-defined and is not allowed to be modified") err = dg.Alter(ctx, &api.Operation{ DropAttr: "dgraph.xid", }) require.Error(t, err) require.Contains(t, err.Error(), - "predicate dgraph.xid is reserved and is not allowed to be dropped") + "predicate dgraph.xid is pre-defined and is not allowed to be dropped") - // Test that reserved predicates act as case-insensitive. + // Test that pre-defined predicates act as case-insensitive. err = dg.Alter(ctx, &api.Operation{ Schema: "dgraph.XID: int .", }) require.Error(t, err) require.Contains(t, err.Error(), - "predicate dgraph.XID is reserved and is not allowed to be modified") + "predicate dgraph.XID is pre-defined and is not allowed to be modified") +} + +func alterPreDefinedTypes(t *testing.T, dg *dgo.Dgraph) { + ctx := context.Background() + + // Test that alter requests are allowed if the new update is the same as + // the initial update for a pre-defined type. + err := dg.Alter(ctx, &api.Operation{ + Schema: ` + type dgraph.type.Group { + dgraph.xid + dgraph.acl.rule + } + `, + }) + require.NoError(t, err) + + err = dg.Alter(ctx, &api.Operation{ + Schema: ` + type dgraph.type.Group { + dgraph.xid + } + `, + }) + require.Error(t, err) + require.Contains(t, err.Error(), + "type dgraph.type.Group is pre-defined and is not allowed to be modified") + + err = dg.Alter(ctx, &api.Operation{ + DropOp: api.Operation_TYPE, + DropValue: "dgraph.type.Group", + }) + require.Error(t, err) + require.Contains(t, err.Error(), + "type dgraph.type.Group is pre-defined and is not allowed to be dropped") } func queryPredicateWithUserAccount(t *testing.T, dg *dgo.Dgraph, shouldFail bool) { diff --git a/schema/schema.go b/schema/schema.go index 925f6070f52..0946d278013 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -533,10 +533,25 @@ func LoadTypesFromDb() error { return nil } -// InitialTypes returns the schema updates to insert at the begining of -// Dgraph's execution. It looks at the schema state to determine which +// InitialTypes returns the type updates to insert at the beginning of +// Dgraph's execution. It looks at the worker options to determine which // types to insert. func InitialTypes() []*pb.TypeUpdate { + return initialTypesInternal(false) +} + +// CompleteInitialTypes returns all the type updates regardless of the worker +// options. This is useful in situations where the worker options are not known +// in advance or it is required to consider all initial pre-defined types. An +// example of such situation is while allowing type updates to go through during +// alter if they are same as existing pre-defined types. This is useful for +// live loading a previously exported schema. +func CompleteInitialTypes() []*pb.TypeUpdate { + return initialTypesInternal(true) +} + +// NOTE: whenever defining a new type here, please also add it in x/keys.go: preDefinedTypeMap +func initialTypesInternal(all bool) []*pb.TypeUpdate { var initialTypes []*pb.TypeUpdate initialTypes = append(initialTypes, &pb.TypeUpdate{ @@ -553,7 +568,7 @@ func InitialTypes() []*pb.TypeUpdate { }, }) - if x.WorkerConfig.AclEnabled { + if all || x.WorkerConfig.AclEnabled { // These type definitions are required for deleteUser and deleteGroup GraphQL API to work // properly. initialTypes = append(initialTypes, &pb.TypeUpdate{ @@ -682,17 +697,18 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { return initialSchema } -// IsPreDefinedPredicateChanged returns true if the initial update for the pre-defined -// predicate pred is different than the passed update. -func IsPreDefinedPredicateChanged(pred string, update *pb.SchemaUpdate) bool { +// IsPreDefPredChanged returns true if the initial update for the pre-defined +// predicate is different than the passed update. +//If the passed update is not a pre-defined predicate then it just returns false. +func IsPreDefPredChanged(update *pb.SchemaUpdate) bool { // Return false for non-pre-defined predicates. - if !x.IsPreDefinedPredicate(pred) { + if !x.IsPreDefinedPredicate(update.Predicate) { return false } initialSchema := CompleteInitialSchema() for _, original := range initialSchema { - if original.Predicate != pred { + if original.Predicate != update.Predicate { continue } return !proto.Equal(original, update) @@ -700,6 +716,33 @@ func IsPreDefinedPredicateChanged(pred string, update *pb.SchemaUpdate) bool { return true } +// IsPreDefTypeChanged returns true if the initial update for the pre-defined +// type is different than the passed update. +// If the passed update is not a pre-defined type than it just returns false. +func IsPreDefTypeChanged(update *pb.TypeUpdate) bool { + // Return false for non-pre-defined types. + if !x.IsPreDefinedType(update.TypeName) { + return false + } + + initialTypes := CompleteInitialTypes() + for _, original := range initialTypes { + if original.TypeName != update.TypeName { + continue + } + if len(original.Fields) != len(update.Fields) { + return true + } + for i, field := range original.Fields { + if field.Predicate != update.Fields[i].Predicate { + return true + } + } + } + + return false +} + func reset() { pstate = new(state) pstate.init() diff --git a/x/keys.go b/x/keys.go index 672b7980280..eb5a9a68560 100644 --- a/x/keys.go +++ b/x/keys.go @@ -552,6 +552,13 @@ var internalPredicateMap = map[string]struct{}{ "uid": {}, } +var preDefinedTypeMap = map[string]struct{}{ + "dgraph.graphql": {}, + "dgraph.type.User": {}, + "dgraph.type.Group": {}, + "dgraph.type.Rule": {}, +} + // IsGraphqlReservedPredicate returns true if it is the predicate is reserved by graphql. func IsGraphqlReservedPredicate(pred string) bool { _, ok := graphqlReservedPredicate[pred] @@ -630,10 +637,28 @@ func IsInternalPredicate(pred string) bool { // We reserve `dgraph.` as the namespace for the types/predicates we may create in future. // So, users are not allowed to create a type under this namespace. // Hence, we should always define internal types under `dgraph.` namespace. +// +// Pre-defined types are subset of reserved types. +// +// When critical, use IsPreDefinedType(typ string) to find out whether the typ was +// actually defined internally or not. func IsReservedType(typ string) bool { return isReservedName(typ) } +// IsPreDefinedType returns true only if the typ has been defined by dgraph internally. +// For example, `dgraph.graphql` or ACL types are defined in the initial internal types. +// +// We reserve `dgraph.` as the namespace for the types/predicates we may create in future. +// So, users are not allowed to create a predicate under this namespace. +// Hence, we should always define internal types under `dgraph.` namespace. +// +// Pre-defined types are subset of reserved types. +func IsPreDefinedType(typ string) bool { + _, ok := preDefinedTypeMap[typ] + return ok +} + // isReservedName returns true if the given name is prefixed with `dgraph.` func isReservedName(name string) bool { return strings.HasPrefix(strings.ToLower(name), "dgraph.")