feat(GRAPHQL): Add language tag support in GraphQL #7663
Conversation
github-actions
bot
added
the
area/graphql
| "dgraph.type": ["Person"], | ||
| "uid": "_:Person_1" | ||
| } |
There was a problem hiding this comment.
nit: Add a line at end of file.
graphql/resolve/query_test.yaml
Outdated
| query { | ||
| queryPerson { | ||
| name |
| @@ -2208,7 +2209,7 @@ func getNonIDFields(schema *ast.Schema, defn *ast.Definition, providesTypeMap ma | |||
|
|
|||
| // Ignore Fields with @external directives also as they shouldn't be present | |||
| // in the Patch Type also. If the field is an argument to `@provides` directive | |||
| // then it should be presnt. | |||
| // then it should be present. | |||
graphql/schema/gqlschema.go
Outdated
| @@ -2264,7 +2271,10 @@ func getFieldsWithoutIDType(schema *ast.Schema, defn *ast.Definition, providesTy | |||
| if hasCustomOrLambda(fld) { | |||
| continue | |||
| } | |||
|
|
|||
| // see also comment in getNonIDFields | |||
There was a problem hiding this comment.
nit: see the comment in getNonIDFields as well.
graphql/schema/gqlschema.go
Outdated
| @@ -2287,6 +2297,23 @@ func getFieldsWithoutIDType(schema *ast.Schema, defn *ast.Definition, providesTy | |||
| return append(fldList, pd) | |||
| } | |||
|
|
|||
| // This function check if given gql field have multiple language tags | |||
graphql/schema/gqlschema.go
Outdated
| @@ -2246,7 +2252,8 @@ func getNonIDFields(schema *ast.Schema, defn *ast.Definition, providesTypeMap ma | |||
| return append(fldList, pd) | |||
| } | |||
|
|
|||
| func getFieldsWithoutIDType(schema *ast.Schema, defn *ast.Definition, providesTypeMap map[string]bool) ast.FieldList { | |||
| func getFieldsWithoutIDType(schema *ast.Schema, defn *ast.Definition, | |||
| providesTypeMap map[string]bool, addInput bool) ast.FieldList { | |||
There was a problem hiding this comment.
nit: Rename addInput to something more meaningful like isAddingInput or something.
graphql/schema/schemagen.go
Outdated
| @@ -394,7 +394,10 @@ func NewHandler(input string, apolloServiceQuery bool) (Handler, error) { | |||
| } | |||
|
|
|||
| metaInfo.extraCorsHeaders = getAllowedHeaders(sch, defns, authHeader) | |||
| dgSchema := genDgSchema(sch, typesToComplete, providesFieldsMap) | |||
| dgSchema, gqlErr := genDgSchema(sch, typesToComplete, providesFieldsMap) | |||
| if gqlErr != nil { | |||
There was a problem hiding this comment.
I believe the errors are returned in case the provided schema is not valid. Why isn't the validation done as part of postGQLValidation. You may do all the validation inside the function dgraphDirectiveValidation
graphql/schema/schemagen.go
Outdated
| for fname, langTagDgPred := range langTagDgPreds { | ||
| dgPredAndTags := strings.Split(fname, "@") | ||
| unTaggedDgPredName := dgPredAndTags[0] | ||
| tags := dgPredAndTags[1] |
There was a problem hiding this comment.
Chance of panic in case @ comes at the end of Dgraph predicate name. See the above comment.
graphql/schema/schemagen.go
Outdated
| for index := range langTagDgPred.indexes { | ||
| unTaggedDgPred.indexes[index] = true | ||
| } | ||
| dgPreds[unTaggedDgPredName] = unTaggedDgPred |
There was a problem hiding this comment.
Should we also be making one more validation ?
Checking if non-nullability of lang tag predicates and untagged predicates is same. Is it possible that name is of type String but name@hi is of type String!
graphql/e2e/common/query.go
Outdated
| "name_Untag_AnyLang": "Alice" | ||
| } | ||
| ] | ||
| }` |
There was a problem hiding this comment.
Can we also update in this e2e test ?
There was a problem hiding this comment.
Also need to add tests in dgraph_schemagen_test.yml for the dgraph schema being generated for the various possible cases.
Haven't really looked at the code, I have only validated the test cases.
Will review the code too once the tests are in a good shape.
Reviewed 1 of 14 files at r1.
Reviewable status: 1 of 14 files reviewed, 20 unresolved discussions (waiting on @jatindevdg, @pawanrawal, and @vmrajas)
graphql/e2e/common/query.go, line 3969 at r1 (raw file):
Previously, vmrajas wrote…
Can we also update in this e2e test ?
Yup! this needs more test scenarios. At present it has only one person which has values for all the languages, we need to add more persons which don't have values for all the languages. So, we can really test that for example nameHi_Zh_Untag works the way it was supposed to work. We should cover every scenario in testing.
graphql/e2e/directives/schema.graphql, line 213 at r1 (raw file):
} type Person {
seems strange that this type was not here already, was it not being used anywhere in the tests earlier?
also, we should add @search too and use a query that uses the or filter fashion (as I have mentioned in a comment later) to make sure it works e2e in all scenarios.
graphql/resolve/query_test.yaml, line 3391 at r1 (raw file):
gqlquery: | query { queryPerson {
we should also add a filter to this query for all the possible scenarios in an or fashion. i.e., a filter with name == "Alice" or nameZh == "some Chinese" or nameHi == "alice in hindi", like that.
graphql/schema/gqlschema_test.yml, line 2899 at r1 (raw file):
Expected `String` type for language tag field: `nameHi` but got :`Int` inside Type `Person`
Can all the new error messages follow the same convention as existing messages?
Type Person; Field nameHi: Expected `String` type for language tag field but got `Int`.
graphql/schema/gqlschema_test.yml, line 2936 at r1 (raw file):
Quoted 10 lines of code…
- name: "Incompatible exact and hash indexes are not allowed at same time on untagged and tagged languages field " input: | type Person { name: String! @search(by: [exact]) nameHi: String @dgraph(pred:"Person.name@en:hi") @search(by: [hash]) } errlist: [ { "message": "Incompatible indexes hash and exact are not allowed on language tagged and untagged fields, language untagged field: `name` have exact index and language tagged field `nameHi` have hash index inside type: `Person`", "locations": [ { "line": 2, "column": 3 } ] }, ]
I have two concerns with this particular test:
- I don't think one should even be able to put a search on multi-language tagged fields. That would error out in DQL. Example:
{
nodes(func: eq(name@en:hi, "Alice")) {
name@en:hi
}
}
is an invalid DQL query. So, we should disallow search on multi-language fields, but continue it on single language fields.
2. If two different fields, doesn't matter language tagged or not, have hash and exact, then that should not be a problem. The problem arises when a single field has hash and exact because it is a limitation in GraphQL to not be able to represent the eq filter differently for them. But if it is two different fields, then there is no issue. And that is the reason it works with an@id field when exact is on a different language tagged field.
graphql/schema/gqlschema_test.yml, line 2943 at r1 (raw file):
] - name: "Incompatible exact and hash indexes are not allowed at same time on tagged and untagged languages field, when hash index is given explicitly on untagged language field "
same concerns as the above test
graphql/schema/gqlschema_test.yml, line 2954 at r1 (raw file):
] valid_schemas:
also add a valid schema test here
graphql/schema/schemagen.go, line 779 at r1 (raw file):
Previously, vmrajas wrote…
Should we also be making one more validation ?
Checking if non-nullability of lang tag predicates and untagged predicates is same. Is it possible that name is of type String but name@hi is of type String!
I think that is possible. One may want to say that they always want a value in hi but they don't have such hard restriction for an untagged/other lang value. Like, they may not have data to fill untagged/other lang values, and only have data for hi values because their app is mostly targeted towards Indian users.
graphql/schema/testdata/schemagen/input/MultiplelanguageTags.graphql, line 1 at r1 (raw file):
type Person {
- Please rename the input and output filenames to match the conventions in that directory.
- Add a field which doesn't have the @id directive, but has other lang fields.
- Add one more field which is language tagged but doesn't have a corresponding untagged field in the schema.
graphql/schema/testdata/schemagen/output/MultiplelanguageTags.graphql, line 323 at r1 (raw file):
nameHiEn nameHi_En_Untag name_Untag_AnyLang
I think these won't work with has filter. Please check.
graphql/schema/testdata/schemagen/output/MultiplelanguageTags.graphql, line 334 at r1 (raw file):
nameHiEn nameHi_En_Untag name_Untag_AnyLang
can't order by multiple languages
There was a problem hiding this comment.
Reviewable status: 1 of 14 files reviewed, 20 unresolved discussions (waiting on @jatindevdg, @pawanrawal, and @vmrajas)
graphql/e2e/common/query.go, line 3969 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Yup! this needs more test scenarios. At present it has only one person which has values for all the languages, we need to add more persons which don't have values for all the languages. So, we can really test that for example
nameHi_Zh_Untagworks the way it was supposed to work. We should cover every scenario in testing.
Also, test the filter and order in the same query.
There was a problem hiding this comment.
Reviewable status: 0 of 17 files reviewed, 20 unresolved discussions (waiting on @abhimanyusinghgaur, @id, @jatindevdg, @pawanrawal, and @vmrajas)
graphql/e2e/common/query.go, line 3969 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Also, test the filter and order in the same query.
added update test for language tag fields.
added more test cases for different scenarios, also with filters and order.
graphql/e2e/directives/schema.graphql, line 213 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
seems strange that this type was not here already, was it not being used anywhere in the tests earlier?
also, we should add
@searchtoo and use a query that uses theorfilter fashion (as I have mentioned in a comment later) to make sure it works e2e in all scenarios.
i haven't found usage of type person. added test for above scenarios.
graphql/resolve/add_mutation_test.yaml, line 4544 at r1 (raw file):
Previously, vmrajas wrote…
nit: Add a line at end of file.
added
graphql/resolve/query_test.yaml, line 3391 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
we should also add a filter to this query for all the possible scenarios in an
orfashion. i.e., a filter with name == "Alice" or nameZh == "some Chinese" or nameHi == "alice in hindi", like that.
added
graphql/schema/gqlschema.go, line 2256 at r1 (raw file):
Previously, vmrajas wrote…
nit: Rename addInput to something more meaningful like isAddingInput or something.
changed
graphql/schema/gqlschema.go, line 2274 at r1 (raw file):
Previously, vmrajas wrote…
nit: see the comment in getNonIDFields as well.
changed
graphql/schema/gqlschema.go, line 2300 at r1 (raw file):
Previously, vmrajas wrote…
nit: have --> has
changed
graphql/schema/gqlschema.go, line 2307 at r1 (raw file):
Previously, vmrajas wrote…
There is a chance of this panicking if there is a dgraph predicate with name like "somePredicate@" . Can you confirm that @ cannot be at the end of Dgraph predicate or handle this case as well.
it doesn't panic. if splits are of length zero then it return empty string.
graphql/schema/gqlschema_test.yml, line 2899 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Expected `String` type for language tag field: `nameHi` but got :`Int` inside Type `Person`Can all the new error messages follow the same convention as existing messages?
Type Person; Field nameHi: Expected `String` type for language tag field but got `Int`.
changed format of errors
graphql/schema/gqlschema_test.yml, line 2936 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
- name: "Incompatible exact and hash indexes are not allowed at same time on untagged and tagged languages field " input: | type Person { name: String! @search(by: [exact]) nameHi: String @dgraph(pred:"Person.name@en:hi") @search(by: [hash]) } errlist: [ { "message": "Incompatible indexes hash and exact are not allowed on language tagged and untagged fields, language untagged field: `name` have exact index and language tagged field `nameHi` have hash index inside type: `Person`", "locations": [ { "line": 2, "column": 3 } ] }, ]I have two concerns with this particular test:
- I don't think one should even be able to put a search on multi-language tagged fields. That would error out in DQL. Example:
{ nodes(func: eq(name@en:hi, "Alice")) { name@en:hi } }is an invalid DQL query. So, we should disallow search on multi-language fields, but continue it on single language fields.
2. If two different fields, doesn't matter language tagged or not, have hash and exact, then that should not be a problem. The problem arises when a single field hashashandexactbecause it is a limitation in GraphQL to not be able to represent theeqfilter differently for them. But if it is two different fields, then there is no issue. And that is the reason it works with an@idfield when exact is on a different language tagged field.
- we removed multi language fields from filters.
- allowed language taaged and tagged filed to have hash and exact at the same time.
graphql/schema/gqlschema_test.yml, line 2943 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
same concerns as the above test
changed .
graphql/schema/gqlschema_test.yml, line 2954 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
also add a valid schema test here
added.
graphql/schema/schemagen.go, line 398 at r1 (raw file):
Previously, vmrajas wrote…
I believe the errors are returned in case the provided schema is not valid. Why isn't the validation done as part of postGQLValidation. You may do all the validation inside the function dgraphDirectiveValidation
moved schema validation to postGQLValidation and in dgraphDirectiveValidation
graphql/schema/schemagen.go, line 729 at r1 (raw file):
Previously, vmrajas wrote…
Chance of panic in case @ comes at the end of Dgraph predicate name. See the above comment.
it doesn't panic. same reason as above.
graphql/schema/schemagen.go, line 779 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
I think that is possible. One may want to say that they always want a value in
hibut they don't have such hard restriction for an untagged/other lang value. Like, they may not have data to fill untagged/other lang values, and only have data forhivalues because their app is mostly targeted towards Indian users.
no change.
graphql/schema/testdata/schemagen/input/MultiplelanguageTags.graphql, line 1 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
- Please rename the input and output filenames to match the conventions in that directory.
- Add a field which doesn't have the @id directive, but has other lang fields.
- Add one more field which is language tagged but doesn't have a corresponding untagged field in the schema.
changed 1st and added above 2 scenarios.
graphql/schema/testdata/schemagen/output/MultiplelanguageTags.graphql, line 323 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
nameHiEn nameHi_En_Untag name_Untag_AnyLangI think these won't work with has filter. Please check.
yeah, removed.
graphql/schema/testdata/schemagen/output/MultiplelanguageTags.graphql, line 334 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
nameHiEn nameHi_En_Untag name_Untag_AnyLangcan't order by multiple languages
removed.
There was a problem hiding this comment.
Reviewed 18 of 18 files at r2, 6 of 6 files at r3.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @jatindevdg, @pawanrawal, and @vmrajas)
graphql/e2e/common/query.go, line 3926 at r2 (raw file):
}`, } // add three Persons
all three persons can be added in a single mutation, we shouldn't have to execute 3 separate mutations for that.
graphql/e2e/common/query.go, line 4034 at r2 (raw file):
Quoted 10 lines of code…
queryPerson( filter: { or: [ { name: { eq: "Julliet" } } { nameHi: { eq: "जूलियट" } } { nameZh: { eq: "朱丽叶" } } { name_Untag_AnyLang: { eq: "Juliet" } } ] } order: { asc: nameHi }
what I meant was: in a single query, we use an or filter with different lang fields to get all the 3 persons, and not do 3 separate queries for them. Because, right now, order won't have any effect as the query returns only one person. It would really show an effect when the query was returning more than one person.
So:
filter: {
or: [
{name: {eq: "Bob"} }
{nameHi: {eq: "जूलियट"} }
{nameZh: {eq: "<the chinese value of Alice here>"} }
]
}
order: {asc: nameHi}
this should give all the 3 persons, with Juliet as the first person in the results as only Juliet has a Hindi name.
Also, to make sure the result is ordered as expected, you will need to use common.JSONEqGraphQL() instead of testutil.CompareJSON().
graphql/schema/dgraph_schemagen_test.yml, line 3 at r2 (raw file):
nice formatting fix 👍
graphql/schema/dgraph_schemagen_test.yml, line 446 at r2 (raw file):
)@lang
there should be a space before @lang in the generated DQL schema
graphql/schema/gqlschema.go, line 879 at r2 (raw file):
Quoted 11 lines of code…
// all the untagged language fields should be of type string for _, defn := range definitions { typ := schema.Types[defn] for _, field := range typ.Fields { if langUntaggedFields[field.Name] && field.Type.Name() != "String" { errs = append(errs, gqlerror.ErrorPosf(field.Position, "Type %s; Field %s: "+ "Expected type `String` for untagged language field but got `%s`", typ.Name, field.Name, field.Type.Name())) } } }
- This could just have been part of
dgraphDirectiveValidation, the reason we have separate validators for each directive is to not make a change here. - The current way of comparison is flawed. If I had
namein both typesPersonandAnimal, and inPersonit had lang fields, but inAnimalit wasInt, it would give error even forAnimal.
graphql/schema/gqlschema.go, line 1644 at r2 (raw file):
opType string
we shouldn't be needing this extra param here, it can just be handled by the only place from where the call for aggregates is being made.
graphql/schema/gqlschema.go, line 1913 at r2 (raw file):
if isOrderable(fld, defn, providesTypeMap, "addAggregateFields") {
It could just be:
if isOrderable(fld, defn, providesTypeMap) || isMultiLangField(fld, false) {
It is cleaner this way.
graphql/schema/gqlschema.go, line 2292 at r2 (raw file):
if isMultiLangTag(fld, "addMutation") && isAddingInput
I guess this would change to:
if isMultiLangField(fld, isAddingInput) {
graphql/schema/gqlschema.go, line 2318 at r2 (raw file):
isMultiLangTag
isMultiLangField
graphql/schema/gqlschema.go, line 2318 at r2 (raw file):
opType string
we can just make this a boolean, and send it true in the cases of add and update.
isAddOrUpdateOp bool
or maybe just:
isAddingInput bool
or better:
isMutationInput bool
graphql/schema/rules.go, line 56 at r2 (raw file):
var langUntaggedFields map[string]bool
this is bad practice. If we store values in pkg variables like this, then next time someone changes schema, this map would affect the next schema change too, while that shouldn't have happened. It also brings in a lot of concurrency issues.
We shouldn't even have a need for this variable. There has to be another way to achieve what this was doing.
graphql/schema/schemagen.go, line 668 at r2 (raw file):
Quoted 4 lines of code…
if strings.Contains(fname, "@") { langTagDgPreds[fname] = getUpdatedPred(fname, typStr, upsertStr, indexes) continue }
this code block should actually go inside the below if condition, as this could just have been an inherited field, in which case we don't need to do anything about it.
Also, add a comment about why we continue and not add it to typ.fields.
graphql/schema/schemagen.go, line 755 at r2 (raw file):
langStr = "@lang"
added space
langStr = " @lang"
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 1 at r2 (raw file):
type Person {
I think, all the file names are kebab-case, so the file name shouldn't contain an uppercase letter. It should be: multiple-language-tags.graphql. Maybe just call it language-tags.graphql, nothing specific to multiple here.
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 3 at r2 (raw file):
language tagged
the untagged
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 7 at r2 (raw file):
filters
filter, order
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 14 at r2 (raw file):
corresponding language untagged field
the correct DQL schema
There was a problem hiding this comment.
Reviewable status: 9 of 17 files reviewed, 28 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @pawanrawal, and @vmrajas)
graphql/e2e/common/query.go, line 3926 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
all three persons can be added in a single mutation, we shouldn't have to execute 3 separate mutations for that.
combined.
graphql/e2e/common/query.go, line 4034 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
queryPerson( filter: { or: [ { name: { eq: "Julliet" } } { nameHi: { eq: "जूलियट" } } { nameZh: { eq: "朱丽叶" } } { name_Untag_AnyLang: { eq: "Juliet" } } ] } order: { asc: nameHi }what I meant was: in a single query, we use an or filter with different lang fields to get all the 3 persons, and not do 3 separate queries for them. Because, right now,
orderwon't have any effect as the query returns only one person. It would really show an effect when the query was returning more than one person.
So:filter: { or: [ {name: {eq: "Bob"} } {nameHi: {eq: "जूलियट"} } {nameZh: {eq: "<the chinese value of Alice here>"} } ] } order: {asc: nameHi}this should give all the 3 persons, with Juliet as the first person in the results as only Juliet has a Hindi name.
Also, to make sure the result is ordered as expected, you will need to use
common.JSONEqGraphQL()instead oftestutil.CompareJSON().
added one query as you described above.
And also added common.JSONEqGraphQL() instead of testutil.CompareJSON().
graphql/resolve/query_test.yaml, line 3392 at r1 (raw file):
Previously, vmrajas wrote…
nit: Indentation
fixed.
graphql/schema/dgraph_schemagen_test.yml, line 446 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
)@langthere should be a space before
@langin the generated DQL schema
added space
graphql/schema/gqlschema.go, line 879 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
// all the untagged language fields should be of type string for _, defn := range definitions { typ := schema.Types[defn] for _, field := range typ.Fields { if langUntaggedFields[field.Name] && field.Type.Name() != "String" { errs = append(errs, gqlerror.ErrorPosf(field.Position, "Type %s; Field %s: "+ "Expected type `String` for untagged language field but got `%s`", typ.Name, field.Name, field.Type.Name())) } } }
- This could just have been part of
dgraphDirectiveValidation, the reason we have separate validators for each directive is to not make a change here.- The current way of comparison is flawed. If I had
namein both typesPersonandAnimal, and inPersonit had lang fields, but inAnimalit wasInt, it would give error even forAnimal.
added it inside dgraphDirectiveValidation and fixed above error.
graphql/schema/gqlschema.go, line 1644 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
opType stringwe shouldn't be needing this extra param here, it can just be handled by the only place from where the call for aggregates is being made.
removed.
graphql/schema/gqlschema.go, line 1913 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
if isOrderable(fld, defn, providesTypeMap, "addAggregateFields") {It could just be:
if isOrderable(fld, defn, providesTypeMap) || isMultiLangField(fld, false) {It is cleaner this way.
fixed.
graphql/schema/gqlschema.go, line 2292 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
if isMultiLangTag(fld, "addMutation") && isAddingInputI guess this would change to:
if isMultiLangField(fld, isAddingInput) {
no we will need && condition, because getFieldsWithoutIDType function gets called from multiple places.
and inside the isMultiLangField function we are using this flag to decide kind of seperators for multiple language tags field.
graphql/schema/gqlschema.go, line 2318 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
isMultiLangTag
isMultiLangField
changed
graphql/schema/gqlschema.go, line 2318 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
opType stringwe can just make this a boolean, and send it true in the cases of add and update.
isAddOrUpdateOp boolor maybe just:
isAddingInput boolor better:
isMutationInput bool
done. refactored it as isMutationInput bool
graphql/schema/rules.go, line 56 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
var langUntaggedFields map[string]boolthis is bad practice. If we store values in pkg variables like this, then next time someone changes schema, this map would affect the next schema change too, while that shouldn't have happened. It also brings in a lot of concurrency issues.
We shouldn't even have a need for this variable. There has to be another way to achieve what this was doing.
removed it.
graphql/schema/schemagen.go, line 668 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
if strings.Contains(fname, "@") { langTagDgPreds[fname] = getUpdatedPred(fname, typStr, upsertStr, indexes) continue }this code block should actually go inside the below if condition, as this could just have been an inherited field, in which case we don't need to do anything about it.
Also, add a comment about why wecontinueand not add it totyp.fields.
added code in if condition and added comment
graphql/schema/schemagen.go, line 755 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
langStr = "@lang"added space
langStr = " @lang"
addded space
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 1 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
I think, all the file names are kebab-case, so the file name shouldn't contain an uppercase letter. It should be:
multiple-language-tags.graphql. Maybe just call itlanguage-tags.graphql, nothing specific tomultiplehere.
renamed.
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 3 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
language taggedthe untagged
changed
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 7 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
filtersfilter, order
added
graphql/schema/testdata/schemagen/input/multiple-language-Tags.graphql, line 14 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
corresponding language untagged fieldthe correct DQL schema
added
There was a problem hiding this comment.
Just a few more nits and test cases, then good to go.
Reviewed 8 of 8 files at r4.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @jatindevdg, @pawanrawal, and @vmrajas)
graphql/e2e/common/common.go, line 630 at r4 (raw file):
require.Equal(t, expected, actual)
nit: a new line before this
graphql/e2e/common/common.go, line 839 at r4 (raw file):
// mutation tests
nit: a new line before this
graphql/e2e/common/query.go, line 3928 at r4 (raw file):
addPersonParams.Variables
nit: we can directly initialize the Variables in the declaration of addPersonParams
graphql/e2e/common/query.go, line 3977 at r4 (raw file):
Quoted 6 lines of code…
queryPersonExpected := `{"queryPerson":[{"name":"Juliet","nameZh":"朱丽叶","nameHi":"जूलियट", "nameHiZh":"जूलियट","nameZhHi":"朱丽叶","nameHi_Zh_Untag":"जूलियट","name_Untag_AnyLang":"Juliet", "professionEn":"singer"},{"name":"Alice","nameZh":null,"nameHi":"ऐलिस","nameHiZh":"ऐलिस", "nameZhHi":"ऐलिस","nameHi_Zh_Untag":"ऐलिस","name_Untag_AnyLang":"Alice","professionEn":"cricketer"}, {"name":"Bob","nameZh":null,"nameHi":null,"nameHiZh":null,"nameZhHi":null,"nameHi_Zh_Untag":"Bob", "name_Untag_AnyLang":"Bob","professionEn":"writer"}]}`
nit: use newlines and tabs to indent this, spaces won't work.
graphql/schema/rules.go, line 1252 at r4 (raw file):
Quoted 10 lines of code…
langUntaggedFieldName := strings.Split(dgPredName, ".")[1] // untagged language field should be of type string for _, fld := range typ.Fields { if langUntaggedFieldName == fld.Name && fld.Type.Name() != "String" { errs = append(errs, gqlerror.ErrorPosf(fld.Position, "Type %s; Field %s: "+ "Expected type `String` for untagged language field but got `%s`", typ.Name, fld.Name, fld.Type.Name())) return errs } }
Just thought of a few more cases with this:
Let's consider the below schema:
interface Node {
f1: String
}
type Person implements Node {
# notice typename is Node, not Person
f1Hi: String @dgraph(pred: "Node.f1@hi")
# notice type `T` isn't defined in the GraphQL schema, it could have been an already existing type in user's DQL internally
f2: String @dgraph(pred: "T.f@no")
# notice no typename.pred syntax, directly pred is given
f3: String @dgraph(pred: "f3@en")
}
So, should we even do this validation?
If there is any other place, where we are assuming typename.pred syntax for @dgraph(pred: ...), then need to check there too...
Also, just for the sake of completeness, let's add the above scenarios in existing tests as well:
- valid schema test
- dgraph_schemagen_test.yml
- schema/testdata/schemagen/input/language-tag.graphql
There was a problem hiding this comment.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @pawanrawal, and @vmrajas)
graphql/e2e/common/common.go, line 630 at r4 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
require.Equal(t, expected, actual)nit: a new line before this
added
graphql/e2e/common/common.go, line 839 at r4 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
// mutation testsnit: a new line before this
added.
graphql/e2e/common/query.go, line 3928 at r4 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
addPersonParams.Variablesnit: we can directly initialize the
Variablesin the declaration ofaddPersonParams
added
graphql/e2e/common/query.go, line 3977 at r4 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
queryPersonExpected := `{"queryPerson":[{"name":"Juliet","nameZh":"朱丽叶","nameHi":"जूलियट", "nameHiZh":"जूलियट","nameZhHi":"朱丽叶","nameHi_Zh_Untag":"जूलियट","name_Untag_AnyLang":"Juliet", "professionEn":"singer"},{"name":"Alice","nameZh":null,"nameHi":"ऐलिस","nameHiZh":"ऐलिस", "nameZhHi":"ऐलिस","nameHi_Zh_Untag":"ऐलिस","name_Untag_AnyLang":"Alice","professionEn":"cricketer"}, {"name":"Bob","nameZh":null,"nameHi":null,"nameHiZh":null,"nameZhHi":null,"nameHi_Zh_Untag":"Bob", "name_Untag_AnyLang":"Bob","professionEn":"writer"}]}`nit: use newlines and tabs to indent this, spaces won't work.
added
graphql/schema/rules.go, line 1252 at r4 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
langUntaggedFieldName := strings.Split(dgPredName, ".")[1] // untagged language field should be of type string for _, fld := range typ.Fields { if langUntaggedFieldName == fld.Name && fld.Type.Name() != "String" { errs = append(errs, gqlerror.ErrorPosf(fld.Position, "Type %s; Field %s: "+ "Expected type `String` for untagged language field but got `%s`", typ.Name, fld.Name, fld.Type.Name())) return errs } }Just thought of a few more cases with this:
Let's consider the below schema:interface Node { f1: String } type Person implements Node { # notice typename is Node, not Person f1Hi: String @dgraph(pred: "Node.f1@hi") # notice type `T` isn't defined in the GraphQL schema, it could have been an already existing type in user's DQL internally f2: String @dgraph(pred: "T.f@no") # notice no typename.pred syntax, directly pred is given f3: String @dgraph(pred: "f3@en") }So, should we even do this validation?
If there is any other place, where we are assumingtypename.predsyntax for@dgraph(pred: ...), then need to check there too...Also, just for the sake of completeness, let's add the above scenarios in existing tests as well:
- valid schema test
- dgraph_schemagen_test.yml
- schema/testdata/schemagen/input/language-tag.graphql
added tests
There was a problem hiding this comment.
Reviewed 7 of 7 files at r5.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @jatindevdg, @pawanrawal, and @vmrajas)
graphql/schema/dgraph_schemagen_test.yml, line 435 at r5 (raw file):
f2: String @dgraph(pred: "T.f@no") f3: String @dgraph(pred: "f3@en")
these two are missing in the dgraph schema, they should have been present with @lang.
There was a problem hiding this comment.
Reviewable status: 15 of 17 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @pawanrawal, and @vmrajas)
graphql/schema/dgraph_schemagen_test.yml, line 435 at r5 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
f2: String @dgraph(pred: "T.f@no") f3: String @dgraph(pred: "f3@en")these two are missing in the dgraph schema, they should have been present with
@lang.
fixed this.
abhimanyusinghgaur
changed the title
There was a problem hiding this comment.
Reviewed 1 of 2 files at r6, 2 of 2 files at r7.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @jatindevdg, @pawanrawal, and @vmrajas)
This PR adds support for language tags in GraphQL. This feature is available in Dgraph.
We exposed language tags to GraphQL using
@dgraphdirective.RFC: https://discuss.dgraph.io/t/rfc-allow-language-tag-support-in-graphql/13027
This change is