Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added lang support #8924

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Added lang support #8924

merged 2 commits into from
Oct 13, 2023

Conversation

harshil-goel
Copy link
Contributor

Added lang support to GraphQL

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2023

CLA assistant check
All committers have signed the CLA.

@dgraph-bot dgraph-bot added area/testing Testing related issues area/graphql Issues related to GraphQL support on Dgraph. area/core internal mechanisms go Pull requests that update Go code labels Jul 31, 2023
@@ -2275,7 +2285,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, isAddingInput bool) ast.FieldList {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the parameter isAddingInput mean?

Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I see tests only for hash and exact index. We should add test for other indexes as well.

// We don't include fields in update patch, which corresponds to multiple language tags in dgraph
// Example, nameHi_En: String @dgraph(pred:"Person.name@hi:en")
// We don't add above field in update patch because it corresponds to multiple languages
if isMultiLangField(fld, true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we add a test for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already present tests: graphql/e2e/directives/schema.graphql

graphql/schema/gqlschema.go Outdated Show resolved Hide resolved
graphql/schema/gqlschema.go Outdated Show resolved Hide resolved
graphql/schema/gqlschema_test.yml Show resolved Hide resolved
graphql/schema/rules.go Outdated Show resolved Hide resolved
graphql/schema/rules.go Show resolved Hide resolved
graphql/schema/schemagen.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of more comments and I see some old unresolved comments too. Please take a look. Thanks

@@ -1905,7 +1910,7 @@ func addAggregationResultType(schema *ast.Schema, defn *ast.Definition, provides
}

// Adds titleMax, titleMin fields for a field of name title.
if isOrderable(fld, defn, providesTypeMap) {
if isOrderable(fld, defn, providesTypeMap) || isMultiLangField(fld, false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isOrderable function already checks for isMultiLandField. Do we need to do it again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fails some tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why though?

graphql/schema/rules.go Outdated Show resolved Hide resolved
@harshil-goel harshil-goel merged commit 283fe63 into main Oct 13, 2023
22 checks passed
@harshil-goel harshil-goel deleted the harshil-goel/lang-support branch October 13, 2023 17:28
shivaji-kharse pushed a commit that referenced this pull request Mar 12, 2024
Added lang support to GraphQL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core internal mechanisms area/graphql Issues related to GraphQL support on Dgraph. area/testing Testing related issues go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

7 participants