Skip to content

feat(GraphQL): add selection set of query to the body of lambda request. #7718

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 5 commits into from

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Apr 13, 2021

Fixes DGRAPH-3180


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Apr 13, 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 2 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @minhaj-shakeel and @pawanrawal)


graphql/e2e/common/lambda.go, line 162 at r1 (raw file):

lambdaOnQueryUsingDql
  1. seems we can merge this into the above lambdaOnQueryUsingDql test. Also, need to update the lambda script files to make use of the info for this query.
  2. We should also add a test for a lambda field that returns an object, there is no such test yet. So that we can test that nested filters are propagated for lambda fields as well.

graphql/schema/custom_http.go, line 197 at r1 (raw file):

*fldValue

maybe just make it an interface{} because there can be cases when the argument value comes from a variable, then the Raw would contain $varName and not the actual value of the $varName.
Also, since Raw is a string so even Int values will be serialized as string, resulting in issues for the users.
By using interface{} we can avoid all that,

OR we can go the way we saw in the Apollo code.

OR just an extension of the interface{} approach, we can use f.ArgumentMap() for the root lambda field, and then don't need to create any struct for args here. fldArgumentList will just be a []map[string]interface{} then. Maybe in a similar manner, we won't need any other structs as well.

I prefer the 3rd one, but let's discuss it tomorrow.


graphql/schema/custom_http.go, line 220 at r1 (raw file):

type fldSelectionSet []fldSelection
type fldSelection interface{}

shouldn't this just be?

type fldSelectionSet []*selectionField

Also, let's cleanup the fld prefix from every struct. We can name them something better. For example, this one can just be selectionSet.
And, add JSON tags within every struct, which ensures that if we rename a field, we don't change the output JSON.


graphql/schema/custom_http.go, line 246 at r1 (raw file):

}

func convert(f ast.Field) selectionField {
  1. maybe rename this function as getInfo(), and it returns an Info object.
  2. Just thinking along what we saw in the apollo resolvers, maybe we should nest the field inside the info:
{
  "info": {
    "field": <the current output returned by convert here>
  }
}

so that if we later want to add the returnType in info, we can do that.


graphql/schema/custom_http.go, line 253 at r1 (raw file):

outField.SelectionSet = make(fldSelectionSet, len(f.SelectionSet))

can be moved within the initializer


graphql/schema/custom_http.go, line 255 at r1 (raw file):

*sel.(*ast.Field)

convert can directly accept a pointer, instead of dereferencing, creating a copy and passing that every time.


graphql/schema/wrappers.go, line 205 at r1 (raw file):

GetField() ast.Field

let's not expose the ast.Field to other packages, rather move the convert logic to wrappers. And actually, as the convert logic is within the same package, we shouldn't even be needing this method. So, rather remove this.

@maaft
Copy link

maaft commented May 28, 2021

Hey,

I'm eagerly waiting for this feature. Why is it still blocked for merging?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

github-actions bot commented Aug 2, 2024

This PR has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open.

@github-actions github-actions bot added the Stale label Aug 2, 2024
@github-actions github-actions bot closed this Aug 10, 2024
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. Stale
Development

Successfully merging this pull request may close these issues.

4 participants