fix(GRAPHQL): Added support for exact index on field having @id directive.

[JatinDev543]

Currently we add hash index on a field of type String! @id by default. And as index exact and hash are not compatible , user can't add exact index on such field and can't take advantage of comparator functions like lt,le,gt,ge.

To allow this we now changing that behavior, i.e. for a field of type String! @id @search(by:[exact]) , we don't generate default hash index and only generate exact index.

---

This change is 

[vmrajas]

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jatindevdg and @pawanrawal)

---

graphql/schema/gqlschema.go, line 1652 at r1 (raw file):

		// If @id directive is applied along with @search, we check if the search has hash as an
		// arg. If it doesn't and there is no exact arg, then we add hash in it.
		if !hasIndex(indexes, "hash") && !hasIndex(indexes, "exact") {

This logic will still give an error if both hash and exact index have been specified in @search. Eg @search (by: [exact, hash]) . Can we also handle this case over here ?

---

graphql/schema/gqlschema.go, line 1660 at r1 (raw file):

func hasIndex(indexes []string, index string) bool {
	for i := range indexes {

for _, index := range indexes .
You may have to change the name of index argument to the function.

---

graphql/schema/schemagen.go, line 661 at r1 (raw file):

							if !hasIndex(indexes, "exact") {
								indexes = append(indexes, "hash")
							}

Should we also be checking for hash over here. I wonder what will happen in case hash is added to search directive. I believe it will get added twice to generated DQL schema.
Example: @id @search(by: [hash])

This could be existing bug.

---

graphql/schema/testdata/schemagen/input/exact-index-on-id-field.graphql, line 1 at r1 (raw file):

# This will add exact index on content field, overwriting the default "hash" index for field of type "String! @id".

Let's try to keep the number of files in schemagen/input as minimum as possible. I believe this change does not warrant a new files of its own. Can you change some existing String! @id in some existing input file to String! @id @search(by: [exact])

[JatinDev543]

Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @id, @pawanrawal, and @vmrajas)

---

graphql/schema/gqlschema.go, line 1652 at r1 (raw file):

Previously, vmrajas wrote…

This logic will still give an error if both hash and exact index have been specified in @search. Eg @search (by: [exact, hash]) . Can we also handle this case over here ?

yeah, that should give an error. Because we can't allow both indexes. But if a user gives only the exact index then he should not get any error .

---

graphql/schema/gqlschema.go, line 1660 at r1 (raw file):

Previously, vmrajas wrote…

for _, index := range indexes .
You may have to change the name of index argument to the function.

changed.

---

graphql/schema/schemagen.go, line 661 at r1 (raw file):

Previously, vmrajas wrote…

Should we also be checking for hash over here. I wonder what will happen in case hash is added to search directive. I believe it will get added twice to generated DQL schema.
Example: @id @search(by: [hash])

This could be existing bug.

no, it won't get added multiple time because we store indexes on dgraph predicate in a map as below.
pred = dgPred{
typ: typStr,
indexes: make(map[string]bool),
upsert: upsertStr,
}

---

graphql/schema/testdata/schemagen/input/exact-index-on-id-field.graphql, line 1 at r1 (raw file):

Previously, vmrajas wrote…

Let's try to keep the number of files in schemagen/input as minimum as possible. I believe this change does not warrant a new files of its own. Can you change some existing String! @id in some existing input file to String! @id @search(by: [exact])

changed existing files.

[danielmai]

Reviewed 5 of 5 files at r1, 4 of 5 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @id, @pawanrawal, and @vmrajas)

[abhimanyusinghgaur]

let's also add a test in dgraph_schemagen_test.yml after this existing test Field with @id directive and a hash arg in search directive generates correct schema. to make sure that the correct dgraph schema is generated for this case too.

Reviewed 1 of 5 files at r1, 3 of 5 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @id, @jatindevdg, @pawanrawal, and @vmrajas)

---

graphql/schema/gqlschema.go, line 1652 at r1 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…

yeah, that should give an error. Because we can't allow both indexes. But if a user gives only the exact index then he should not get any error .

Yeah! if explicitly both hash and exact have been specified we should give an error as is happening currently.

---

graphql/schema/gqlschema.go, line 1659 at r2 (raw file):

}

func hasIndex(indexes []string, indexName string) bool {

we already have a function in x pkg: x.HasString() which does the same thing. Use that in place of this.

---

graphql/schema/gqlschema_test.yml, line 2840 at r2 (raw file):

        f2: String! @id
      }

Let's also add an invalid_schemas test which applies both hash and exact in @search and should give appropriate error.

[vmrajas]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jatindevdg and @pawanrawal)

[vmrajas]

LGTM. With Abhimanyu's requested changes, this should be good to go.

[JatinDev543]

Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @danielmai, @pawanrawal, and @vmrajas)

---

graphql/schema/gqlschema.go, line 1659 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we already have a function in x pkg: x.HasString() which does the same thing. Use that in place of this.

changed.

---

graphql/schema/gqlschema_test.yml, line 2840 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

Let's also add an invalid_schemas test which applies both hash and exact in @search and should give appropriate error.

There is already a test for it.
"Search doesn't allow hash and exact together"

[abhimanyusinghgaur]

Reviewed 3 of 3 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pawanrawal)