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

feat(GraphQL): Webhooks on add/update/delete mutations (GRAPHQL-1045) #7494

Merged
merged 19 commits into from
Mar 19, 2021

Conversation

abhimanyusinghgaur
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur commented Mar 1, 2021

Please see the RFC for more details.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 1, 2021
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewed 74 of 74 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @vmrajas, and @vvbalaji-dgraph)


graphql/resolve/mutation_rewriter.go, line 819 at r1 (raw file):

	uids := make([]uint64, 0, len(uidSlice))

	if len(uidSlice) > 0 {

this if block is not required


graphql/resolve/webhook.go, line 110 at r1 (raw file):

	b, err := json.Marshal(payload)
	if err != nil {
		// don't care to send the payload if there are JSON marshalling errors

We should log the error using glog.Error in V3 level


graphql/resolve/webhook.go, line 117 at r1 (raw file):

	headers.Set("Content-Type", "application/json")
	// don't care for the response/errors, if any.
	_, _ = schema.MakeHttpRequest(nil, http.MethodPost, x.Config.GraphQL.GetString("lambda-url"),

let's log the error if its non-nil or if response code is not a success

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Also add a way to override this behavior using a key that is part of extensions.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @vmrajas, and @vvbalaji-dgraph)

…ebhooks

# Conflicts:
#	graphql/e2e/schema/apollo_service_response.graphql
#	graphql/resolve/mutation_rewriter.go
#	graphql/schema/dgraph_schemagen_test.yml
#	graphql/schema/gqlschema.go
#	graphql/schema/gqlschema_test.yml
#	graphql/schema/testdata/apolloservice/output/auth-directive.graphql
#	graphql/schema/testdata/apolloservice/output/custom-directive.graphql
#	graphql/schema/testdata/apolloservice/output/extended-types.graphql
#	graphql/schema/testdata/apolloservice/output/generate-directive.graphql
#	graphql/schema/wrappers.go
Copy link
Contributor

@vmrajas vmrajas left a comment

Choose a reason for hiding this comment

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

LGTM
The mutation_rewriter.go part looks good to me.
Just added few nits.

t.Run("entitites Query on extended type with key field of type String", entitiesQueryWithKeyFieldOfTypeString)
t.Run("entitites Query on extended type with key field of type Int", entitiesQueryWithKeyFieldOfTypeInt)
t.Run("entities Query on extended type with key field of type String", entitiesQueryWithKeyFieldOfTypeString)
t.Run("entities Query on extended type with key field of type Int", entitiesQueryWithKeyFieldOfTypeInt)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -341,7 +341,8 @@ type Region {
district: District
}

type District {
type District @lambdaOnMutate(add: true, update: true, delete: true) {
dgId: ID!
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Indentation seems off. Likely due to different tab spaces being used.

self.addWebHookResolvers({
"District.add": districtWebhook,
"District.update": districtWebhook,
"District.delete": districtWebhook,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Line at end of file.

self.addWebHookResolvers({
"District.add": districtWebhook,
"District.update": districtWebhook,
"District.delete": districtWebhook,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Extra line at end of file.

if err != nil {
glog.Errorf("Error occured while aborting transaction: %s", err)
glog.Errorf("Error occurred while aborting transaction: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -263,12 +263,12 @@ func (xidMetadata *xidMetadata) isDuplicateXid(atTopLevel bool, xidVar string,
// simply ignored.
// If it is found out that the Person with id 0x123 does not exist, the corresponding
// mutation will fail.
func (mrw *AddRewriter) RewriteQueries(
func (arw *AddRewriter) RewriteQueries(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

type Question implements Post @lambdaOnMutate(add: true, update: true) {
id: ID!
questionText: String
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Line at end of file

Copy link
Contributor Author

@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.

Reviewable status: 54 of 91 files reviewed, 6 unresolved discussions (waiting on @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)


graphql/resolve/mutation_rewriter.go, line 819 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

this if block is not required

Done.


graphql/resolve/webhook.go, line 110 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We should log the error using glog.Error in V3 level

Done.


graphql/resolve/webhook.go, line 117 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

let's log the error if its non-nil or if response code is not a success

Done.

@abhimanyusinghgaur abhimanyusinghgaur merged commit 348119e into master Mar 19, 2021
abhimanyusinghgaur added a commit that referenced this pull request Mar 19, 2021
…#7494)

Please see the [RFC](https://discuss.dgraph.io/t/webhook-lambda-on-add-update-delete-mutations/12904) for more details.

(cherry picked from commit 348119e)

# Conflicts:
#	graphql/schema/testdata/schemagen/output/interface-with-id-directive-and-ID-field.graphql
abhimanyusinghgaur added a commit that referenced this pull request Mar 19, 2021
…#7494) (#7616)

Please see the [RFC](https://discuss.dgraph.io/t/webhook-lambda-on-add-update-delete-mutations/12904) for more details.

(cherry picked from commit 348119e)

# Conflicts:
#	graphql/schema/testdata/schemagen/output/interface-with-id-directive-and-ID-field.graphql
@abhimanyusinghgaur abhimanyusinghgaur deleted the abhimanyu/webhooks branch March 19, 2021 11:42
aman-bansal pushed a commit that referenced this pull request Mar 25, 2021
…#7494) (#7616)

Please see the [RFC](https://discuss.dgraph.io/t/webhook-lambda-on-add-update-delete-mutations/12904) for more details.

(cherry picked from commit 348119e)

# Conflicts:
#	graphql/schema/testdata/schemagen/output/interface-with-id-directive-and-ID-field.graphql
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