Skip to content

fix (graphql): disallowing field names with as #6474

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

Closed
wants to merge 4 commits into from

Conversation

aman-bansal
Copy link
Contributor

@aman-bansal aman-bansal commented Sep 17, 2020

Signed-off-by: aman-bansal bansalaman2905@gmail.com
This is related to GRAPHQL-564. as is Dgraph reserved keyword. It's being used by DQL to identify variables. This PR is to restrict the naming for fields with the name as.

Though interesting things have been found while working on this. There is a difference in how we check as keyword while query parsing. For example, in normal DQL queries, the parser uses this strings.ToLower(peekIt[0].Val) == "as" (dgraph/gql/parser.go:2552) this means as, As, AS, aS all are the same.

But while parsing facets, its been treated like this !trySkipItemVal(it, "as") (dgraph/gql/parser.go:1984).

Though having a different use case, keyword significance is the same. Would love to hear thoughts about these implementations.


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Sep 17, 2020
@aman-bansal aman-bansal marked this pull request as ready for review September 24, 2020 05:23
Signed-off-by: aman-bansal <bansalaman2905@gmail.com>
Signed-off-by: aman-bansal <bansalaman2905@gmail.com>
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

we also have a @dgraph(type: "", pred: "") directive, using which one can override the GraphQL generated name for any field or type. I think that also needs to have this restriction.

@ashish-goswami can say more about its usage in facets.

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @aman-bansal, @danielmai, @MichaelJCompton, and @pawanrawal)


wiki/content/graphql/schema/reserved.md, line 10 at r1 (raw file):

Names `Int`, `Float`, `Boolean`, `String`, `DateTime` and `ID` are reserved and cannot be used to define any other identifiers.
Similarly, Names like `uid`, `Subscription`, `as`, `Query` and `Mutation` are also reserved and cannot be used to define any other Identifiers.

Maybe just combine the two lines, or state it all like:

The following names are reserved and can't be used to define any other identifiers:
* Int
* Float
...
* as/As/aS/AS
* Query
...

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

Nice work!

Sorry, I should have pointed it the last time itself, but we also need to add tests for the same. Have a look in graphql/schema/gqlschema_test.yml and just add invalid_schemas using as at all the different places, i.e., as a type name, field name, and the same cases with @dgraph.

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @danielmai, @MichaelJCompton, and @pawanrawal)

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @abhimanyusinghgaur, @danielmai, @MichaelJCompton, and @pawanrawal)

@abhimanyusinghgaur
Copy link
Contributor

Actually, the target branch should be master.

@aman-bansal aman-bansal deleted the aman/graphql_564 branch December 15, 2020 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants