Skip to content

Commit

Permalink
Fix(GraphQL): This PR fix multi cors and multi schema nodes issue by …
Browse files Browse the repository at this point in the history
…selecting one of the latest added nodes, and add dgraph type to cors. (#7270) (#7302)

* Fix(GraphQL): This PR fix multi cors and multi schema nodes issue by selecting one of the latest added nodes, and add dgraph type to cors. (#7270)

Fixes GRAPHQL-948
We already fixed the issue which causes dgraph to have multiple schema or cores nodes. But there are some cases in which this bug can occur again, for example, someone restored a backup that was taken before the fix. This bug continuously fill the logs on alpha and blocks graphql layer. Then we can't do mutate/query or add schema.

Now, we are changing this behavior, if we have multiple schema or cores nodes then we will select the one which was added last.
In addition to it, we are adding dgraph.type.cors type to to the dgraph.cors node, which allow us to use delete (S * *) type mutation on it, in case we want to delete extra node manually.

(cherry picked from commit fde1031)
JatinDev543 authored Jan 15, 2021
1 parent 74e8b82 commit 32557ec
Showing 15 changed files with 131 additions and 39 deletions.
2 changes: 1 addition & 1 deletion dgraph/cmd/alpha/run.go
Original file line number Diff line number Diff line change
@@ -762,7 +762,7 @@ func run() {
edgraph.ResetCors(updaters)
// Update the accepted cors origins.
for updaters.Ctx().Err() == nil {
origins, err := edgraph.GetCorsOrigins(updaters.Ctx())
_, origins, err := edgraph.GetCorsOrigins(updaters.Ctx())
if err != nil {
glog.Errorf("Error while retrieving cors origins: %s", err.Error())
time.Sleep(time.Second)
54 changes: 42 additions & 12 deletions edgraph/graphql.go
Original file line number Diff line number Diff line change
@@ -23,6 +23,8 @@ import (
"encoding/hex"
"encoding/json"
"fmt"
"github.com/dgraph-io/dgraph/gql"
"sort"
"time"

"github.com/dgraph-io/dgo/v200/protos/api"
@@ -52,6 +54,12 @@ func ResetCors(closer *z.Closer) {
Predicate: "dgraph.cors",
ObjectValue: &api.Value{Val: &api.Value_StrVal{StrVal: "*"}},
},
{
Subject: "_:a",
Predicate: "dgraph.type",
ObjectValue: &api.Value{Val: &api.Value_StrVal{
StrVal: "dgraph.type.cors"}},
},
},
Cond: `@if(eq(len(cors), 0))`,
},
@@ -71,61 +79,83 @@ func ResetCors(closer *z.Closer) {
}
}

func generateNquadsForCors(origins []string) []byte {
func generateNquadsForCors(uid string, origins []string) []byte {
out := &bytes.Buffer{}
for _, origin := range origins {
out.Write([]byte(fmt.Sprintf("uid(cors) <dgraph.cors> \"%s\" . \n", origin)))
out.Write([]byte(fmt.Sprintf("<%s> <dgraph.cors> \"%s\" . \n", uid, origin)))
}
return out.Bytes()
}

// AddCorsOrigins Adds the cors origins to the Dgraph.
func AddCorsOrigins(ctx context.Context, origins []string) error {
uid, _, err := GetCorsOrigins(ctx)
if err != nil {
return err
}
req := &api.Request{
Query: `query{
cors as var(func: has(dgraph.cors))
}`,
Mutations: []*api.Mutation{
{
SetNquads: generateNquadsForCors(origins),
Cond: `@if(eq(len(cors), 1))`,
DelNquads: []byte(`uid(cors) <dgraph.cors> * .`),
SetNquads: generateNquadsForCors(uid, origins),
Cond: `@if(gt(len(cors), 0))`,
DelNquads: []byte(`<` + uid + `>` + ` <dgraph.cors> * .`),
},
},
CommitNow: true,
}
_, err := (&Server{}).doQuery(context.WithValue(ctx, IsGraphql, true), req, NoAuthorize)
_, err = (&Server{}).doQuery(context.WithValue(ctx, IsGraphql, true), req, NoAuthorize)
return err
}

// GetCorsOrigins retrieve all the cors origin from the database.
func GetCorsOrigins(ctx context.Context) ([]string, error) {
func GetCorsOrigins(ctx context.Context) (string, []string, error) {
req := &api.Request{
Query: `query{
me(func: has(dgraph.cors)){
uid
dgraph.cors
}
}`,
ReadOnly: true,
}
res, err := (&Server{}).doQuery(context.WithValue(ctx, IsGraphql, true), req, NoAuthorize)
if err != nil {
return nil, err
return "", nil, err
}

type corsResponse struct {
Me []struct {
Uid string `json:"uid"`
UidInt uint64
DgraphCors []string `json:"dgraph.cors"`
} `json:"me"`
}
corsRes := &corsResponse{}
if err = json.Unmarshal(res.Json, corsRes); err != nil {
return nil, err
return "", nil, err
}
if len(corsRes.Me) != 1 {
return []string{}, fmt.Errorf("GetCorsOrigins returned %d results", len(corsRes.Me))
if len(corsRes.Me) == 0 {
return "", []string{}, fmt.Errorf("GetCorsOrigins returned 0 results")
} else if len(corsRes.Me) == 1 {
return corsRes.Me[0].Uid, corsRes.Me[0].DgraphCors, nil
}
// Multiple nodes for cors found, returning the one that is added last
for i := range corsRes.Me {
iUid, err := gql.ParseUid(corsRes.Me[i].Uid)
if err != nil {
return "", nil, err
}
corsRes.Me[i].UidInt = iUid
}
return corsRes.Me[0].DgraphCors, nil
sort.Slice(corsRes.Me, func(i, j int) bool {
return corsRes.Me[i].UidInt < corsRes.Me[j].UidInt
})
glog.Errorf("Multiple nodes of type dgraph.type.cors found, using the latest one.")
corsLast := corsRes.Me[len(corsRes.Me)-1]
return corsLast.Uid, corsLast.DgraphCors, nil
}

// UpdateSchemaHistory updates graphql schema history.
49 changes: 30 additions & 19 deletions edgraph/server.go
Original file line number Diff line number Diff line change
@@ -101,6 +101,7 @@ type Server struct{}
// graphQLSchemaNode represents the node which contains GraphQL schema
type graphQLSchemaNode struct {
Uid string `json:"uid"`
UidInt uint64
Schema string `json:"dgraph.graphql.schema"`
}

@@ -159,18 +160,32 @@ func GetGQLSchema() (uid, graphQLSchema string, err error) {
if err := json.Unmarshal(resp.GetJson(), &result); err != nil {
return "", "", errors.Wrap(err, "Couldn't unmarshal response from Dgraph query")
}

if len(result.ExistingGQLSchema) == 0 {
res := result.ExistingGQLSchema
if len(res) == 0 {
// no schema has been stored yet in Dgraph
return "", "", nil
} else if len(result.ExistingGQLSchema) == 1 {
} else if len(res) == 1 {
// we found an existing GraphQL schema
gqlSchemaNode := result.ExistingGQLSchema[0]
gqlSchemaNode := res[0]
return gqlSchemaNode.Uid, gqlSchemaNode.Schema, nil
}

// found multiple GraphQL schema nodes, this should never happen
return "", "", worker.ErrMultipleGraphQLSchemaNodes
// returning the schema node which is added last
for i := range res {
iUid, err := gql.ParseUid(res[i].Uid)
if err != nil {
return "", "", err
}
res[i].UidInt = iUid
}

sort.Slice(res, func(i, j int) bool {
return res[i].UidInt < res[j].UidInt
})
glog.Errorf("Multiple schema node found, using the last one")
resLast := res[len(res)-1]
return resLast.Uid, resLast.Schema, nil
}

// UpdateGQLSchema updates the GraphQL and Dgraph schemas using the given inputs.
@@ -393,7 +408,7 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er
}

if len(op.DropAttr) > 0 || op.DropOp == api.Operation_ATTR {
if op.DropOp == api.Operation_ATTR && len(op.DropValue) == 0 {
if op.DropOp == api.Operation_ATTR && op.DropValue == "" {
return empty, errors.Errorf("If DropOp is set to ATTR, DropValue must not be empty")
}

@@ -433,7 +448,7 @@ func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, er
}

if op.DropOp == api.Operation_TYPE {
if len(op.DropValue) == 0 {
if op.DropValue == "" {
return empty, errors.Errorf("If DropOp is set to TYPE, DropValue must not be empty")
}

@@ -607,7 +622,7 @@ func (s *Server) doMutate(ctx context.Context, qc *queryContext, resp *api.Respo
// buildUpsertQuery modifies the query to evaluate the
// @if condition defined in Conditional Upsert.
func buildUpsertQuery(qc *queryContext) string {
if len(qc.req.Query) == 0 || len(qc.gmuList) == 0 {
if qc.req.Query == "" || len(qc.gmuList) == 0 {
return qc.req.Query
}

@@ -652,7 +667,7 @@ func buildUpsertQuery(qc *queryContext) string {
func updateMutations(qc *queryContext) error {
for i, condVar := range qc.condVars {
gmu := qc.gmuList[i]
if len(condVar) != 0 {
if condVar != "" {
uids, ok := qc.uidRes[condVar]
if !(ok && len(uids) == 1) {
gmu.Set = nil
@@ -1039,7 +1054,7 @@ func (s *Server) doQuery(ctx context.Context, req *api.Request, doAuth AuthMode)
}

req.Query = strings.TrimSpace(req.Query)
isQuery := len(req.Query) != 0
isQuery := req.Query != ""
if !isQuery && !isMutation {
span.Annotate(nil, "empty request")
return nil, errors.Errorf("empty request")
@@ -1104,7 +1119,7 @@ func (s *Server) doQuery(ctx context.Context, req *api.Request, doAuth AuthMode)

func processQuery(ctx context.Context, qc *queryContext) (*api.Response, error) {
resp := &api.Response{}
if len(qc.req.Query) == 0 {
if qc.req.Query == "" {
// No query, so make the query cost 0.
resp.Metrics = &api.Metrics{
NumUids: map[string]uint64{"_total": 0},
@@ -1268,7 +1283,7 @@ func parseRequest(qc *queryContext) error {
qc.valRes = make(map[string]map[uint64]types.Val)
upsertQuery = buildUpsertQuery(qc)
needVars = findMutationVars(qc)
if len(upsertQuery) == 0 {
if upsertQuery == "" {
if len(needVars) > 0 {
return errors.Errorf("variables %v not defined", needVars)
}
@@ -1286,11 +1301,7 @@ func parseRequest(qc *queryContext) error {
if err != nil {
return err
}
if err = validateQuery(qc.gqlRes.Query); err != nil {
return err
}

return nil
return validateQuery(qc.gqlRes.Query)
}

func authorizeRequest(ctx context.Context, qc *queryContext) error {
@@ -1381,7 +1392,7 @@ func hasAdminAuth(ctx context.Context, tag string) (net.Addr, error) {
}

func hasPoormansAuth(ctx context.Context) error {
if len(worker.Config.AuthToken) == 0 {
if worker.Config.AuthToken == "" {
return nil
}
md, ok := metadata.FromIncomingContext(ctx)
@@ -1525,7 +1536,7 @@ func validateNQuads(set, del []*api.NQuad, qc *queryContext) error {

func validateKey(key string) error {
switch {
case len(key) == 0:
case key == "":
return errors.Errorf("Has zero length")
case strings.ContainsAny(key, "~@"):
return errors.Errorf("Has invalid characters")
14 changes: 13 additions & 1 deletion ee/acl/acl_test.go
Original file line number Diff line number Diff line change
@@ -196,7 +196,7 @@ func TestPasswordReturn(t *testing.T) {

func TestGetCurrentUser(t *testing.T) {
token := testutil.GrootHttpLogin(adminEndpoint)

currentUser := getCurrentUser(t, token)
currentUser.RequireNoGraphQLErrors(t)
require.Equal(t, string(currentUser.Data), `{"getCurrentUser":{"name":"groot"}}`)
@@ -2299,6 +2299,14 @@ func TestSchemaQueryWithACL(t *testing.T) {
}
],
"name": "dgraph.type.User"
},
{
"fields": [
{
"name": "dgraph.cors"
}
],
"name": "dgraph.type.cors"
}
]
}`
@@ -2337,6 +2345,10 @@ func TestSchemaQueryWithACL(t *testing.T) {
{
"fields": [],
"name": "dgraph.type.User"
},
{
"fields": [],
"name": "dgraph.type.cors"
}
]
}`
2 changes: 1 addition & 1 deletion graphql/admin/cors.go
Original file line number Diff line number Diff line change
@@ -68,7 +68,7 @@ func resolveReplaceAllowedCORSOrigins(ctx context.Context, m schema.Mutation) (*

// resolveGetCors retrieves cors details from the database.
func resolveGetCors(ctx context.Context, q schema.Query) *resolve.Resolved {
origins, err := edgraph.GetCorsOrigins(ctx)
_, origins, err := edgraph.GetCorsOrigins(ctx)
if err != nil {
return resolve.EmptyResult(q, err)
}
8 changes: 8 additions & 0 deletions graphql/e2e/directives/schema_response.json
Original file line number Diff line number Diff line change
@@ -968,6 +968,14 @@
}
],
"name": "test.dgraph.employee.en"
},
{
"fields": [
{
"name": "dgraph.cors"
}
],
"name": "dgraph.type.cors"
}
]
}
8 changes: 8 additions & 0 deletions graphql/e2e/normal/schema_response.json
Original file line number Diff line number Diff line change
@@ -967,6 +967,14 @@
}
],
"name": "post1"
},
{
"fields": [
{
"name": "dgraph.cors"
}
],
"name": "dgraph.type.cors"
}
]
}
8 changes: 8 additions & 0 deletions schema/schema.go
Original file line number Diff line number Diff line change
@@ -588,6 +588,14 @@ func initialTypesInternal(all bool) []*pb.TypeUpdate {
ValueType: pb.Posting_STRING,
},
},
}, &pb.TypeUpdate{
TypeName: "dgraph.type.cors",
Fields: []*pb.SchemaUpdate{
{
Predicate: "dgraph.cors",
ValueType: pb.Posting_STRING,
},
},
})

if all || x.WorkerConfig.AclEnabled {
2 changes: 1 addition & 1 deletion systest/backup/encryption/backup_test.go
Original file line number Diff line number Diff line change
@@ -334,7 +334,7 @@ func runRestore(t *testing.T, lastDir string, commitTs uint64) map[string]string

restoredTypes, err := testutil.GetTypeNames(pdir)
require.NoError(t, err)
require.ElementsMatch(t, []string{"Node", "dgraph.graphql", "dgraph.graphql.history", "dgraph.graphql.persisted_query"}, restoredTypes)
require.ElementsMatch(t, []string{"Node", "dgraph.graphql", "dgraph.graphql.history", "dgraph.graphql.persisted_query", "dgraph.type.cors"}, restoredTypes)

require.NoError(t, err)
t.Logf("--- Restored values: %+v\n", restored)
2 changes: 1 addition & 1 deletion systest/backup/filesystem/backup_test.go
Original file line number Diff line number Diff line change
@@ -195,7 +195,7 @@ func TestBackupFilesystem(t *testing.T) {
preds := []string{"dgraph.graphql.schema", "dgraph.cors", "name", "dgraph.graphql.xid",
"dgraph.type", "movie", "dgraph.graphql.schema_history", "dgraph.graphql.schema_created_at",
"dgraph.graphql.p_query", "dgraph.graphql.p_sha256hash", "dgraph.drop.op"}
types := []string{"Node", "dgraph.graphql", "dgraph.graphql.history", "dgraph.graphql.persisted_query"}
types := []string{"Node", "dgraph.graphql", "dgraph.graphql.history", "dgraph.graphql.persisted_query", "dgraph.type.cors"}
testutil.CheckSchema(t, preds, types)

verifyUids := func(count int) {
2 changes: 1 addition & 1 deletion systest/backup/minio/backup_test.go
Original file line number Diff line number Diff line change
@@ -125,7 +125,7 @@ func TestBackupMinio(t *testing.T) {
preds := []string{"dgraph.graphql.schema", "dgraph.cors", "dgraph.graphql.xid", "dgraph.type", "movie",
"dgraph.graphql.schema_history", "dgraph.graphql.schema_created_at", "dgraph.graphql.p_query",
"dgraph.graphql.p_sha256hash", "dgraph.drop.op"}
types := []string{"Node", "dgraph.graphql", "dgraph.graphql.history", "dgraph.graphql.persisted_query"}
types := []string{"Node", "dgraph.graphql", "dgraph.graphql.history", "dgraph.graphql.persisted_query", "dgraph.type.cors"}
testutil.CheckSchema(t, preds, types)

checks := []struct {
3 changes: 3 additions & 0 deletions systest/export/export_test.go
Original file line number Diff line number Diff line change
@@ -93,6 +93,9 @@ type <dgraph.graphql> {
dgraph.graphql.schema
dgraph.graphql.xid
}
type <dgraph.type.cors> {
dgraph.cors
}
type <dgraph.graphql.history> {
dgraph.graphql.schema_history
dgraph.graphql.schema_created_at
Loading

0 comments on commit 32557ec

Please sign in to comment.