-
-
Notifications
You must be signed in to change notification settings - Fork 816
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
[Do not merge] 2.0 with schema stitching #382
Conversation
Made the tests pass by adding One idea: What if instead of prefixing field names, we put them in a nested field? Like: # current
query {
Booking_bookingById {
...
}
}
# proposed
query {
Booking {
bookingById { ... }
}
} |
1e62433
to
ff5d9b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments, more to come
src/stitching/TypeRegistry.ts
Outdated
|
||
public getSchema(name: string): GraphQLSchema { | ||
if (!this.schemas[name]) { | ||
throw new Error(`No such type: ${name}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "no such schema" - do we even need these getters though? Would it be easier to just make schemas
public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it won't type check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait really? Why not?
src/stitching/TypeRegistry.ts
Outdated
return this.linksByType[name] || []; | ||
} | ||
|
||
public getLinkByAddress(typeName: string, link: string): SchemaLink { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe getLinkByFieldName
?
|
||
export default function addSimpleRoutingResolvers( | ||
schema: GraphQLSchema, | ||
// prolly should be a fetcher function like (query) => Promise<result> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - I'll want this for the demo. Might write it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
src/stitching/mergeSchemas.ts
Outdated
const queryFields = registry.query.getFields(); | ||
return fromPairs( | ||
links.map(link => { | ||
const [schemaName, field] = link.to.split('_'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it would be better if we took two separate arguments for schema and field name, instead of splitting them on _
. This couples it to the serialization we choose in a different part of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, whole namespacing related code is a bit of a hack and will be better once we figure out proper namespaces.
RE: prefixing - my initial use-case is to merge an existing in-production schema with a new one. This means that actually I don't want to prefix my existing schema at all, that should be supported shouldn't it? |
@timbotnik say more? Oh you mean you just want to graft a new one onto an existing one? I think it would make sense to be able to have no prefix at all. |
@stubailo yes "grafting" is a good word for it ;-) Also, if you knew for sure that there would be no namespace conflicts, it could be useful to support a non-prefixed merge in general. Thinking through a general case for non-prefixed merges, something that would be amazing would be if there was a way to handle de-duplication. I'm thinking more here about things like custom scalars and "utility" types that could be imagined to resolve exactly the same way on either endpoint. Maybe more complicated, but also worth considering that an interface type might be shared between two schemas but each schema provides a different implementation. |
Yeah - I think the deduplication is probably going to be a future thing. Do you think it's critical for your initial use case? |
@stubailo I don't think it's critical at all, however the "custom scalar" thing is probably the thing that will look the strangest - like having |
Yeah, that's a great point. We should definitely work towards that. Having a concrete production use case would be super great. |
@timbotnik We didn't really plan for "grafting" use case, some of the code would probably be shared, but I don't think we will support it in first iteration. However it seems that there are lots of use cases in schema merging, so I'll definitely think on how to allow some customization of how stuff will be merged, maybe providing functions to resolve type conflicts. (Having said that, we really don't know what is the end goal of what we are doing, it's very much explanatory thing and a way to collect schema use cases. Maybe grafting is a very common one and then we'll certainly support it) |
@timbotnik Re: de-duplication, our longer term plan involved having namespaces that can depend on other namespaces for shared types. Thus you could have 'common types' as one namespace and use scalars and shared types from there. |
@stubailo This could work for field namespacing, but we would still need namespaces for types. I'm not sure :) |
package.json
Outdated
@@ -49,24 +49,26 @@ | |||
"homepage": "https://github.com/apollostack/graphql-tools#readme", | |||
"dependencies": { | |||
"deprecated-decorator": "^0.1.6", | |||
"lodash": "^4.17.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm for the final release it would be super awesome to not include lodash - for client side use that really kills the bundle size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can either inline them or use lodash/function modules.
@freiksenet I think perhaps the definition of "grafting" is getting confused. By this, all I mean is that I want to use an existing "local" schema (that I already have clients using in production), and merge a new schema onto it. Since the existing schema is already in use in the wild, I don't want to introduce a prefix on that schema. I think the only feature ask here is to allow me to turn off prefixing for this one schema during the merge. |
Just published |
@timbotnik Ah, now I get it. Yeah, I will separate namespacing from merging
today.
…On Thu, 17 Aug 2017, 5.00 timbotnik ***@***.***> wrote:
@freiksenet <https://github.com/freiksenet> I think perhaps the
definition of "grafting" is getting confused. By this, all I mean is that I
want to use an existing "local" schema (that I already have clients using
in production), and merge a new schema onto it. Since the existing schema
is already in use in the wild, I don't want to introduce a prefix on that
schema. I *think* the only feature ask here is to allow me to turn off
prefixing for this one schema during the merge.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#382 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKjiBznbY9X96nZZKYL0Xval4VRzpyxks5sY56ggaJpZM4O3jxQ>
.
|
I'm excited for |
BTW here's my sample code from the talk at GraphQL NYC! This only works with import * as express from "express";
import * as bodyParser from "body-parser";
import { graphqlExpress, graphiqlExpress } from "apollo-server-express";
import { makeRemoteExecutableSchema, mergeSchemas } from "graphql-tools";
import { createApolloFetch } from "apollo-fetch";
async function run() {
// XXX fix typings
const universeSchema = await makeRemoteExecutableSchema(
createApolloFetch({
uri: "https://www.universe.com/graphql/beta"
}) as any
);
const weatherSchema = await makeRemoteExecutableSchema(
createApolloFetch({
uri: "https://5rrx10z19.lp.gql.zone/graphql"
}) as any
);
const schema = mergeSchemas({
schemas: [
{ prefix: "universe", schema: universeSchema },
{ prefix: "weather", schema: weatherSchema }
],
links: [
{
name: "location",
from: "Event",
to: "weather_location",
resolveArgs: parent => ({ place: parent.cityName }),
fragment: `
fragment WeatherLocationArgs on Event {
cityName
}
`
}
]
});
const app = express();
app.use("/graphql", bodyParser.json(), graphqlExpress({ schema }));
app.use(
"/graphiql",
graphiqlExpress({
endpointURL: "/graphql"
})
);
app.listen(3000);
console.log("Listening!");
}
try {
run();
} catch (e) {
console.log(e, e.message, e.stack);
} Here's the query and result: Also here are my slides for the talk: https://www.slideshare.net/sashko1/modular-graphql-with-schema-stitching |
I added tests that check for local schema merging, remote schema merging, and a hybrid of the two. Should help us catch bugs with remote schemas! Already found one :] |
@freiksenet @stubailo this is amazing stuff. so much progress in such a short amount of time. so I have been very busy decomposing our production graphql server over the last couple of weeks. I have decomposed them into something we call GraphQL modules. It's in the perfect state to use this project to test a production use case. The only problem is that I will not be able to share the project. However, we are currently merging/stitching 6 modules together at build time to make up our GraphQL server including a pretty large set of common types so it should be a good use case. I will be able to share snippets as I find problems. Would this be helpful? |
Absolutely! Please let us know whatever problems you run into, and also what features are missing for you to be able to use the tool :) Super excited to see people getting pumped about this. |
} | ||
|
||
const resolvers: { | ||
Query: ResolverMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remote schemas are not working if they have root types that are not called Query
or Mutation
, such as
schema {
query: RootQuery
mutation: RootMutation
}
b/c the schema actually needs to be transformed as well to replace RootQuery
=> Query
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You get errors like Error: "Query" defined in resolvers, but not in schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed that.
How easy to get this working over websockets? |
@gilesbradshaw you could easily get queries and mutations over websockets working with the right fetcher passed in, but for subscriptions it gets a bit more complicated for sure. That would be a big extra feature on top of this. |
4a2e62f
to
8133dca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a changelog entry 😛
@sashko 🖕 This is ready to ship now :) |
@freiksenet where can I find the documentation for this now that this is merged? |
Awesome this seems to be done, but I just tried it and aliases don't work anymore if routed through makeRemoteExecutableSchema. |
@pozylon: Can you elaborate on that and maybe include a code example that shows what doesn't work? |
Of course, sorry, here you go with the query:
I run it in GraphiQL and that was the result without proxying:
And that's what happens with proxying through the stitching:
total is of type "Money" which has a currency and amount (String + Int) |
@pozylon: So are you saying it works if you remove the |
@martijnwalraven: exactly, if i call it without the alias it's all fine. |
@pozylon: Great, thanks for explaining. @freiksenet, do you have any idea what could be causing this? |
I have a micro service (A) which has no protection. is there a recipe on how to (B) can protect (A) queries? Like to if the user authorized or validate logic first? |
@gviligvili: Use a middleware together with apollo-fetch based Fetcher and cancel requests when appropriate. https://github.com/apollographql/apollo-fetch |
@lifeiscontent we'll merge in the docs asap, see https://github.com/apollographql/tools-docs/pull/134 for progress. @pozylon Thanks, i'll check it. |
Hello again, I have a small issue stitching a schema that was automatically generated with postgraphql library(https://github.com/postgraphql/postgraphql). The service works fine on it's own but when i try to mergeSchemas i get "Unhandled promise rejection (rejection id: 1): Error: No such type: Query". I already stitched 3 other services and it works perfectly. You can find here the generated schema https://gist.github.com/filtudo/40ffe1bf27863d736e1646f9576dab4d And here a link with the graphiql schema docs Any help or guidance would be greatly appreciated. Cheers |
@filtudo The reason it doesn't work is because it refers to Query type in each payload. We handle Query type specially, tbh I didn't envision Query used this way. I wonder how common such use case is in practice. |
Any chance on "tackling" this issue on your side(since postgraphql i would say is a pretty decently used library) or i should start thinking of alternatives/hacks for this case? Thank you |
@filtudo It seems to be a common pattern in many APIs to support Relay, so I will fix it, yes. |
@filtudo Fixed in #413 |
@filtudo Published as 2.2.1. @pozylon Your bug is fixed in 2.2.0. Hey folks, this is part of main release now, please create new issues if you have any problems with schema stitching, it's hard to track this thread cause it's closed. |
This is a WIP take on schema-stiching and schema merging. This will be published to a pre-release tag. See
src/test/testingSchemas.js
for examples..API
makeRemoteExecutableSchema({ schema: GraphQLSchema, fetcher: Fetcher }): GraphQLSchema
Given a GraphQL schema (can be a non-executable client schema made by
buildClientSchema
) and a Fetcher, produce a GraphQL Schema that routes all requests to the fetcher.introspectSchema( fetcher: Fetcher, context?: {[key: string]: any} ): Promise<GraphQLSchema>
Use fetcher to build a client schema using introspection query. For easy of use of
makeRemoteExecutableSchema
. Provides a client schema, so a non-executable schema. Accepts optional second argumentcontext
, which is passed to the fetcher.Fetcher
Usage with apollo-link
Usage with apollo-fetch
Usage with a generic HTTP client (like node-fetch)
mergeSchemas
schemas
schemas
can be bothGraphQLSchema
(but it has to be an executable schema) or strings. In case they are strings only extensions (extend type
) will be used. Passing strings is useful to add fields to existing types to link schemas together.resolvers
resolvers
is an optional a function that takes one argument -mergeInfo
andreturns resolvers in makeExecutableSchema format.
mergeInfo and delegate
mergeInfo
currenty is an object with one propeprty -delegate
.delegate
takes operation and root field names, together with GraphQL contextand resolve info, as well as arguments for the root field. It forwards query to
one of the merged schema and makes sure that only relevant fields are requested.
onTypeConflict
onTypeConflict
lets you customize type resolving logic. Default logic is totake the first encountered type of all the types with the same name. This
methods allows customization of this, for example by taking other type or
merging types together.
Example
Known issues
Production ready checklist