From 8a650d7a67dd3a668ba2f3b2e34ff33e1d15236c Mon Sep 17 00:00:00 2001 From: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com> Date: Wed, 8 Apr 2020 16:16:31 +0530 Subject: [PATCH] graphql: Fix non-unique schema issue (#5054) Fixes #GRAPHQL-337 During the time, when alpha's admin server was waiting for Dgraph cluster to boot up, if someone sent a GraphQL schema update request, it would create a new node for GQLSchema in Dgraph. This PR resolves this problem. From now on, there will only be one node of type `dgraph.graphql` ever in dgraph. --- dgraph/cmd/bulk/systest/test-bulk-schema.sh | 1 + edgraph/server.go | 3 +- graphql/admin/admin.go | 157 +++++++++++------- graphql/admin/schema.go | 65 ++------ graphql/e2e/common/admin.go | 44 +++++ graphql/e2e/common/common.go | 6 +- .../e2e/directives/dgraph_directives_test.go | 8 + graphql/e2e/normal/normal_test.go | 8 + graphql/resolve/resolver.go | 14 ++ schema/schema.go | 10 ++ systest/21million/queries/query-052 | 2 +- systest/backup/filesystem/backup_test.go | 3 +- systest/backup/minio/backup_test.go | 3 +- systest/loader/loader_test.go | 13 +- systest/queries_test.go | 3 + x/keys.go | 1 + x/x.go | 11 +- 17 files changed, 226 insertions(+), 126 deletions(-) diff --git a/dgraph/cmd/bulk/systest/test-bulk-schema.sh b/dgraph/cmd/bulk/systest/test-bulk-schema.sh index 0374f68b897..d16ffdf5b05 100755 --- a/dgraph/cmd/bulk/systest/test-bulk-schema.sh +++ b/dgraph/cmd/bulk/systest/test-bulk-schema.sh @@ -201,6 +201,7 @@ EOF diff <(LC_ALL=C sort all_dbs.out | uniq -c) - < 0 || resp.Data.Len() == 0 { - return nil, resp.Errors - } - - var result struct { - GetGQLSchema *gqlSchema - } - - err := json.Unmarshal(resp.Data.Bytes(), &result) - - return result.GetGQLSchema, err -} - func resolverFactoryWithErrorMsg(msg string) resolve.ResolverFactory { errFunc := func(name string) error { return errors.Errorf(msg, name) } qErr := diff --git a/graphql/admin/schema.go b/graphql/admin/schema.go index 75d8f591e31..a7e31ee7a7d 100644 --- a/graphql/admin/schema.go +++ b/graphql/admin/schema.go @@ -38,14 +38,11 @@ type updateSchemaResolver struct { mutation schema.Mutation - // schema that is generated from the mutation input - newGQLSchema schema.Schema + // dgraph schema that is generated from the mutation input newDgraphSchema string - newSchema gqlSchema // The underlying executor and rewriter that persist the schema into Dgraph as // GraphQL metadata - baseAddRewriter resolve.MutationRewriter baseMutationRewriter resolve.MutationRewriter baseMutationExecutor resolve.MutationExecutor } @@ -53,9 +50,7 @@ type updateSchemaResolver struct { type getSchemaResolver struct { admin *adminServer - gqlQuery schema.Query - baseRewriter resolve.QueryRewriter - baseExecutor resolve.QueryExecutor + gqlQuery schema.Query } type updateGQLSchemaInput struct { @@ -72,32 +67,19 @@ func (asr *updateSchemaResolver) Rewrite( return nil, nil, err } - asr.newSchema.Schema = input.Set.Schema - schHandler, err := schema.NewHandler(asr.newSchema.Schema) + schHandler, err := schema.NewHandler(input.Set.Schema) if err != nil { return nil, nil, err } - - asr.newSchema.GeneratedSchema = schHandler.GQLSchema() - asr.newGQLSchema, err = schema.FromString(asr.newSchema.GeneratedSchema) - if err != nil { - return nil, nil, err - } - asr.newDgraphSchema = schHandler.DGSchema() - if asr.admin.schema.ID == "" { - // There's never been a GraphQL schema in this Dgraph before so rewrite this into - // an add - m.SetArgTo(schema.InputArgName, map[string]interface{}{"schema": asr.newSchema.Schema}) - return asr.baseAddRewriter.Rewrite(m) - } - - // there's already a value, just continue with the GraphQL update + // There will always be a graphql schema node present in Dgraph cluster. So, we just need to + // update that node. We will always have its ID present in adminServer, so just need to write a + // filter for that ID. m.SetArgTo(schema.InputArgName, map[string]interface{}{ "filter": map[string]interface{}{"ids": []interface{}{asr.admin.schema.ID}}, - "set": map[string]interface{}{"schema": asr.newSchema.Schema}, + "set": map[string]interface{}{"schema": input.Set.Schema}, }) return asr.baseMutationRewriter.Rewrite(m) } @@ -115,35 +97,17 @@ func (asr *updateSchemaResolver) Mutate( ctx context.Context, query *gql.GraphQuery, mutations []*dgoapi.Mutation) (map[string]string, map[string]interface{}, error) { - - asr.admin.mux.Lock() - defer asr.admin.mux.Unlock() - assigned, result, err := asr.baseMutationExecutor.Mutate(ctx, query, mutations) if err != nil { return nil, nil, err } - if asr.admin.schema.ID == "" { - // should only be 1 assigned, but we don't know the name - for _, v := range assigned { - asr.newSchema.ID = v - } - } else { - asr.newSchema.ID = asr.admin.schema.ID - } - _, err = (&edgraph.Server{}).Alter(ctx, &dgoapi.Operation{Schema: asr.newDgraphSchema}) if err != nil { return nil, nil, schema.GQLWrapf(err, "succeeded in saving GraphQL schema but failed to alter Dgraph schema ") } - asr.admin.resetSchema(asr.newGQLSchema) - asr.admin.schema = asr.newSchema - - glog.Infof("Successfully loaded new GraphQL schema. Serving New GraphQL API.") - return assigned, result, nil } @@ -154,29 +118,20 @@ func (asr *updateSchemaResolver) Query(ctx context.Context, query *gql.GraphQuer func (gsr *getSchemaResolver) Rewrite(ctx context.Context, gqlQuery schema.Query) (*gql.GraphQuery, error) { gsr.gqlQuery = gqlQuery - gqlQuery.Rename("queryGQLSchema") - return gsr.baseRewriter.Rewrite(ctx, gqlQuery) + return nil, nil } func (gsr *getSchemaResolver) Query(ctx context.Context, query *gql.GraphQuery) ([]byte, error) { - if gsr.admin.schema.ID == "" { - return gsr.baseExecutor.Query(ctx, query) - } - return doQuery(gsr.admin.schema, gsr.gqlQuery) } -func doQuery(gql gqlSchema, field schema.Field) ([]byte, error) { +func doQuery(gql *gqlSchema, field schema.Field) ([]byte, error) { var buf bytes.Buffer x.Check2(buf.WriteString(`{ "`)) x.Check2(buf.WriteString(field.ResponseName())) - if gql.ID == "" { - x.Check2(buf.WriteString(`": null }`)) - return buf.Bytes(), nil - } - x.Check2(buf.WriteString(`": [{`)) + for i, sel := range field.SelectionSet() { var val []byte var err error diff --git a/graphql/e2e/common/admin.go b/graphql/e2e/common/admin.go index 4a96d32ffa5..997e66bac72 100644 --- a/graphql/e2e/common/admin.go +++ b/graphql/e2e/common/admin.go @@ -44,6 +44,15 @@ const ( "predicate": "dgraph.graphql.schema", "type": "string" }, + { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + }, { "predicate": "dgraph.type", "type": "string", @@ -59,6 +68,8 @@ const ( "fields": [ { "name": "dgraph.graphql.schema" + },{ + "name": "dgraph.graphql.xid" } ], "name": "dgraph.graphql" @@ -80,6 +91,15 @@ const ( "predicate": "dgraph.graphql.schema", "type": "string" }, + { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + }, { "predicate": "dgraph.type", "type": "string", @@ -103,6 +123,8 @@ const ( "fields": [ { "name": "dgraph.graphql.schema" + },{ + "name": "dgraph.graphql.xid" } ], "name": "dgraph.graphql" @@ -139,6 +161,15 @@ const ( "predicate": "dgraph.graphql.schema", "type": "string" }, + { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + }, { "predicate": "dgraph.type", "type": "string", @@ -165,6 +196,8 @@ const ( "fields": [ { "name": "dgraph.graphql.schema" + },{ + "name": "dgraph.graphql.xid" } ], "name": "dgraph.graphql" @@ -209,6 +242,15 @@ const ( "predicate": "dgraph.graphql.schema", "type": "string" }, + { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + }, { "predicate": "dgraph.type", "type": "string", @@ -238,6 +280,8 @@ const ( "fields": [ { "name": "dgraph.graphql.schema" + },{ + "name": "dgraph.graphql.xid" } ], "name": "dgraph.graphql" diff --git a/graphql/e2e/common/common.go b/graphql/e2e/common/common.go index c373aaa07bd..a6378a88239 100644 --- a/graphql/e2e/common/common.go +++ b/graphql/e2e/common/common.go @@ -633,7 +633,7 @@ func checkGraphQLStarted(url string) error { func hasCurrentGraphQLSchema(url string) (bool, error) { schemaQry := &GraphQLParams{ - Query: `query { getGQLSchema { id } }`, + Query: `query { getGQLSchema { schema } }`, } req, err := schemaQry.createGQLPost(url) if err != nil { @@ -657,7 +657,7 @@ func hasCurrentGraphQLSchema(url string) (bool, error) { var sch struct { GetGQLSchema struct { - ID string + Schema string } } @@ -666,7 +666,7 @@ func hasCurrentGraphQLSchema(url string) (bool, error) { return false, errors.Wrap(err, "error trying to unmarshal GraphQL query result") } - if sch.GetGQLSchema.ID == "" { + if sch.GetGQLSchema.Schema == "" { return false, nil } diff --git a/graphql/e2e/directives/dgraph_directives_test.go b/graphql/e2e/directives/dgraph_directives_test.go index 1e99418833c..5099756be70 100644 --- a/graphql/e2e/directives/dgraph_directives_test.go +++ b/graphql/e2e/directives/dgraph_directives_test.go @@ -109,6 +109,12 @@ func TestSchema_WithDgraphDirectives(t *testing.T) { }, { "predicate": "dgraph.graphql.schema", "type": "string" + }, { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": ["exact"], + "upsert": true }, { "predicate": "dgraph.topic", "type": "string", @@ -287,6 +293,8 @@ func TestSchema_WithDgraphDirectives(t *testing.T) { }, { "fields": [{ "name": "dgraph.graphql.schema" + }, { + "name": "dgraph.graphql.xid" }], "name": "dgraph.graphql" }, { diff --git a/graphql/e2e/normal/normal_test.go b/graphql/e2e/normal/normal_test.go index f71cff4be1d..e49ef698312 100644 --- a/graphql/e2e/normal/normal_test.go +++ b/graphql/e2e/normal/normal_test.go @@ -205,6 +205,12 @@ func TestSchema_Normal(t *testing.T) { }, { "predicate": "dgraph.graphql.schema", "type": "string" + }, { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": ["exact"], + "upsert": true }, { "predicate": "dgraph.type", "type": "string", @@ -363,6 +369,8 @@ func TestSchema_Normal(t *testing.T) { }, { "fields": [{ "name": "dgraph.graphql.schema" + }, { + "name": "dgraph.graphql.xid" }], "name": "dgraph.graphql" }] diff --git a/graphql/resolve/resolver.go b/graphql/resolve/resolver.go index 8fcb46456d6..256822a1c24 100644 --- a/graphql/resolve/resolver.go +++ b/graphql/resolve/resolver.go @@ -153,6 +153,10 @@ func AdminQueryExecutor() QueryExecutor { return &adminExecutor{} } +func AdminMutationExecutor() MutationExecutor { + return &adminExecutor{} +} + // DgraphAsMutationExecutor builds a MutationExecutor. func DgraphAsMutationExecutor() MutationExecutor { return &dgraphExecutor{} @@ -163,6 +167,16 @@ func (de *adminExecutor) Query(ctx context.Context, query *gql.GraphQuery) ([]by return dgraph.Query(ctx, query) } +// Mutates the queries/mutations given and returns a map of new nodes assigned and result of the +// performed queries/mutations +func (de *adminExecutor) Mutate( + ctx context.Context, + query *gql.GraphQuery, + mutations []*dgoapi.Mutation) (map[string]string, map[string]interface{}, error) { + ctx = context.WithValue(ctx, edgraph.Authorize, false) + return dgraph.Mutate(ctx, query, mutations) +} + func (de *dgraphExecutor) Query(ctx context.Context, query *gql.GraphQuery) ([]byte, error) { return dgraph.Query(ctx, query) } diff --git a/schema/schema.go b/schema/schema.go index 9dde2ff3ad9..8d9892aaf75 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -534,6 +534,10 @@ func InitialTypes() []*pb.TypeUpdate { Predicate: "dgraph.graphql.schema", ValueType: pb.Posting_STRING, }, + { + Predicate: "dgraph.graphql.xid", + ValueType: pb.Posting_STRING, + }, }, }) @@ -616,6 +620,12 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { }, &pb.SchemaUpdate{ Predicate: "dgraph.graphql.schema", ValueType: pb.Posting_STRING, + }, &pb.SchemaUpdate{ + Predicate: "dgraph.graphql.xid", + ValueType: pb.Posting_STRING, + Directive: pb.SchemaUpdate_INDEX, + Tokenizer: []string{"exact"}, + Upsert: true, }) if all || x.WorkerConfig.AclEnabled { diff --git a/systest/21million/queries/query-052 b/systest/21million/queries/query-052 index 1f8f809daca..02b1de147f2 100644 --- a/systest/21million/queries/query-052 +++ b/systest/21million/queries/query-052 @@ -7,7 +7,7 @@ { "q": [ { - "count": 3508686 + "count": 3508687 } ] } diff --git a/systest/backup/filesystem/backup_test.go b/systest/backup/filesystem/backup_test.go index b7bdb9fff7d..3739b70800e 100644 --- a/systest/backup/filesystem/backup_test.go +++ b/systest/backup/filesystem/backup_test.go @@ -282,7 +282,8 @@ func runRestore(t *testing.T, backupLocation, lastDir string, commitTs uint64) m restoredPreds, err := testutil.GetPredicateNames(pdir, commitTs) require.NoError(t, err) - require.ElementsMatch(t, []string{"dgraph.graphql.schema", "dgraph.type", "movie"}, restoredPreds) + require.ElementsMatch(t, []string{"dgraph.graphql.schema", "dgraph.graphql.xid", "dgraph.type", + "movie"}, restoredPreds) restoredTypes, err := testutil.GetTypeNames(pdir, commitTs) require.NoError(t, err) diff --git a/systest/backup/minio/backup_test.go b/systest/backup/minio/backup_test.go index 3f7a7dc19e9..02b708fbbf0 100644 --- a/systest/backup/minio/backup_test.go +++ b/systest/backup/minio/backup_test.go @@ -289,7 +289,8 @@ func runRestore(t *testing.T, lastDir string, commitTs uint64) map[string]string restoredPreds, err := testutil.GetPredicateNames(pdir, commitTs) require.NoError(t, err) - require.ElementsMatch(t, []string{"dgraph.graphql.schema", "dgraph.type", "movie"}, restoredPreds) + require.ElementsMatch(t, []string{"dgraph.graphql.schema", "dgraph.graphql.xid", "dgraph.type", + "movie"}, restoredPreds) restoredTypes, err := testutil.GetTypeNames(pdir, commitTs) require.NoError(t, err) diff --git a/systest/loader/loader_test.go b/systest/loader/loader_test.go index 20567bc6eb6..2841926f84b 100644 --- a/systest/loader/loader_test.go +++ b/systest/loader/loader_test.go @@ -107,11 +107,14 @@ func TestLoaderXidmap(t *testing.T) { out, err := exec.Command("sh", "-c", cmd).Output() require.NoError(t, err) - expected = `<0x1> "13" . -<0x1> <0x2711> . -<0x1> "Wonderland" . -<0x1> "Alice" . -<0x2711> "Bob" . + expected = `<0x1> ""^^ . +<0x1> "dgraph.graphql.schema"^^ . +<0x1> "dgraph.graphql"^^ . +<0x2712> "Bob" . +<0x2> "13" . +<0x2> <0x2712> . +<0x2> "Wonderland" . +<0x2> "Alice" . ` require.Equal(t, expected, string(out)) } diff --git a/systest/queries_test.go b/systest/queries_test.go index bbf0f359653..b15f846b94c 100644 --- a/systest/queries_test.go +++ b/systest/queries_test.go @@ -406,6 +406,9 @@ func SchemaQueryTestPredicate1(t *testing.T, c *dgo.Dgraph) { { "predicate": "dgraph.graphql.schema" }, + { + "predicate": "dgraph.graphql.xid" + }, { "predicate": "dgraph.user.group" }, diff --git a/x/keys.go b/x/keys.go index 9f725d4dcce..fd800c56c64 100644 --- a/x/keys.go +++ b/x/keys.go @@ -539,6 +539,7 @@ var aclPredicateMap = map[string]struct{}{ } var graphqlReservedPredicate = map[string]struct{}{ + "dgraph.graphql.xid": {}, "dgraph.graphql.schema": {}, } diff --git a/x/x.go b/x/x.go index 6a8d1f2f265..431ee4a1464 100644 --- a/x/x.go +++ b/x/x.go @@ -121,10 +121,12 @@ const ( ` InitialTypes = ` - "types": [{"fields": [{"name": "dgraph.graphql.schema"}],"name": "dgraph.graphql"}, +"types": [ +{"fields":[{"name":"dgraph.graphql.schema"},{"name":"dgraph.graphql.xid"}],"name":"dgraph.graphql"}, {"fields": [{"name": "dgraph.password"},{"name": "dgraph.xid"},{"name": "dgraph.user.group"}],"name": "User"}, {"fields": [{"name": "dgraph.acl.rule"},{"name": "dgraph.xid"}],"name": "Group"}, -{"fields": [{"name": "dgraph.rule.predicate"},{"name": "dgraph.rule.permission"}],"name": "Rule"}]` +{"fields": [{"name": "dgraph.rule.predicate"},{"name": "dgraph.rule.permission"}],"name": "Rule"} +]` // GroupIdFileName is the name of the file storing the ID of the group to which // the data in a postings directory belongs. This ID is used to join the proper @@ -133,7 +135,10 @@ const ( GroupIdFileName = "group_id" // GraphqlPredicates is the json representation of the predicate reserved for graphql system. - GraphqlPredicates = `{"predicate":"dgraph.graphql.schema", "type": "string"}` + GraphqlPredicates = ` +{"predicate":"dgraph.graphql.schema", "type": "string"}, +{"predicate":"dgraph.graphql.xid","type":"string","index":true,"tokenizer":["exact"],"upsert":true} +` ) var (