-
-
Notifications
You must be signed in to change notification settings - Fork 796
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
Stacked object merges fail in v7 (regression) #2157
Comments
(Also worth noting, this is a fairly contrived layout of objects and fields that look a little strange in this microcosm test configuration... the error is real, even if the combination of merge patterns look a little odd here when taken literally) |
Blork. |
Are you able to again share the actual requests/responses to the different microservices. My suspicion is that delegation is occurring successfully, but that this is a merge failure... But not sure |
Sure thing. Focusing just on test 3 (the failure)–the initial queries are as I would expect, but it fails without querying for the merged network:
posts {
posts(ids: [55]) {
title
network {
__typename
id
}
__typename
id
}
}
# returns:
# [ { id: '55', title: 'Post 55', network: { id: 57 } } ]
_posts query ($_v0_ids: [ID!]!) {
_posts(ids: $_v0_ids) {
sections
__typename
id
}
}
# returns:
# [ { id: '55', sections: [ 'News' ] } ]
{
"errors": [
{
"message": "Cannot return null for non-nullable field Network.domain.",
"locations": [
{
"line": 5,
"column": 21
}
],
"path": [
"posts",
0,
"network",
"domain"
]
}
],
"data": {
"posts": [
{
"title": "Post 55",
"network": null,
"sections": [
"News"
]
}
]
}
} |
Awesome, so not a merge failure it's a problem with the delegation plan are you able to get some logging into the get Fields not in subschema function again |
Sorry, traveling |
I think problem here https://github.com/ardatan/graphql-tools/blob/master/packages/delegate/src/externalObjects.ts#L76 I am overriding instead of updating the field subschema map |
Not always start with an empty object |
Sometimes target[FIELD_SUBSCHEMA_MAP_SYMBOL] |
What you're describing, it's that this...? https://github.com/ardatan/graphql-tools/blob/master/packages/delegate/src/externalObjects.ts#L79-L81 I just tried changing the linked line to |
Actually that code needs a lot of improvement but I just did a quick refactor and was able to fix locally. Well hopefully get a fix out this afternoon |
I'm still working my way through another similar bug in my project. I'll open up a separate issue with a minimal repo just in case it's a diff issue, and can merge later if the root cause ends up being the same as @gmac's report. |
v7 introduced a regression in the merging of ExternalObjects that causes type merging to fail when undergoing multiple rounds of merging. #2157
* fix(stitch): mergeExternalObjects regressions v7 introduced a regression in the merging of ExternalObjects that causes type merging to fail when undergoing multiple rounds of merging. #2157 * add changeset
Hi @gmac @yaacovCR 👋 We receive
instead of
"@graphql-tools/batch-delegate": "^7.0.0", |
Have you run a manual upgrade on all of those packages? Specifically: batch-delegate, which didn’t get an update but must be upgraded to lock onto a new delegate version. I had the same issue while trying to grab the latest. Manually running yarn upgrade on everything sorted it out. All that is to say, yes, I’m pretty confident the fixes made are viable. We’ve got a pretty rigorous stitching test suite on our gateway and the latest stitch package passes it |
Upgrading to v7, my API test suite shows an outstanding failure possibly similar to #2137, though distinct. While #2137 was a newly-discovered issue that existed in v6, this is demonstrably a regression that worked in v6 and is failing in v7.
In this scenario, objects merging atop other merged objects now appear to fail. Here's another three-part test that demonstrates:
Post.network
in Posts service merges withNetwork
from Layouts service. This also works fine.Of note, I'm running the alpha stitching branch from #2154. The test suite had several failures with 7.0.0, though running with that fix has brought it down to just this one (so significant progress!)
cc @yaacovCR @DrewML
The text was updated successfully, but these errors were encountered: