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

mergeSchemas breaks the recursive nature of resolvers #443

Closed
exogen opened this issue Oct 19, 2017 · 6 comments
Closed

mergeSchemas breaks the recursive nature of resolvers #443

exogen opened this issue Oct 19, 2017 · 6 comments

Comments

@exogen
Copy link
Contributor

exogen commented Oct 19, 2017

I noticed that if a new object type field is added via mergeSchemas, that type’s existing resolvers aren't used for that field, which definitely seems like a bug. More info here: a simple demo with repro instructions: https://github.com/exogen/schema-stitching-bugs

@exogen
Copy link
Contributor Author

exogen commented Oct 19, 2017

I added a 4th demo to the repo of using GraphQL.js’ (undocumented) extendSchema followed by addResolverFunctionsToSchema, which works – it seems like mergeSchemas should behave roughly the same way as doing that.

@imranolas
Copy link
Contributor

imranolas commented Oct 19, 2017

In example 3 the issue is that you are merging in a typeDef rather than an executable schema.

I'm not sure it makes sense to extend a schema in this way. From what I understand the intention for mergeSchema is to merge published executable schemas rather than extend existing schemas. The second schema in your example is not a valid schema alone as it only declares a type extension.

I could well be wrong though :)

@imranolas
Copy link
Contributor

Ignore me. I am wrong.

@lifeiscontent
Copy link

@exogen I think this might be a similar problem I'm running into as well. #432

@freiksenet
Copy link
Contributor

This looks like a bug, thank you!

@freiksenet freiksenet added the bug label Nov 28, 2017
@yaacovCR
Copy link
Collaborator

This is not a bug, but rather the way stitching works. The OP hopes that the original schema that mergeSchemas modifies the original schema with addition of a new field. That, however, is what graphql-js extendSchema does.

mergeSchemas, however, should really be called stitchSchemas, because it merges multiple schemas using proxying so that remote schemas can also be merged. This means that new fields added to "merged" or proxied subschemas will get resolvers whose first argument (source, root, parent, whatever you want to call it) is not equivalent to the value within the original schema, but rather a value only containing the requested fields. Similarly, although types from all subschemas are present in the merged schema, the resolver will just be the defaultMergeResolver that attempts to extract the merged result (resolving aliases, etc), and not the resolver from the original schema.

To accomplish what the OP has in mind, the new field added here must delegate to the subschema in question, which in the provided example is not possible, as a root query type for bar is not exposed. In theory, if the Bar type was exposed any which way, transforms would be possible to get at it. Bar isn't used at all in the subschema, as the example is somewhat contrived, but this could be a real world problem in some examples.

This is not a bug, but could be a design flaw. But the question is, what is the OP trying to accomplish? If you are trying to merge schemas of which you have local control, graphql-merge-schemas or graphql-compose may be better options, Schema stitching is for "merging" schemas that are or could be remote, i.e. when you do not have access to their internal workings, resolvers, serialize/parse functions, parent values. Of course, it works for local schemas as well, because remote schemas are made to appear local, but that is not the primary use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants