-
-
Notifications
You must be signed in to change notification settings - Fork 810
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
Greedy selection of merged fields with dependencies #1959
Comments
not sure this is a field selection set issue our algorithm currently just assumes that each subservice should be visited only once it therefore subtracts the fields from the subschemas visited so far and visits only subschemas not yet visited but, even without a field selection set, i.e. if networkId is moved to the object-level selectionSet, the originating subservice in this example must be visited twice this sounds like a true bug in the algorithm |
Ah, I see not a bug in field selectionSet also not a bug in the algorithm.... Just assumed that each subservice can truly work independently, which differs from federation... Deferring field resolution could be considered as an enhancement, of course with algorithm changes, but I guess for now working as designed |
I should also have provided some more context here because this example is contrived as a root query. In practice, where this commonly happens is when Post is returned as an ID-only stub. For example, a “recentPosts” service might return id-only stubs representing the posts. The practical issue is that “recentPosts” would include the selection for fields it may not have dependency data for. While it seems counter-intuitive that we’d visit the same service twice at a glance, this is how basic stitch works... fields have dependencies and talk to SOME service to fulfill them, and coincidence would have it that sometimes it’s the same service.
|
Circling back, to distill down the insights learned here... it sounds like what we have are static and dynamic types:
So where we run into trouble is with a hybrid type that has both static and dynamic data. This doesn’t work because the static data could originate in a subservice without gateway input present during the delegation round. I think the docs can be updated to articulate this differentiation. I do think there’s a strong practical case to be made for considering a staged resolution approach that builds around dependency groups rather than just schema membership in the long run. |
Hmmm. Not sure I understand all of above, but I'm looking forward to reading through the example more and trying to get it. What I am wondering from example above is whether the network id should be wrapped in a network type by transform prior to merging... Would that solve/change anything? |
I think I understand a bit more and that it wouldn't. Overall, would like to get query batching in play to replace this federation-type behavior. I think the easiest pattern to teach and implement is where each GraphQL schema is valid on its own. Would be in favor of docs noting that federation type schemas are not necessarily recommended. |
Query batching is definitely a huge feature, and I certainly don’t mean to
distract from that. This is an interesting prospect — with that mechanism
in place, many delegated fields could then stand to rival federation in
terms of batch performance (although I can’t speak to the execution costs
associated with one big delegation of fields versus many individual ones.
Are they comparable?). So the strategy would be to turn federation on its
head with that stub types pattern — rather than one partial type with ten
associations placed into service A, you’d instead place stubs of the ten
associated types into service B? I hadn’t considered stubs at that scale
before due to performance implications. It’d be relatively intuitive,
although it’d be messier from the schema standpoint in that nothing is
really organized by concern... there’d just be stubs everywhere. Is that
bad? I dunno. I might have to try building a quick skeleton of our main
schema laid out that way to study it; will pass it along if I do, it’d be a
good test subject for studying query batching. Certainly, the stub types
are nice in that they hide the underlying IDs in the schema.
Pleasure working with you on this; it’s mind blowing stuff how certain
alternative channels can crack the surface on viable new workflows.
Oh, as for docs — let’s leave things as they stand until query batching
comes together. I’ll be interested in studying it, and that may result in a
totally new narrative. Awesome work on all this!
…On Sat, Aug 29, 2020 at 4:07 PM Yaacov Rydzinski ***@***.***> wrote:
I think I understand a bit more and that it wouldn't.
Overall, would like to get query batching in play to replace this
federation-type behavior.
I think the easiest pattern to teach and implement is where each GraphQL
schema is valid on its own. Would be in favor of docs noting that
federation type schemas are not necessarily recommended.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1959 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFRROHLRXCXQHYNCP5BJJ3SDFNY7ANCNFSM4QNWJYYA>
.
|
Okay, one other small detail that is actually a major consideration with
the prospect of exploding the scale of stub types: docstrings. As far as I
can tell at the type level, the last type definition in the list of
subschemas provides the final docstring. We’re already wrestling with the
“where does this docstring go” question now that we have a few duplicate
types. The prospect of putting stubs everywhere would make this a hugely
messy problem. Would it be possibly to, say, have merging ignore any
docstring that starts with “!!” (or something), which would allow the
official docstring to go with the main type definition in any service, and
all other services could opt out of providing a docstring with a helpful
note to developers like, “!! do not document this type here”. Possible?
On Sat, Aug 29, 2020 at 10:51 PM Greg MacWilliam <gmacwill77@gmail.com>
wrote:
… Query batching is definitely a huge feature, and I certainly don’t mean to
distract from that. This is an interesting prospect — with that mechanism
in place, many delegated fields could then stand to rival federation in
terms of batch performance (although I can’t speak to the execution costs
associated with one big delegation of fields versus many individual ones.
Are they comparable?). So the strategy would be to turn federation on its
head with that stub types pattern — rather than one partial type with ten
associations placed into service A, you’d instead place stubs of the ten
associated types into service B? I hadn’t considered stubs at that scale
before due to performance implications. It’d be relatively intuitive,
although it’d be messier from the schema standpoint in that nothing is
really organized by concern... there’d just be stubs everywhere. Is that
bad? I dunno. I might have to try building a quick skeleton of our main
schema laid out that way to study it; will pass it along if I do, it’d be a
good test subject for studying query batching. Certainly, the stub types
are nice in that they hide the underlying IDs in the schema.
Pleasure working with you on this; it’s mind blowing stuff how certain
alternative channels can crack the surface on viable new workflows.
Oh, as for docs — let’s leave things as they stand until query batching
comes together. I’ll be interested in studying it, and that may result in a
totally new narrative. Awesome work on all this!
On Sat, Aug 29, 2020 at 4:07 PM Yaacov Rydzinski ***@***.***>
wrote:
>
>
> I think I understand a bit more and that it wouldn't.
>
>
> Overall, would like to get query batching in play to replace this
> federation-type behavior.
>
>
> I think the easiest pattern to teach and implement is where each GraphQL
> schema is valid on its own. Would be in favor of docs noting that
> federation type schemas are not necessarily recommended.
>
>
>
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#1959 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFRROHLRXCXQHYNCP5BJJ3SDFNY7ANCNFSM4QNWJYYA>
> .
>
>
>
|
I think best would be to provide some generic options to configure how types and fields are merged. For example, this code picks the last description for a type and picks up into the gateway the last version of a field form all the subschemas: graphql-tools/packages/stitch/src/mergeCandidates.ts Lines 60 to 70 in e60a814
Something like |
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Does not pass `info` argument to the batched executor call, disabling any executors that used extensions annotated onto info. TODO: - Add testing! - Extensions support should be added by a custom option? Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Does not pass `info` argument to the batched executor call, disabling any executors that used extensions annotated onto info. TODO: - Add testing! - Extensions support should be added by a custom option? Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Does not pass `info` argument to the batched executor call, disabling any executors that used extensions annotated onto info. TODO: - Add testing! - Extensions support should be added by a custom option? Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
|
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. TODO: - Add testing! Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954 fix
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. TODO: - Add testing! Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. TODO: - Add testing! Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
Hmmm. I am wondering if instead of using I think I would prefer that... |
I see, so there’s already an onTypeConflict that can handle type-level
docstring mashing? This is another prime area for documentation explaining
how to do this.
…On Mon, Aug 31, 2020 at 3:20 PM Yaacov Rydzinski ***@***.***> wrote:
Hmmm. I am wondering if instead of using onDescriptionConflict and
onDeprecatedConflict, it would be better to have one simple
onFieldConflict option...
I think I would prefer that...
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1959 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFRROG65VSEIYZPDKPWQFDSDPZXDANCNFSM4QNWJYYA>
.
|
Ah, made a mistake already, |
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. TODO: - Add additional testing! Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
Not exactly. When merging types, the fields and attributes require a host of custom merging that I think we will need separate functions for each type attribute that could conflict, i.e. |
The default |
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. TODO: - Add additional testing! Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. TODO: - Add additional testing! Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
WIP: When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. TODO: - Add additional testing! Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
When `batch` is set to true for a given subschemaConfig, batches all delegated root fields into a combined request passed to the executor. Moreover, batches all requests to a given subschema into the minimum number of requests, collecting queries and mutations separately, preserving operation order. Distributes properly pathed errors to the originating requests. Adapted from Gatsby query batcher by @vladar. Caveats: * Uses a Dataloader under the hood, which is created anew upon each request -- relies on a unique context argument per request to make this happen! * Passed `info` argument from first executor call to the batched executor call, making info argument unreliable. Related: gatsbyjs/gatsby#22347 (comment) #1710 (comment) #1959 (comment) #1954
Okay, I think I follow all that. Using the final definition of a field is
pretty intuitive, and we’ve been generally able to be pretty productive
around that known behavior. Also, the default technical merging behavior of
both fields and types has proven itself to be pretty darn good, and as
implementors we don’t really want to mess around with something dealing
with internals unless we absolutely have to (go with the flow, and enjoy
library updates as they come!)
So, I would say that the docstring conflict scenario is the one boilerplate
case where we do want to get involved with the merge while not touching
anything technical. My suggestion would be this:
onDescriptionConflict(oldStr, newStr, Type, Field?): string
This would call whenever a conflict was encountered on a type description
or a field description. OldStr is the working description on the merged
type/field, and newStr is the newly encountered description. Type would
always be passed as the current merge candidate type, and Field would
optionally be passed when this conflict happens on a field. The method
returns a string, which is the resolved description to use on the merged
element. This would also be useful to run on InputObjectTypes.
…On Mon, Aug 31, 2020 at 3:56 PM Yaacov Rydzinski ***@***.***> wrote:
The default onTypeConflict behavior when stitching btw is to just take
the last type.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1959 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFRROE3ZRTKI75NJ6OLQ6LSDP555ANCNFSM4QNWJYYA>
.
|
I’ve got some catching up to do, but I like this approach of not wrapping
stitchSchema and using some kind of semi-formal plugin system. The computed
request scope thing seemed like a pretty small feature to add a full
breakout wrapper for.
Also, docs-wise this still works pretty well in that the merge guide can be
reduced down to static schemas and an “advanced” guide page can be added
for more involved patterns.
…On Fri, Sep 4, 2020 at 4:03 AM Yaacov Rydzinski ***@***.***> wrote:
I reworked subschema config sets to just be arrays of subschema config
objects #2001 <#2001>
The helper function can operate on a subschema config object and return a
new subschema config object or an array of them that can either be spread
or passed whole within the subschemas property of stitchSchemas.
See
https://github.com/ardatan/graphql-tools/blob/6a2e5ab8a1efd4b7524437ee0be5a03d7f312b1e/packages/delegate/tests/batchExecution.test.ts#L64-L177
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1959 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFRROGL6TSTX4J74CKGPZ3SECNNRANCNFSM4QNWJYYA>
.
|
I think your mention of a const gatewaySchema = stitchSchemas({
subschemas: [ ... ],
mergeTypes: true,
plugins: [
new ComputedFieldsPlugin(),
new CombineDescriptionsPlugin({ excludePrefix: '!omit' }),
new UndeprecateFieldsPlugin({ withReason: 'gateway access only' }),
]
}); |
I imagine each plugin would take each SubschemaConfig object as parameter and return modified version for next plugin in pipeline? |
I was envisioning that plugins would be at the level of the stitched schema, not the level of subschemas. That way a plugin could tap into numerous aspects of the stitched configuration. I just pushed an update to #1989 that demonstrates the proposed pattern. |
Actually, I guess the plugins would have to take the whole options object as input and return modified version. Should we allow plugins to add additional plugins then? I would imagine not 🙂 |
Large scale engines like Webpack are pretty hands-off about that. A plug-in
has access to the whole config, and can customize as needed for its
objectives. Plugins adding plugins isn’t unheard of because a responder may
want to lock in at multiple priorities within the queue (front, back, etc), or install supporting modules that it takes advantage of, assuming they weren’t installed manually.
So all that said, probably the simplest and most conventional approach is
to provide everything and call it a day.
…On Sat, Sep 5, 2020 at 2:49 PM Yaacov Rydzinski ***@***.***> wrote:
Actually, I guess the plugins would have to take the whole options object
as input and return modified version.
Should we allow plugins to add additional plugins then? I would imagine
not 🙂
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1959 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFRROCKMXGZ7KA2C74AO43SEKB4TANCNFSM4QNWJYYA>
.
|
See my latest changes in #1989, it appears that field selection set isolation can cause a subschema that properly returns its fields to fail, if it returns new data not present in the other subschemas.
When a product with upc of 4 is returned from the inventory schema along with price and weight, the inventory schema on its own can calculate the shippingEstimate. If you enable field selection set isolation, the gateway cannot, because upc of 4 is not present in productSchema, Similarly, for upc of 3, when enabling field selection set isolation, price and weight are read from the product schema, even though supplied by the inventory schema, so that results may differ from schema to gateway. What should the default behavior be? Presumably, it should be up to configuration on the gateway, but I do not think it is obvious that isolation should be enabled by default. It depends on the expectations of how the schema resolvers were written. If, for example, price and weight are never returned, presumably field selection set isolation should be enabled. But that is difficult to automatically introspect.... |
Trying to follow along... it's a little confusing because I don't see a
The inventory schema doesn't provide fields for
This sounds like a data error by any standard that has nothing to do with computed fields. Products schema is the only place that defines
This sounds right to me for computed fields flow. The only thing that sounds off is that inventory schema is doing anything with local price and weight values when the expectation is that the gateway provides those values. Bottom line is – when a field is marked as required, it is expecting its value to come from the gateway (and in turn–from some other service elsewhere). The developer has enabled a hard |
ALL THAT SAID – I feel like we're getting into a gray area here where we've added a pretty high-touch feature that (with all due respect) doesn't sound like you're super comfortable with the implementation or use of. I wouldn't be the least bit offended if we were to scrap #1989 and scratch it off the board. It's going to be a bigger liability than boon in the long term if the primary author isn't comfortable with it being there. Type merging is still damn good without it, and personally I can navigate my way around without it there. I was only promoting it because I saw an opportunity for a general purpose feature that others looking for a federation alternative could benefit from. |
Just to make a final appeal here before scrapping this or shipping a Main note from the original test:
^^ That service isn't realistic for several reasons. The import { graphql } from 'graphql';
import { makeExecutableSchema } from '@graphql-tools/schema';
import { ExecutionResult } from '@graphql-tools/utils';
import { stitchSchemas } from '@graphql-tools/stitch';
describe('merging using type merging', () => {
const users = [
{
id: '1',
name: 'Ada Lovelace',
birthDate: '1815-12-10',
username: '@ada'
},
{
id: '2',
name: 'Alan Turing',
birthDate: '1912-06-23',
username: '@complete',
},
];
const accountsSchema = makeExecutableSchema({
typeDefs: `
type Query {
me: User
_userById(id: ID!): User
_usersById(ids: [ID!]!): [User]
}
type User {
id: ID!
name: String
username: String
}
`,
resolvers: {
Query: {
me: () => users[0],
_userById: (_root, { id }) => users.find(user => user.id === id),
_usersById: (_root, { ids }) => ids.map((id: any) => users.find(user => user.id === id)),
},
},
});
const inventory = [
{ upc: '1', inStock: true },
{ upc: '2', inStock: false },
{ upc: '3', inStock: true }
];
const inventorySchema = makeExecutableSchema({
typeDefs: `
directive @requires(selectionSet: String!) on FIELD_DEFINITION
input ProductRepresentation {
upc: String!
price: Int
weight: Int
}
type Product {
upc: String!
inStock: Boolean
shippingEstimate: Int @requires(selectionSet: "{ price weight }")
}
type Query {
mostStockedProduct: Product
_products(representations: [ProductRepresentation!]!): [Product]!
}
`,
resolvers: {
Product: {
shippingEstimate: product => {
if (product.price > 1000) {
return 0 // free for expensive items
}
return Math.round(product.weight * 0.5) || null; // estimate is based on weight
}
},
Query: {
mostStockedProduct: () => inventory.find(i => i.upc === '3'),
_products: (_root, { representations }) => {
return representations.map(rep => ({ ...rep, ...inventory.find(i => i.upc === rep.upc) }));
},
},
},
});
const products = [
{
upc: '1',
name: 'Table',
price: 899,
weight: 100
},
{
upc: '2',
name: 'Couch',
price: 1299,
weight: 1000
},
{
upc: '3',
name: 'Chair',
price: 54,
weight: 50
}
];
const productsSchema = makeExecutableSchema({
typeDefs: `
type Query {
topProducts(first: Int = 2): [Product]
_productByUpc(upc: String!): Product
_productsByUpc(upcs: [String!]!): [Product]
}
type Product {
upc: String!
name: String
price: Int
weight: Int
}
`,
resolvers: {
Query: {
topProducts: (_root, args) => products.slice(0, args.first),
_productByUpc: (_root, { upc }) => products.find(product => product.upc === upc),
_productsByUpc: (_root, { upcs }) => upcs.map((upc: any) => products.find(product => product.upc === upc)),
}
},
});
const usernames = [
{ id: '1', username: '@ada' },
{ id: '2', username: '@complete' },
];
const reviews = [
{
id: '1',
authorId: '1',
product: { upc: '1' },
body: 'Love it!',
},
{
id: '2',
authorId: '1',
product: { upc: '2' },
body: 'Too expensive.',
},
{
id: '3',
authorId: '2',
product: { upc: '3' },
body: 'Could be better.',
},
{
id: '4',
authorId: '2',
product: { upc: '1' },
body: 'Prefer something else.',
},
];
const reviewsSchema = makeExecutableSchema({
typeDefs: `
type Review {
id: ID!
body: String
author: User
product: Product
}
type User {
id: ID!
username: String
numberOfReviews: Int
reviews: [Review]
}
type Product {
upc: String!
reviews: [Review]
}
type Query {
_userById(id: ID!): User
_usersById(ids: [ID!]!): [User]
_reviewById(id: ID!): Review
_productByUpc(upc: String!): Product
_productsByUpc(upcs: [String!]!): [Product]
}
`,
resolvers: {
Review: {
author: (review) => ({ __typename: 'User', id: review.authorId }),
},
User: {
reviews: (user) => reviews.filter(review => review.authorId === user.id),
numberOfReviews: (user) => reviews.filter(review => review.authorId === user.id).length,
username: (user) => {
const found = usernames.find(username => username.id === user.id)
return found ? found.username : null
},
},
Product: {
reviews: (product) => reviews.filter(review => review.product.upc === product.upc),
},
Query: {
_reviewById: (_root, { id }) => reviews.find(review => review.id === id),
_userById: (_root, { id }) => ({ id }),
_usersById: (_root, { ids }) => ids.map((id: string) => ({ id })),
_productByUpc: (_, { upc }) => ({ upc }),
_productsByUpc: (_, { upcs }) => upcs.map((upc: string) => ({ upc })),
},
}
});
const stitchedSchema = stitchSchemas({
subschemas: [
{
schema: accountsSchema,
merge: {
User: {
selectionSet: '{ id }',
fieldName: '_userById',
args: ({ id }) => ({ id })
}
},
batch: true
},
{
schema: inventorySchema,
enableFieldSelectionSetIsolation: true,
merge: {
Product: {
selectionSet: '{ upc }',
fieldName: '_products',
key: ({ upc, weight, price }) => ({ upc, weight, price }),
argsFromKeys: (representations) => ({ representations }),
}
},
batch: true
},
{
schema: productsSchema,
merge: {
Product: {
selectionSet: '{ upc }',
fieldName: '_productByUpc',
args: ({ upc }) => ({ upc }),
}
},
batch: true
},
{
schema: reviewsSchema,
merge: {
User: {
selectionSet: '{ id }',
fieldName: '_usersById',
args: (ids) => ({ ids }),
key: ({ id }) => id,
},
Product: {
selectionSet: '{ upc }',
fieldName: '_productByUpc',
args: ({ upc }) => ({ upc }),
},
},
batch: true
}],
mergeTypes: true,
});
it('gets most stocked product', async () => {
const result = await graphql(
stitchedSchema, `
query {
topProducts(first: 2) {
upc
shippingEstimate
}
mostStockedProduct {
upc
shippingEstimate
}
}
`,
undefined,
{},
);
console.log(JSON.stringify(result, null, 2))
});
}); |
Hmmm... actually, looks like the present implementation is back to only visiting each subschema once, or else isolation isnt working properly anymore. This has proper shipping estimates but no it('gets most stocked product', async () => {
const result = await graphql(
stitchedSchema, `
query {
topProducts(first: 2) {
upc
shippingEstimate
inStock
}
mostStockedProduct {
upc
shippingEstimate
inStock
}
}
`,
undefined,
{},
);
console.log(JSON.stringify(result, null, 2))
}); While this has it('gets most stocked product', async () => {
const result = await graphql(
stitchedSchema, `
query {
topProducts(first: 2) {
upc
inStock
shippingEstimate
}
mostStockedProduct {
upc
inStock
shippingEstimate
}
}
`,
undefined,
{},
);
console.log(JSON.stringify(result, null, 2))
}); It would appear that we're back to only visiting each subservice once here, and the two portions of the inventory schema are together only visited once? |
There's no single products database potentially, as you mentioned in terms of stale data, could be multiple, especially in terms of remote schemas not under control. So which is stale? To prefer the database with exported fields is one choice, but it is perfectly valid to have the product object contain metadata that is not available via GraphQL, but possibly more accurate... in terms of calculating the shipping estimate. I think the default of setting the option to true makes sense if you are in control of all the subschemas, as in your use case, but potentially false when thinking about remote schemas not under control. Easiest thing is putting it under an option, either once for the entire stitch schemas, or for each sub schema, or for each field... We can also consider instead of a flag, putting the selectionSet data for fields meant to be isolated under something other than fields, like gatewayFields, that is essentially like making the option enabled on a per field basis... In terms of bug, we'll have to investigate! All tests pass under current implementation, so looks like we need more tests prior to shipping. |
Can you add the failing tests and I will work on it? |
could not reproduce your failing test, in the meantime i switched the test that i did add to use the field selection set isolation method I renamed the property switch to |
a bit snazzier than |
I guess I still don't think that a single Then to some points above:
Sure, but in this case, there's no reason to put an
I guess I'd disagree here. Let's say we're consuming a third-party federation service and we read a field marked with So, all told – I still think this feature makes the most sense as it was originally written: a field marked as required is isolated for the simple purpose of making it always send its required data. No required fields means nothing is isolated. |
useGatewayData false with field selection sets is still useful because when the field is not requested, the gateway does not need to add the selection set. that would break a schema that is not expecting it when the fresh data is at the remote subschema... but it sounds like you are agreeing that it is okay to have a flag with this behavior off by default I think it makes sense for the directive to be called requires, but within the mergeConfig it kind of doesn't make sense, because all fields "require" the selection set, it makes sense to call it "federate: true" or something like that... |
Just added this test here that demonstrates the failure: e1533fc When querying |
And then the default for the requires directive could be to have federate true, optionally have federate false |
I still don't fully understand the subservice level flag; but at the field level, yes, I would agree that this would be a field level flag that would be off by default and opted into. What the field is called in field level config I guess I'm not willing to go to battle over, but I still think So does this mean you have to enable a subservice-level flag in order to activate @requires behavior? If so, that feels weird to me that there's a setting to activate a setting. |
If we have federate or some other field level flag we don't need a subschema level flag, correct |
The problem with required as name for merge config is that setting it to false is a bit misleading, the gateway is still required to request the selection set in order to resolve the field unless the data originates from that sub service... Within the directive, requires is the name of the directive and we are never setting it to false, we would have a different flag with perhaps a different default such as federate to indicate whether we are using the isolating behavior.... This sounds like the best option to me... |
Not quite sure about that failing test, I wonder what happens when you turn batching off... Hopefully will be able to take a look at it in a few hours |
Sorry I forced push probably should have merge master |
#1989 merged! |
Available in latest release... |
It appears that fields with selection dependencies are greedily selected early if merging visits their service prior to collecting their dependencies. Logistically we'd need to visit their service but not collect them, go elsewhere for dependencies, and then return for the dependent fields.
Actually, I'm not sure this is specifically an issue with field-level selections. Using a single static
selectionSet: '{ id networkId }'
also has the same issue. Either way, I may have led us down a bad road here with the nudge to support federation-style injection patterns – your ethos of "if a service has a field it should be able to fulfill it" avoids this completely. If that's a fundamental divide in the goals of the algorithm, it would be totally reasonable if you want to stick to your original path and drop official support for these dependency patterns. We could pullinjected keys
from docs and all mentions of Federation similarities, letting it just be its own thing.The text was updated successfully, but these errors were encountered: