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

JatinDev543 · 2021-01-11T07:49:02Z

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.

This change is

…QL-948

fix minio test

abhimanyusinghgaur

Let's discuss with @pawanrawal if we should use the max node in case we encounter more than one node.

Reviewed 10 of 15 files at r1.
Reviewable status: 10 of 15 files reviewed, 3 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

edgraph/graphql.go, line 150 at r1 (raw file):

glog.Infof("

Errorf
and again I don't feel we should do this. If this happens that means there is some other issue, which we should fix. Or if it happens due to older backups we should have a way to migrate from those which we already have. So, this feels like it should still return an error if we get either 0 or more than 1 node.

edgraph/server.go, line 177 at r1 (raw file):

if uidMax < schNode.Uid {

uid here is a string. Shouldn't we first convert it to int and then compare?
Because, even the difference of A and a can make it go wrong right now.

edgraph/server.go, line 182 at r1 (raw file):

		}
	}
	glog.Infof("Multiple schema node found,returning last added node")

This is supposed to be an error state, so we should use glog.Errorf.

But, it doesn't feel like we should make it return the max node instead of giving an error. Such states should never be achieved and we also haven't seen any evidence for GraphQL schema case where two nodes have been seen after the fixes done long back.
So, I believe that we should still return an error because we need to know if such thing happens, instead of just ignoring it.

JatinDev543 · 2021-01-12T13:08:13Z

Let's discuss with @pawanrawal if we should use the max node in case we encounter more than one node.

Reviewed 10 of 15 files at r1.
Reviewable status: 10 of 15 files reviewed, 3 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

edgraph/graphql.go, line 150 at r1 (raw file):
glog.Infof("
Errorf
and again I don't feel we should do this. If this happens that means there is some other issue, which we should fix. Or if it happens due to older backups we should have a way to migrate from those which we already have. So, this feels like it should still return an error if we get either 0 or more than 1 node.

edgraph/server.go, line 177 at r1 (raw file):
if uidMax < schNode.Uid {
uid here is a string. Shouldn't we first convert it to int and then compare?
Because, even the difference of A and a can make it go wrong right now.

edgraph/server.go, line 182 at r1 (raw file):
		}
	}
	glog.Infof("Multiple schema node found,returning last added node")
This is supposed to be an error state, so we should use glog.Errorf.

But, it doesn't feel like we should make it return the max node instead of giving an error. Such states should never be achieved and we also haven't seen any evidence for GraphQL schema case where two nodes have been seen after the fixes done long back.
So, I believe that we should still return an error because we need to know if such thing happens, instead of just ignoring it.

Changed uid comparison. Also log errors using Errorf.

pawanrawal

Reviewable status: 0 of 15 files reviewed, 8 unresolved discussions (waiting on @jatindevdg, @manishrjain, and @vvbalaji-dgraph)

edgraph/graphql.go, line 150 at r1 (raw file):

		}
	}
	glog.Infof("Multiple nodes of type dgraph.type.cors found,returning last added node")

space after ,

Multiple nodes of type dgraph.type.cors found, using the latest one.

edgraph/graphql.go, line 143 at r2 (raw file):

		return corsRes.Me[0].Uid, corsRes.Me[0].DgraphCors, nil
	}
	cors := corsRes.Me[0].DgraphCors

Add a comment here that if there are multiple nodes for cors we pick the one with the max uid.

edgraph/graphql.go, line 144 at r2 (raw file):

	}
	cors := corsRes.Me[0].DgraphCors
	uidMax := corsRes.Me[0].Uid

maxUid

edgraph/graphql.go, line 145 at r2 (raw file):

	cors := corsRes.Me[0].DgraphCors
	uidMax := corsRes.Me[0].Uid
	uidMaxInt, err := gql.ParseUid(uidMax)

maxUidInt

edgraph/graphql.go, line 150 at r2 (raw file):

	}
	for _, me := range corsRes.Me[1:] {
		cUidInt, err := gql.ParseUid(me.Uid)

sort.Slice(corsRes.Me, func(i, j int) bool {
  // parse the uids into integer and then compare
  return corsRes.Me[i].uidInt < corsResMe[j].uidInt 
})

edgraph/graphql.go, line 155 at r2 (raw file):

		}
		if uidMaxInt < cUidInt {
			uidMaxInt = cUidInt

instead of doing all this why not sort them and then pick the last value?

edgraph/server.go, line 187 at r2 (raw file):

		if uidMaxInt < cUidInt {
			uidMaxInt = cUidInt
			uidMax = schNode.Uid

use sort here

worker/graphql_schema.go, line 109 at r2 (raw file):

		// there seems to be multiple nodes for GraphQL schema,Ideally we should never reach here
		// But if by any bug we reach here then return the schema node which is added last
		uidMax := res.GetUidMatrix()[0].GetUids()[0]

the list should already be sorted but you can anyways sort and just pick last value

JatinDev543

Reviewable status: 0 of 15 files reviewed, 8 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

edgraph/graphql.go, line 150 at r1 (raw file):