Mesh SDK execution fails #1339
Labels
bug
Something isn't working
Comments
|
UPDATE: Unlike last time, the queries work well with encapsulation now but all mutations fail with latest packages. Moved away again from encapsulation to using only prefix and rewriting it all again and the errors are now gone. I will stay away from encapsulation for some time. Don't want to rewrite everything again 😅 Want to freeze the stack and focus on the logic now. Will stick with prefix since that works without issues. |
|
Is this |
|
@ardatan Not doing anything else from my end in this case. The error occurs when the SDK call happens (when the mutation is encapsulated). Now that I have removed the encapsulation, errors went away. Not sure what you mean here by |
|
For context, this is the PNPM Lockfile I use with all the deps: https://gist.github.com/tvvignesh/b882379b25db4629fb29f3330ba0957c |
|
Are you able to log the query hitting dgraph prior to it failing? It looks like aliases used for batching are not working right, or not working right with __typename, prefixing, etc. What actually gets sent? |
|
@tvvignesh How do you write custom mutation and what does it have inside of it? |
|
@ardatan It is a query to Dgraph. Nothing special. Here This is how I wrote it when I used encapsulation: mutation updateAuthMethod ($updatedAuthMethod: globalDBUpdateAuthMethodInput!) {
globalDB {
updateAuthMethod(input: $updatedAuthMethod) {
authMethod {
id
authKey
verificationStatus
authProviderMethod {
id
}
metadata
account {
id
}
creationTime
}
}
}
}And when I removed encapsulation, this is how I use it now (which works): mutation updateAuthMethod ($updatedAuthMethod: globalDBUpdateAuthMethodInput!) {
updateAuthMethod(input: $updatedAuthMethod) {
authMethod {
id
authKey
verificationStatus
authProviderMethod {
id
}
metadata
account {
id
}
creationTime
}
}
} |
|
@yaacovCR I have not enabled query logging in Dgraph. Will add it now. Anywhere you would like me to add a log in the Mesh SDK rather? PS: This happens not only for specific mutations but all mutations when using encapsulation. |
|
Dgraph log works, final transformed query. |
|
The request:
The complete response: |
|
To give a minimal repro, I did not involve any of my code in the repro above. I just did a Rolling back to prefix now. Anything else you want me to test before that? |
|
I added a log to the Mesh SDK execution like this in
And these are the logs I get: |
|
And the result of the above request is nothing but the same error as mentioned in this issue:
(And all this without involving any of my code and just using mesh serve) |
|
The error occurs in the mesh runtime precisely in this line: |
|
Digging deep the error occurs when composing resolvers, the package from gql tools: |
|
I think the error you are getting has to do with aliasing the __typename introspection field for mutations. I think the things to see is whether you can submit to dgraph a mutation with __typename as root field, and then try again with an alias.... Although I am not clear if the mutation is not hitting dgraph at all or if it fails validation so it doesn't get logged... Either way, we need to see that query, I wonder if you can put logging into batch-execute package which seems to be responsible for that __typename prefix |
|
Sure. Will try it and reply now. Btw, this is the response of composeResolvers when server starts (not during mutation execution).
|
|
@yaacovCR This is what I get. Any specific value you are looking for? |
|
Anyways, no pressure 😅 Rolling it back now to prefix now and will work with it. Maybe this error is caused since nesting the mutations using encapsulation require nesting the resolvers as well. |
|
Print that document, please :) |
|
@yaacovCR This is what I see when I print the document: mutation ($graphqlTools0_updatedAuthMethod: UpdateAuthMethodInput!) {
graphqlTools0___typename: __typename
graphqlTools0___typename: __typename
graphqlTools0___typename: __typename
graphqlTools0___gqtld0__: __typename
graphqlTools0___typename: __typename
graphqlTools0___gqtld1__: updateAuthMethod(
input: $graphqlTools0_updatedAuthMethod
) {
__typename
__typename
__typename
authMethod {
__typename
__typename
__typename
id
__typename
__typename
__typename
authKey
__typename
__typename
__typename
verificationStatus
__typename
__typename
__typename
authProviderMethod {
__typename
__typename
__typename
id
}
__typename
__typename
__typename
metadata
__typename
__typename
__typename
account {
__typename
__typename
__typename
id
}
__typename
__typename
__typename
creationTime
}
}
} |
|
Will be afk for 3 days, going out. Will come back after I return. |
|
That's a lot of __typename. But looks valid, maybe send that in to dgraph and see what happens. Might make sense to open up bug in graphql-tools for why so much type name and if that works when submitted directly but not when stitching. Not sure that this is a mesh issue seems either related to tools or dgraph |
|
@yaacovCR I don't think the issue is with Dgraph cause the error occurs only when adding encapsulate config from mesh without changing anything in dgraph and not otherwise which means that the error gets fixed by changing the mesh sdk and not dgraph. But still, as you said I am curious as to why there are these many __typename in the mutation. |
|
We should be able to prove whether or not is an issue with dgraph by just sending in that mutation. Looks a little weird, but well formed at least to my eye, so I'm a bit not sure why it wouldn't work. If it does actually work then we know it is not an issue with dgraph for sure... |
|
@yaacovCR @ardatan Just came back from a short vacation. Tested what you said by sending the mutation to Dgraph directly. And it throws a validation error like this:
I guess it was not logged cause Dgraph cause the validation failed and hence Dgraph did not execute the mutation itself. And I also checked the schema properly. I did a quick search in the |
|
@yaacovCR @ardatan Ahh.. Wait a sec. Just noticed this: https://discuss.dgraph.io/t/bug-upmost-typename-in-mutation-finds-no-resolver/11989/5 Looks similar and looks like the Dgraph team is addressing this and the issue states the same behavior where it works for queries but not for mutations. What a crazy day 😂 |
|
On a sidenote, may I know why I guess, it is good to add some deduplication logic in GQL Mesh or Tools to avoid duplicating the fields if I am not wrong which can cater not just to |
|
We don't always add __typename but some transforms require it, and if you are using multiple transforms that require it, we don't check whether it exists before adding it again. This is because the graphql server will coalesce these anyway.... See ardatan/graphql-tools#2225 |
|
@yaacovCR Thanks for the link. Just had a look. So, I will just go with this then considering graphql/graphql-js#2342 has to be implemented for deduplication. Wondering why mutation works without encapsulate transform then. I guess that does not add @ardatan You can close this if you think there is nothing to fix/change. Thanks. |
|
This deduplication can be done on executor level, maybe? |
|
I am wondering if we should just request __typename on every field automatically so that we never have to add or deduplicate.... But I think the most important step forward is to combine as many transforms as we can to do work in parallel so that we do not have to traverse the request/response tree so many times. If we combine transforms in that way, we will add fewer duplicates... |
|
@yaacovCR Adding |
|
I don't have a good feel for this in production for many reasons but it's a question of the trade-off of asking for extra fields that we don't needversus asking for only what we need but having queries contain a few duplicates which will be removed on the server |
|
Actually having duplicates are fine but it is the number of duplicates which is high here. In the mutation I have shared, the number of I don't know its impact on the performance since I have no metrics to measure so far. But I feel that rather than adding duplicates and then using a function to remove it, it is better to prevent adding duplicates if possible (I may be wrong here). Anyways, feel free to take a call on what is right according to you guys. |
Closed
Closed
This was referenced Apr 30, 2024
Closed
Closed
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I just got a huge scary error today when I called an SDK function. This error occurs at runtime when the call is made and there are no syntactical errors.
This error is most probably because of adding encapsulation (it worked before).
Any clues? Thanks,
CC: @dotansimha @ardatan
The text was updated successfully, but these errors were encountered: