Did not fetch typename for object, unable to resolve interface

[BrendanBall]

I'm trying to use this fork as error handling is broken in graphql-tools. However I'm experiencing different bugs with this.
I only use these from graphql-tools:

• makeExecutableSchema
• introspectSchema
• makeRemoteExecutableSchema
• mergeSchemas
• ReplaceFieldWithFragment
• WrapQuery

I'm experiencing a weird bug when trying to stitch something that results in the error Did not fetch typename for object, unable to resolve interface. I have found that handleResult in checkResultAndHandleErrors first gets a result that is a list which is handled fine and recognised as a list, but then the same result runs through that code again for some reason and even though the result is still a list, it gets recognised as a CompositeType which then breaks resolveFromParentTypename as it's expecting an object and not a list. All this is very difficult to debug so I don't currently have a repro that I can share. Given that this wasn't a problem in the latest graphql-tools I assume it's some bug introduced here or some weird change in behaviour somewhere.

I read through the changelog and nothing stood out to me to be the cause of this issue I'm experiencing.

[yaacovCR]

What are you doing in WrapQuery?

[BrendanBall]

I'm using it to make sure the ID field is always queried for the upstream query even if the original query didn't ask for it, so that I can do the stitching properly in the response data, as most queries don't rely on list index like Dataloader to relate data.

EDIT: I commented out that WrapQuery transform and the same issue is still present, so it's not related to that.

[yaacovCR]

This sounds really annoying. If you can share a minimal or even maximal reproduction, I would love to fix it.

Sounded to me like there is a mismatch between the upstream result and your gateway schema, which is why I suspected you were doing something in WrapQuery to modify result.

I guess not so simple.

Unrelated, isn't what you are doing with WrapQuery really what ReplaceFieldWithFragment or the mergeSchemas fragment syntax is for.

If you can't make a minimal reproduction, it might help to give me a link to as much code as you can share.

Versus, you can just give me the result received from the upstream query and the result of printing your merged/transformed final schema.

I would love to address expeditiously.

[BrendanBall]

@yaacovCR thanks very much for the quick response. I'll try make a reproduction. I can't remember exactly why I went with WrapQuery as it was quite long ago, so there might be a better way to do that. Until recently our graphql gateway was still running graphql-tools 3.x and graphql-js 0.x because of all kinds of dependency problems and compatibility problems with apollo server 2.

Are you using graphql-tools-fork in production currently?

[yaacovCR]

A reproduction would be great. I am only lightly using graphql-tools-fork in production and so I rely on bug reports extensively.

Recently, Gatsby has switched to using the fork, so a lot of users will be using the fork in production, but they are using a small set of straightforward transforms, and do not use mergeSchemas, although the underlying stitching mechanism is the same.

[yaacovCR]

@BrendanBall any luck putting together a reproduction, would love to take a look at this.

[BrendanBall]

no not yet, but I've found it's actually breaking in my dataloaders, possibly with ReplaceFieldWithFragment. I haven't had time to dig deeper today, but I'll let you know as soon as I've figured it out.

[yaacovCR]

Let me know when. Would love to help.

[BrendanBall]

I have reproduced the issue here. You can run npm run start to run it.

[yaacovCR]

Ah!

In order to properly serialize enums/scalars, the fork checks the return type on info to properly handle the proxied result. To use delegateToSchema with a dataloader you cannot just use the info object from the first key, you have to wrap the returnType in a GraphQlList.

I would not modify the info object, though, just create a new one with the modified returnType and spread the other properties.

[yaacovCR]

I did not test this solution yet, will update with a patch hopefully over weekend, but can you try that first?

[yaacovCR]

See https://github.com/yaacovCR/graphql-tools-fork-issues-33/commit/abe062ced461c9e002d128d1fa23c0a6ec161d35

[yaacovCR]

Also see your comment from a ways back: ardatan#786 (comment)

How did you solve the different selection set problem when using dataloader?

I stand by my response there that batching should be happening in the link, either using something like https://github.com/prisma-labs/http-link-dataloader or Apollo client itself like I mention there.

[BrendanBall]

Sorry I'm not really available on the weekend.
Is there documentation around delegateToSchema and needing to use GraphQLList with a dataloader? If not can you briefly tell me why this is necessary? Is it a temporary fix? As it seems very hacky.

Regarding my old comment, I solved it by grouping keys by selections sets (and args) in the dataloader, so any single dataloader resolution would do 1 request for each unique batch. We manually implemented batch queries which take in a list of IDs to facilitate this batching. IMO the way batching is done in http-link-dataloader is not scalable as it duplicates the selection set for each ID which ends up creating huge queries unnecessarily. We also don't have array-based batching enabled currently and don't currently use dataloaders in itemByID (however that is solvable). The code we implemented for this batching is very hacky and hasn't been maintained as it's been working fine for us so far, but I'm sure it can be improved a lot.

We are still evaluating whether we want to implement apollo federation, but even if we decide on that, it would be a while before that gets done, so I need to be able to use graphql-tools for stitching for quite a while still. So I do have an interest in having graphql-tools-fork working properly.

[yaacovCR]

Docs in progress, as above. I am not sure this is a hack, it kind of is fitting that if you are going to have keys[0].info in your arguments, then you might need to indicate that you are moving from a single GraphQLObject to a list. Or, in other words, the hack is delegation from a single object to a list in the first place, wrapping the returnType is just an acknowledgement that you are doing that. :)

In theory, we could allow an additional returnType argument within the delegateToSchema options that allows you to override the returnType set on info. I would accept a PR around that.

Again, this is necessary to support using enum/custom scalar internal values within the gateway resolvers for subschema defined types. They have to be properly serialized, and it seemed to me best to use the returnType.

Another option is to iterate over the final processed request within checkResultAndHandleErrors. That would have the advantage of solving point 2 in #35, but the disadvantage of requiring an additional result traversal. Right now, the only transform that requires a result traversal is RenameTypes, which is only necessary to support gateway only interfaces, otherwise we could just wrap all the subschema type resolvers. It would be nice if we could unify these result traversals in some way. I may accept a PR around that, but it sounds less straight forward.

[yaacovCR]

Or....

Maybe you are suggesting the default behavior should be to take the return type from the proxy target rather than from the source? The problem is that the return type check happens after transforms have been employed, and so the stitching logic expects the types to now match the source schema and just require serialization and merging of fields from other subschemas..

[yaacovCR]

Ah, which is why we can't iterate over the final processed request as, it may no longer match the result because of transforms...

[BrendanBall]

Wrapping the returnType with GraphQLList is causing other problems in our dataloader code. This seems like it's getting really complex with lots of edge cases and likely to be buggy. I'm going to have to spend some time looking at all the changes and how it relates to our existing stitching code, because it seems the version of graphql-tools that we're currently running is a lot simpler and has been working for quite a while now.

[yaacovCR]

Not sure from your comment here what additional error you are hitting.

I'd be happy to help out or game out another approach to supporting your desired feature set, but would need a better description of the problem you are running into.

Not sure how much code you can share, but am also happy to sign a NDA if necessary. I am a big believer in schema delegation/stitching. :)

[BrendanBall]

I'm getting some weird bug where I'm getting back a list of promises where I'm expecting results, presumably something to do with wrapping in a GraphQLList. Still need to investigate further. @yaacovCR what is your main interest in maintaining graphql-tools? Are you using stitching at a company or open source project? I could probably open source the core stitching code, but it's a big mess that I wouldn't just want to give someone to go through as is.

[yaacovCR]

I suppose another option would be to add options that disable return type checking if you do not need to serialize custom scalars or merge types, and revert to the original behavior. I would be willing to add that functionality if I knew that would actually help you, but I am fearful your dataloaders might be breaking because of the error handling itself. I imagine I might be able to guide you somewhat as with the return type if you were able to share my code.

I am sorry I can't be of more help. Let me know if anything.

[yaacovCR]

Lists of promises that resolve to results makes sense to me because every result might need to asynchronously merge types. That is known behavior and not necessarily a bug.

[yaacovCR]

This is transparent in the usual use case because graphql will resolve those for us.

[yaacovCR]

Just await those and let me know. In terms of my interest in schema stitching, it is purely from my personal projects/open source involvement.

Although I reserve the right to hit it big one day.

[yaacovCR]

Closing this for now as consistent with known behavior. Feel free to reopen if you think there is something actionable from my. And feel free to ping me otherwise if I can be of any help.

[yaacovCR]

On second thought promise issue should be fixable, opened separate issue.

[BrendanBall]

with normal graphql-tools I only have to resolve one promise that delegateToSchema returns and then I have data. With your fork I resolve the promise that delegateToSchema returns then I have another list of promises to resolve. Why do you need to asynchronously merge types after you already did the asynchronous call to the remote schema?

[yaacovCR]

This can and should be changed in your case as you are not merging types from multiple subschemas.

When merging types from multiple schemas, the merge happens on each object. I previously believed it was more correct to defer promise resolution in this case just as graphql defers promise resolution; now I am not so sure!

[yaacovCR]

Sorry, the above should be I previously believed delegateToSchema should allow a list of promises as the result as graphql will convert this to a promise to a list eventually. I know am not sure, as this approach maximizes concurrency, but could cause confusion.

In your case, since you are not merging types from multiple subschemas, this can and should be avoided entirely.

A related larger issue is that delegateToSchema in upstream graphql-tools always returns a promise. This is unnecessary as well just as execute can sometimes be sync.

[yaacovCR]

Above tracked in separate issue #36

[yaacovCR]

@BrendanBall, if you can let me know if you catch any additional errors, that would be amazing. If you need me to look over your dataloader code further, I would be glad to.

Thank you so much for your help. Your reproduction repo was extremely helpful. I am very glad that fixing #36 came out of this.

[yaacovCR]

@BrendanBall I added returnType option so you can override info.returnType without hacks. I also know export the new createDelegatingRequest method that may be useful for dataloaders. Hope your upgrade path to graphql-tools-fork goes smoothly. 🙂