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.

Sign up for GitHub

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

Jump to bottom

Handle external types that do not exist gracefully #3914

Closed
wants to merge 3 commits into from
Closed

Handle external types that do not exist gracefully #3914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9b00c4
Handle unresolved elements gracefully
mduesterhoeft Mar 22, 2020
203f20f
Add changelog entry
mduesterhoeft May 25, 2020
df8fcd6
Fix test printing sdl
mduesterhoeft Aug 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/apollo-federation-integration-testsuite/src/fixtures/product.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export const typeDefs = gql`

extend type Query {
product(upc: String!): Product
products(upcs: [String!]!): [Product]
vehicle(id: String!): Vehicle
topProducts(first: Int = 5): [Product]
topCars(first: Int = 5): [Car]
Expand Down Expand Up @@ -151,7 +152,8 @@ const products = [
{ __typename: 'Book', isbn: '0201633612', price: 49 },
{ __typename: 'Book', isbn: '1234567890', price: 59 },
{ __typename: 'Book', isbn: '404404404', price: 0 },
{ __typename: 'Book', isbn: '0987654321', price: 29 },
{ __typename: 'Book', isbn: '0987654321', price: 29, upc: '0987654321' },
{ __typename: 'Book', isbn: '9999999999', price: 31, upc: '9999999999' },
];

const vehicles = [
Expand Down Expand Up @@ -232,6 +234,10 @@ export const resolvers: GraphQLResolverMap<any> = {
product(_, args) {
return products.find(product => product.upc === args.upc);
},
products(_, args) {
const upcs: Array<string> = args.upcs
return upcs.map(upc => products.find(product => product.upc === upc));
},
vehicle(_, args) {
return vehicles.find(vehicles => vehicles.id === args.id);
},
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-federation/src/service/__tests__/printComposedSdl.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ describe('printComposedSdl', () => {
library(id: ID!): Library @resolve(graph: \\"books\\")
body: Body! @resolve(graph: \\"documents\\")
product(upc: String!): Product @resolve(graph: \\"product\\")
products(upcs: [String!]!): [Product] @resolve(graph: \\"product\\")
vehicle(id: String!): Vehicle @resolve(graph: \\"product\\")
topProducts(first: Int = 5): [Product] @resolve(graph: \\"product\\")
topCars(first: Int = 5): [Car] @resolve(graph: \\"product\\")
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-federation/src/service/__tests__/printFederatedSchema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ describe('printFederatedSchema', () => {
library(id: ID!): Library
body: Body!
product(upc: String!): Product
products(upcs: [String!]!): [Product]
vehicle(id: String!): Vehicle
topProducts(first: Int = 5): [Product]
topCars(first: Int = 5): [Car]
Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-gateway/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.

- _Nothing yet! Stay tuned!_
- __FIX__: Continue resolving when an `@external` reference cannot be resolved. [#3914](https://github.com/apollographql/apollo-server/pull/3914)

## v0.19.0

Expand Down
46 changes: 46 additions & 0 deletions packages/apollo-gateway/src/__tests__/executeQueryPlan.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,4 +641,50 @@ describe('executeQueryPlan', () => {
}
`);
});

it('can execute queries with selections on unresolved @requires fields', async () => {
// query a book not known to the book service
const query = gql`
query {
products(upcs: ["9999999999", "0987654321"]) {
upc
name
price
}
}
`;

const operationContext = buildOperationContext(schema, query);
const queryPlan = buildQueryPlan(operationContext);

const response = await executeQueryPlan(
queryPlan,
serviceMap,
buildRequestContext(),
operationContext,
);

expect(response.errors?.map(e => e.message)).toEqual(expect.arrayContaining([
"Cannot return null for non-nullable field Book.isbn.",
"Field \"title\" was not found in response.",
"Field \"year\" was not found in response.",
]));

expect(response.data).toMatchInlineSnapshot(`
Object {
"products": Array [
Object {
"name": "undefined (undefined)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part gives me the most pause.

The string with undefined bits is a result of the name resolver receiving unexpectedly missing @requires data, but carrying on anyway. If the requires fields were nested, this would throw.

name(object, { delimeter }) {
  return `${object.title}${delimeter}(${object.year})`;
},

Does this mean our resolvers needs to treat all @requires fields as nullable? I think so! Is that unreasonable or terrible, maybe not - maybe it's the best we can do? In any case, I think we need to document this since it's a potential pitfall of any @requires fields that our resolvers expect to have. And with a wider lens, I think we could set the same expectations for any requests that cross service boundaries like this.

I don't think I have any real ask with this comment, but I do want to raise concerns and instigate conversation about the implications here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review @trevor-scheer . I also was wondering about this case. In this case here both fields are nullable. And I think external fields should always be. Maybe we should document this as a kind of best-practice.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - this isn't something that's documented much in the Federation docs, but extending with non-nullable fields makes your aggregated service not fault-tolerant at all.

"price": "31",
"upc": "9999999999",
},
Object {
"name": "No Books Like This Book! (2019)",
"price": "29",
"upc": "0987654321",
},
],
}
`);
});
});
14 changes: 9 additions & 5 deletions packages/apollo-gateway/src/executeQueryPlan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ async function executeFetch<TContext>(
const representationToEntity: number[] = [];

entities.forEach((entity, index) => {
const representation = executeSelectionSet(entity, requires);
const representation = executeSelectionSet(entity, requires, context);
if (representation && representation[TypeNameMetaFieldDef.name]) {
representations.push(representation);
representationToEntity.push(index);
Expand Down Expand Up @@ -386,9 +386,10 @@ async function executeFetch<TContext>(
* @param source Result of GraphQL execution.
* @param selectionSet
*/
function executeSelectionSet(
function executeSelectionSet<TContext>(
source: Record<string, any> | null,
selectionSet: SelectionSetNode,
context: ExecutionContext<TContext>,
): Record<string, any> | null {

// If the underlying service has returned null for the parent (source)
Expand All @@ -406,16 +407,19 @@ function executeSelectionSet(
const selectionSet = selection.selectionSet;

if (typeof source[responseName] === 'undefined') {
throw new Error(`Field "${responseName}" was not found in response.`);
// collect error but continue to resolve
context.errors.push(new GraphQLError(`Field "${responseName}" was not found in response.`));
break;
}
if (Array.isArray(source[responseName])) {
result[responseName] = source[responseName].map((value: any) =>
selectionSet ? executeSelectionSet(value, selectionSet) : value,
selectionSet ? executeSelectionSet(value, selectionSet, context) : value,
);
} else if (selectionSet) {
result[responseName] = executeSelectionSet(
source[responseName],
selectionSet,
context
);
} else {
result[responseName] = source[responseName];
Expand All @@ -430,7 +434,7 @@ function executeSelectionSet(
if (typename === selection.typeCondition.name.value) {
deepMerge(
result,
executeSelectionSet(source, selection.selectionSet),
executeSelectionSet(source, selection.selectionSet, context),
);
}
break;
Expand Down