[Breaking] chore(GraphQL): Remove dgraph.graphql.p_sha256hash predica…

…te and merge it into dgraph.graphql.p_query (#7451) Fixes GraphQL-1017 We have also defined a new tokenizer on String of type sha256 which concatenates the sha256 along with the query. The first 64 chars represent the sha256 sum which is used to store the token. The second part of the input data is the query for persisted queries. This change would mean that existing persisted queries don't work as expected unless the data for them is migrated.
hypermodeinc · Feb 19, 2021 · 44a51fa · 44a51fa
1 parent 5049092
commit 44a51fa
Show file tree

Hide file tree

Showing 15 changed files with 48 additions and 61 deletions.
diff --git a/dgraph/cmd/bulk/systest/test-bulk-schema.sh b/dgraph/cmd/bulk/systest/test-bulk-schema.sh
@@ -203,7 +203,6 @@ EOF
       1 dgraph.cors
       1 dgraph.drop.op
       1 dgraph.graphql.p_query
-      1 dgraph.graphql.p_sha256hash
       1 dgraph.graphql.schema
       1 dgraph.graphql.schema_created_at
       1 dgraph.graphql.schema_history

diff --git a/edgraph/graphql.go b/edgraph/graphql.go
@@ -50,13 +50,15 @@ func ProcessPersistedQuery(ctx context.Context, gqlReq *schema.Request) error {
 		return nil
 	}
 
-	queryForSHA := `query Me($sha: string){
-						me(func: eq(dgraph.graphql.p_sha256hash, $sha)){
+	join := sha256Hash + query
+
+	queryForSHA := `query Me($join: string){
+						me(func: eq(dgraph.graphql.p_query, $join)){
 							dgraph.graphql.p_query
 						}
 					}`
 	variables := map[string]string{
-		"$sha": sha256Hash,
+		"$join": join,
 	}
 	req := &Request{
 		req: &api.Request{
@@ -105,12 +107,7 @@ func ProcessPersistedQuery(ctx context.Context, gqlReq *schema.Request) error {
 							{
 								Subject:     "_:a",
 								Predicate:   "dgraph.graphql.p_query",
-								ObjectValue: &api.Value{Val: &api.Value_StrVal{StrVal: query}},
-							},
-							{
-								Subject:     "_:a",
-								Predicate:   "dgraph.graphql.p_sha256hash",
-								ObjectValue: &api.Value{Val: &api.Value_StrVal{StrVal: sha256Hash}},
+								ObjectValue: &api.Value{Val: &api.Value_StrVal{StrVal: join}},
 							},
 							{
 								Subject:   "_:a",
@@ -137,11 +134,16 @@ func ProcessPersistedQuery(ctx context.Context, gqlReq *schema.Request) error {
 		return fmt.Errorf("same sha returned %d queries", len(shaQueryRes.Me))
 	}
 
-	if len(query) > 0 && shaQueryRes.Me[0].PersistedQuery != query {
+	gotQuery := ""
+	if len(shaQueryRes.Me[0].PersistedQuery) >= 64 {
+		gotQuery = shaQueryRes.Me[0].PersistedQuery[64:]
+	}
+
+	if len(query) > 0 && gotQuery != query {
 		return errors.New("query does not match persisted query")
 	}
 
-	gqlReq.Query = shaQueryRes.Me[0].PersistedQuery
+	gqlReq.Query = gotQuery
 	return nil
 
 }

diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go
@@ -2171,13 +2171,9 @@ func TestSchemaQueryWithACL(t *testing.T) {
 	},
 	{
 		"predicate":"dgraph.graphql.p_query",
-		"type":"string"
-	},
-	{
-		"predicate":"dgraph.graphql.p_sha256hash",
 		"type":"string",
 		"index":true,
-		"tokenizer":["exact"]
+		"tokenizer":["sha256"]
 	},
     {
       "predicate": "dgraph.graphql.schema",
@@ -2250,9 +2246,6 @@ func TestSchemaQueryWithACL(t *testing.T) {
 		"fields": [
 			{
 				"name": "dgraph.graphql.p_query"
-			},
-			{
-				"name": "dgraph.graphql.p_sha256hash"
 			}
 		],
 		"name": "dgraph.graphql.persisted_query"

diff --git a/graphql/e2e/directives/schema_response.json b/graphql/e2e/directives/schema_response.json
@@ -493,14 +493,10 @@
     },
     {
       "predicate": "dgraph.graphql.p_query",
-      "type": "string"
-    },
-    {
-      "predicate": "dgraph.graphql.p_sha256hash",
       "type": "string",
       "index": true,
       "tokenizer": [
-        "exact"
+        "sha256"
       ]
     },
     {
@@ -1222,9 +1218,6 @@
       "fields": [
         {
           "name": "dgraph.graphql.p_query"
-        },
-        {
-          "name": "dgraph.graphql.p_sha256hash"
         }
       ],
       "name": "dgraph.graphql.persisted_query"

diff --git a/graphql/e2e/normal/schema_response.json b/graphql/e2e/normal/schema_response.json
@@ -666,14 +666,10 @@
     },
     {
       "predicate": "dgraph.graphql.p_query",
-      "type": "string"
-    },
-    {
-      "predicate": "dgraph.graphql.p_sha256hash",
       "type": "string",
       "index": true,
       "tokenizer": [
-        "exact"
+        "sha256"
       ]
     },
     {
@@ -1324,9 +1320,6 @@
       "fields": [
         {
           "name": "dgraph.graphql.p_query"
-        },
-        {
-          "name": "dgraph.graphql.p_sha256hash"
         }
       ],
       "name": "dgraph.graphql.persisted_query"

diff --git a/schema/schema.go b/schema/schema.go
@@ -572,9 +572,6 @@ func initialTypesInternal(namespace uint64, all bool) []*pb.TypeUpdate {
 				{
 					Predicate: "dgraph.graphql.p_query",
 					ValueType: pb.Posting_STRING,
-				}, {
-					Predicate: "dgraph.graphql.p_sha256hash",
-					ValueType: pb.Posting_STRING,
 				},
 			},
 		})
@@ -677,11 +674,8 @@ func initialSchemaInternal(namespace uint64, all bool) []*pb.SchemaUpdate {
 		}, &pb.SchemaUpdate{
 			Predicate: "dgraph.graphql.p_query",
 			ValueType: pb.Posting_STRING,
-		}, &pb.SchemaUpdate{
-			Predicate: "dgraph.graphql.p_sha256hash",
-			ValueType: pb.Posting_STRING,
 			Directive: pb.SchemaUpdate_INDEX,
-			Tokenizer: []string{"exact"},
+			Tokenizer: []string{"sha256"},
 		})
 
 	if all || x.WorkerConfig.AclEnabled {

diff --git a/systest/backup/encryption/backup_test.go b/systest/backup/encryption/backup_test.go
@@ -329,7 +329,7 @@ func runRestore(t *testing.T, lastDir string, commitTs uint64) map[string]string
 	restoredPreds, err := testutil.GetPredicateNames(pdir)
 	require.NoError(t, err)
 	require.ElementsMatch(t, []string{"dgraph.graphql.schema", "dgraph.graphql.xid", "dgraph.type",
-		"movie", "dgraph.graphql.p_query", "dgraph.graphql.p_sha256hash", "dgraph.drop.op"},
+		"movie", "dgraph.graphql.p_query", "dgraph.drop.op"},
 		restoredPreds)
 
 	restoredTypes, err := testutil.GetTypeNames(pdir)

diff --git a/systest/backup/filesystem/backup_test.go b/systest/backup/filesystem/backup_test.go
@@ -191,7 +191,7 @@ func TestBackupFilesystem(t *testing.T) {
 	// Check the predicates and types in the schema are as expected.
 	// TODO: refactor tests so that minio and filesystem tests share most of their logic.
 	preds := []string{"dgraph.graphql.schema", "name", "dgraph.graphql.xid", "dgraph.type",
-		"movie", "dgraph.graphql.p_query", "dgraph.graphql.p_sha256hash", "dgraph.drop.op"}
+		"movie", "dgraph.graphql.p_query", "dgraph.drop.op"}
 	types := []string{"Node", "dgraph.graphql", "dgraph.graphql.persisted_query"}
 	testutil.CheckSchema(t, preds, types)
 

diff --git a/systest/backup/filesystem/data/to_restore/dgraph.20201125.173944.587/manifest.json b/systest/backup/filesystem/data/to_restore/dgraph.20201125.173944.587/manifest.json
@@ -1 +1 @@
-{"type":"full","since":68,"groups":{"1":["dgraph.graphql.p_sha256hash","Author.pwd","dgraph.graphql.xid","dgraph.drop.op","dgraph.graphql.schema_history","dgraph.graphql.schema_created_at","dgraph.type","dgraph.graphql.schema","dgraph.cors","dgraph.graphql.p_query","Author.name","Person.name"],"2":[],"3":[]},"backup_id":"loving_jones5","backup_num":1,"encrypted":false,"drop_operations":null}
+{"type":"full","since":68,"groups":{"1":["Author.pwd","dgraph.graphql.xid","dgraph.drop.op","dgraph.graphql.schema_history","dgraph.graphql.schema_created_at","dgraph.type","dgraph.graphql.schema","dgraph.cors","dgraph.graphql.p_query","Author.name","Person.name"],"2":[],"3":[]},"backup_id":"loving_jones5","backup_num":1,"encrypted":false,"drop_operations":null}
diff --git a/systest/backup/minio/backup_test.go b/systest/backup/minio/backup_test.go
@@ -123,7 +123,7 @@ func TestBackupMinio(t *testing.T) {
 	// Check the predicates and types in the schema are as expected.
 	// TODO: refactor tests so that minio and filesystem tests share most of their logic.
 	preds := []string{"dgraph.graphql.schema", "dgraph.graphql.xid", "dgraph.type", "movie",
-		"dgraph.graphql.p_query", "dgraph.graphql.p_sha256hash", "dgraph.drop.op"}
+		"dgraph.graphql.p_query", "dgraph.drop.op"}
 	types := []string{"Node", "dgraph.graphql", "dgraph.graphql.persisted_query"}
 	testutil.CheckSchema(t, preds, types)
 

diff --git a/systest/export/export_test.go b/systest/export/export_test.go
@@ -81,8 +81,7 @@ var expectedSchema = `[0x0] <movie>:string .` + " " + `
 [0x0] <dgraph.drop.op>:string .` + " " + `
 [0x0] <dgraph.graphql.xid>:string @index(exact) @upsert .` + " " + `
 [0x0] <dgraph.graphql.schema>:string .` + " " + `
-[0x0] <dgraph.graphql.p_query>:string .` + " " + `
-[0x0] <dgraph.graphql.p_sha256hash>:string @index(exact) .` + " " + `
+[0x0] <dgraph.graphql.p_query>:string @index(sha256) .` + " " + `
 [0x0] type <Node> {
 	movie
 }
@@ -92,7 +91,6 @@ var expectedSchema = `[0x0] <movie>:string .` + " " + `
 }
 [0x0] type <dgraph.graphql.persisted_query> {
 	dgraph.graphql.p_query
-	dgraph.graphql.p_sha256hash
 }
 `
 

diff --git a/systest/queries_test.go b/systest/queries_test.go
@@ -378,9 +378,6 @@ func SchemaQueryTestPredicate1(t *testing.T, c *dgo.Dgraph) {
 	  {
 		"predicate": "dgraph.graphql.p_query"
 	  },
-	  {
-		"predicate": "dgraph.graphql.p_sha256hash"
-	  },
       {
         "predicate": "dgraph.xid"
 	  },

diff --git a/testutil/schema.go b/testutil/schema.go
@@ -37,8 +37,7 @@ const (
 	otherInternalPreds = `
 {"predicate":"dgraph.type","type":"string","index":true,"tokenizer":["exact"],"list":true},
 {"predicate":"dgraph.drop.op", "type": "string"},
-{"predicate":"dgraph.graphql.p_query","type":"string"},
-{"predicate":"dgraph.graphql.p_sha256hash","type":"string","index":true,"tokenizer":["exact"]},
+{"predicate":"dgraph.graphql.p_query","type":"string","index":true,"tokenizer":["sha256"]},
 {"predicate":"dgraph.graphql.schema", "type": "string"},
 {"predicate":"dgraph.graphql.xid","type":"string","index":true,"tokenizer":["exact"],"upsert":true}
 `
@@ -59,7 +58,7 @@ const (
 	"fields": [{"name": "dgraph.graphql.schema"},{"name": "dgraph.graphql.xid"}],
 	"name": "dgraph.graphql"
 },{
-	"fields": [{"name": "dgraph.graphql.p_query"},{"name": "dgraph.graphql.p_sha256hash"}],
+	"fields": [{"name": "dgraph.graphql.p_query"}],
 	"name": "dgraph.graphql.persisted_query"
 }
 `

diff --git a/tok/tok.go b/tok/tok.go
@@ -52,6 +52,7 @@ const (
 	IdentBool      = 0x9
 	IdentTrigram   = 0xA
 	IdentHash      = 0xB
+	IdentSha       = 0xC
 	IdentCustom    = 0x80
 	IdentDelimiter = 0x1f // ASCII 31 - Unit seperator
 )
@@ -99,6 +100,7 @@ func init() {
 	registerTokenizer(HashTokenizer{})
 	registerTokenizer(TermTokenizer{})
 	registerTokenizer(FullTextTokenizer{})
+	registerTokenizer(Sha256Tokenizer{})
 	setupBleve()
 }
 
@@ -376,6 +378,24 @@ func (t FullTextTokenizer) Identifier() byte { return IdentFullText }
 func (t FullTextTokenizer) IsSortable() bool { return false }
 func (t FullTextTokenizer) IsLossy() bool    { return true }
 
+// Sha256Tokenizer generates tokens for the sha256 hash part from string data.
+type Sha256Tokenizer struct{ text string }
+
+func (t Sha256Tokenizer) Name() string { return "sha256" }
+func (t Sha256Tokenizer) Type() string { return "string" }
+func (t Sha256Tokenizer) Tokens(v interface{}) ([]string, error) {
+	str, ok := v.(string)
+	if !ok || len(str) < 64 {
+		return []string{}, nil
+	}
+	// The first 64 characters contain the sha256 for a query in hex format so
+	// lets extract and index that part.
+	return []string{str[:64]}, nil
+}
+func (t Sha256Tokenizer) Identifier() byte { return IdentSha }
+func (t Sha256Tokenizer) IsSortable() bool { return false }
+func (t Sha256Tokenizer) IsLossy() bool    { return false }
+
 // BoolTokenizer returns tokens from boolean data.
 type BoolTokenizer struct{}
 

diff --git a/x/keys.go b/x/keys.go
@@ -656,11 +656,10 @@ var aclPredicateMap = map[string]struct{}{
 // predicates, but for all those which are PreDefined and whose value is not allowed to be mutated
 // by users. When renaming this also rename the IsGraphql context key in edgraph/server.go.
 var graphqlReservedPredicate = map[string]struct{}{
-	"dgraph.graphql.xid":          {},
-	"dgraph.graphql.schema":       {},
-	"dgraph.drop.op":              {},
-	"dgraph.graphql.p_query":      {},
-	"dgraph.graphql.p_sha256hash": {},
+	"dgraph.graphql.xid":     {},
+	"dgraph.graphql.schema":  {},
+	"dgraph.drop.op":         {},
+	"dgraph.graphql.p_query": {},
 }
 
 // internalPredicateMap stores a set of Dgraph's internal predicate. An internal
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		{"type":"full","since":68,"groups":{"1":["dgraph.graphql.p_sha256hash","Author.pwd","dgraph.graphql.xid","dgraph.drop.op","dgraph.graphql.schema_history","dgraph.graphql.schema_created_at","dgraph.type","dgraph.graphql.schema","dgraph.cors","dgraph.graphql.p_query","Author.name","Person.name"],"2":[],"3":[]},"backup_id":"loving_jones5","backup_num":1,"encrypted":false,"drop_operations":null}
		{"type":"full","since":68,"groups":{"1":["Author.pwd","dgraph.graphql.xid","dgraph.drop.op","dgraph.graphql.schema_history","dgraph.graphql.schema_created_at","dgraph.type","dgraph.graphql.schema","dgraph.cors","dgraph.graphql.p_query","Author.name","Person.name"],"2":[],"3":[]},"backup_id":"loving_jones5","backup_num":1,"encrypted":false,"drop_operations":null}
-Original file line number
+Diff line change
@@ Expand Up @@
     	  {
     		"predicate": "dgraph.graphql.p_query"
     	  },
-    	  {
-    		"predicate": "dgraph.graphql.p_sha256hash"
-    	  },
           {
             "predicate": "dgraph.xid"
     	  },
@@ Expand Down @@