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

Fix(GraphQL): Add extra checks for deleting UpdateTypeInput #7595

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented Mar 17, 2021

Motivation:
A user had reported panic on schema update. The reason was that cleanupInput function cleared any input types which had name of the form, UpdateTInput and contained exactly one field. This PR adds extra conditions to ensure that the input type is removed only when its generated by dgraph and contains a field by name, filter.

Testing:
Added unit test in graphql/schema/testdata/schemagen

Fixes GRAPHQL-1102


This change is Reviewable

@vmrajas vmrajas requested a review from pawanrawal as a code owner March 17, 2021 05:30
@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 17, 2021
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.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pawanrawal and @vmrajas)


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

	if strings.HasPrefix(def.Name, "Update") && len(def.Fields) == 1 {
		if def.Fields[0].Name == "filter" {

we can make the condition exactly match what we generate, like this:

if strings.HasPrefix(def.Name, "Update") && strings.HasSuffix(def.Name, "Input") && len(def.Fields) == 1 {
  typeDef := sch.Types[def.Name[6:len(def.Name)-5]]
  if typeDef != nil && typeDef.Directives.ForName(remoteDir) == nil && (typeDef.Kind == ast.Object || typeDef.Kind == ast.Interface) {
    // this ensures that it was us who generated the `UpdateTInput`
    // and allows users to still be able to define a type `UpdateT1Input` with a field named `filter` in that input type and not get cleaned up.
  }
}

@vmrajas vmrajas force-pushed the rajas/GRAPHQL-1102 branch from b5e06e3 to 753390a Compare March 17, 2021 11:09
@vmrajas vmrajas merged commit ce9f4f5 into release/v21.03 Mar 17, 2021
@vmrajas vmrajas deleted the rajas/GRAPHQL-1102 branch March 17, 2021 11:26
vmrajas added a commit that referenced this pull request Mar 17, 2021
* Add extra checks for deleting UpdateTypeInput

* Address Abhimanyu's comment

(cherry picked from commit ce9f4f5)
vmrajas added a commit that referenced this pull request Mar 17, 2021
…7600)

* Add extra checks for deleting UpdateTypeInput

* Address Abhimanyu's comment

(cherry picked from commit ce9f4f5)
aman-bansal pushed a commit that referenced this pull request Mar 25, 2021
* Add extra checks for deleting UpdateTypeInput

* Address Abhimanyu's comment
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.

3 participants