Skip to content

Commit

Permalink
fix: handle overlapping fields correctly across subschemas (#6091)
Browse files Browse the repository at this point in the history
* fix: handle overlapping fields correctly across subschemas

* Fix prettier

* Add another changeset

* Introduce a new field
  • Loading branch information
ardatan committed Apr 25, 2024
1 parent c8d45c7 commit 9bca9e0
Show file tree
Hide file tree
Showing 9 changed files with 325 additions and 29 deletions.
34 changes: 34 additions & 0 deletions .changeset/fresh-kangaroos-exist.md
@@ -0,0 +1,34 @@
---
"@graphql-tools/stitch": minor
---

New option `useNonNullableFieldOnConflict` in `typeMergingOptions` of `stitchSchemas`

When you have two schemas like below, you will get a warning about the conflicting fields because `name` field is defined as non-null in one schema and nullable in the other schema, and non-nullable field can exist in the stitched schema because of the order or any other reasons, and this might actually cause an unexpected behavior when you fetch `User.name` from the one who has it as non-nullable.
This option supresses the warning, and takes the field from the schema that has it as non-nullable.

```graphql
type Query {
user: User
}

type User {
id: ID!
name: String
email: String
}
```
And;

```graphql
type Query {
user: User
}

type User {
id: ID!
name: String!
}
```


97 changes: 97 additions & 0 deletions .changeset/many-ads-run.md
@@ -0,0 +1,97 @@
---
"@graphql-tools/federation": patch
"@graphql-tools/delegate": patch
"@graphql-tools/stitch": patch
---

If the gateway receives a query with an overlapping fields for the subschema, it uses aliases to resolve it correctly.

Let's say subschema A has the following schema;

```graphql
type Query {
user: User
}

interface User {
id: ID!
name: String!
}

type Admin implements User {
id: ID!
name: String!
role: String!
}

type Customer implements User {
id: ID!
name: String
email: String
}
```

And let's say the gateway has the following schema instead;

```graphql
type Query {
user: User
}

interface User {
id: ID!
name: String!
}

type Admin implements User {
id: ID!
name: String!
role: String!
}

type Customer implements User {
id: ID!
name: String!
email: String!
}
```

In this case, the following query is fine for the gateway but for the subschema, it's not;

```graphql
query {
user {
... on Admin {
id
name # This is nullable in the subschema
role
}
... on Customer {
id
name # This is non-nullable in the subschema
email
}
}
}
```

So the subgraph will throw based on this rule [OverlappingFieldsCanBeMerged](https://github.com/graphql/graphql-js/blob/main/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts)

To avoid this, the gateway will use aliases to resolve the query correctly. The query will be transformed to the following;

```graphql
query {
user {
... on Admin {
id
name # This is nullable in the subschema
role
}
... on Customer {
id
name: _nullable_name # This is non-nullable in the subschema
email
}
}
}
```
114 changes: 114 additions & 0 deletions packages/delegate/src/OverlappingAliasesTransform.ts
@@ -0,0 +1,114 @@
import { isNullableType, Kind, visit } from 'graphql';
import { ExecutionRequest, ExecutionResult } from '@graphql-tools/utils';
import { DelegationContext, Transform } from './types.js';

const OverlappingAliases = Symbol('OverlappingAliases');

interface OverlappingAliasesContext {
[OverlappingAliases]: boolean;
}

export class OverlappingAliasesTransform<TContext>
implements Transform<OverlappingAliasesContext, TContext>
{
transformRequest(
request: ExecutionRequest,
delegationContext: DelegationContext<TContext>,
transformationContext: OverlappingAliasesContext,
) {
const newDocument = visit(request.document, {
[Kind.SELECTION_SET]: node => {
const seenNonNullable = new Set<string>();
const seenNullable = new Set<string>();
return {
...node,
selections: node.selections.map(selection => {
if (selection.kind === Kind.INLINE_FRAGMENT) {
const selectionTypeName = selection.typeCondition?.name.value;
if (selectionTypeName) {
const selectionType =
delegationContext.transformedSchema.getType(selectionTypeName);
if (selectionType && 'getFields' in selectionType) {
const selectionTypeFields = selectionType.getFields();
return {
...selection,
selectionSet: {
...selection.selectionSet,
selections: selection.selectionSet.selections.map(subSelection => {
if (subSelection.kind === Kind.FIELD) {
const fieldName = subSelection.name.value;
if (!subSelection.alias) {
const field = selectionTypeFields[fieldName];
if (field) {
let currentNullable: boolean;
if (isNullableType(field.type)) {
seenNullable.add(fieldName);
currentNullable = true;
} else {
seenNonNullable.add(fieldName);
currentNullable = false;
}
if (seenNullable.size && seenNonNullable.size) {
transformationContext[OverlappingAliases] = true;
return {
...subSelection,
alias: {
kind: Kind.NAME,
value: currentNullable
? `_nullable_${fieldName}`
: `_nonNullable_${fieldName}`,
},
};
}
}
}
}
return subSelection;
}),
},
};
}
}
}
return selection;
}),
};
},
});
return {
...request,
document: newDocument,
};
}

transformResult(
result: ExecutionResult,
_delegationContext: DelegationContext<TContext>,
transformationContext: OverlappingAliasesContext,
) {
if (transformationContext[OverlappingAliases]) {
return removeOverlappingAliases(result);
}
return result;
}
}

function removeOverlappingAliases(result: any): any {
if (result != null) {
if (Array.isArray(result)) {
return result.map(removeOverlappingAliases);
} else if (typeof result === 'object') {
const newResult: Record<string, any> = {};
for (const key in result) {
if (key.startsWith('_nullable_') || key.startsWith('_nonNullable_')) {
const newKey = key.replace(/^_nullable_/, '').replace(/^_nonNullable_/, '');
newResult[newKey] = removeOverlappingAliases(result[key]);
} else {
newResult[key] = removeOverlappingAliases(result[key]);
}
}
return newResult;
}
}
return result;
}
7 changes: 4 additions & 3 deletions packages/delegate/src/Transformer.ts
@@ -1,6 +1,7 @@
import { ExecutionRequest, ExecutionResult } from '@graphql-tools/utils';
import { checkResultAndHandleErrors } from './checkResultAndHandleErrors.js';
import { finalizeGatewayRequest } from './finalizeGatewayRequest.js';
import { OverlappingAliasesTransform } from './OverlappingAliasesTransform.js';
import { prepareGatewayDocument } from './prepareGatewayDocument.js';
import { DelegationContext, Transform } from './types.js';

Expand All @@ -18,8 +19,10 @@ export class Transformer<TContext extends Record<string, any> = Record<string, a
const transforms = context.transforms;
const delegationTransforms = transforms.slice().reverse();
for (const transform of delegationTransforms) {
this.addTransform(transform, {});
this.addTransform(transform);
}
// TODO: Move this to the core, later
this.addTransform(new OverlappingAliasesTransform());
}

private addTransform(transform: Transform<any, TContext>, context = {}) {
Expand Down Expand Up @@ -52,7 +55,6 @@ export class Transformer<TContext extends Record<string, any> = Record<string, a

public transformResult(originalResult: ExecutionResult) {
let result = originalResult;

// from right to left
for (let i = this.transformations.length - 1; i >= 0; i--) {
const transformation = this.transformations[i];
Expand All @@ -64,7 +66,6 @@ export class Transformer<TContext extends Record<string, any> = Record<string, a
);
}
}

return checkResultAndHandleErrors(result, this.delegationContext);
}
}
12 changes: 12 additions & 0 deletions packages/federation/src/supergraph.ts
Expand Up @@ -13,6 +13,7 @@ import {
ObjectTypeDefinitionNode,
ObjectTypeExtensionNode,
parse,
parseType,
print,
ScalarTypeDefinitionNode,
TypeDefinitionNode,
Expand Down Expand Up @@ -159,8 +160,16 @@ export function getSubschemasFromSupergraphSdl({
argumentNode.value.value === true,
);
if (!isExternal) {
const typeArg = joinFieldDirectiveNode.arguments?.find(
argumentNode => argumentNode.name.value === 'type',
);
const typeNode =
typeArg?.value.kind === Kind.STRING
? parseType(typeArg.value.value)
: fieldNode.type;
fieldDefinitionNodesOfSubgraph.push({
...fieldNode,
type: typeNode,
directives: fieldNode.directives?.filter(
directiveNode => directiveNode.name.value !== 'join__field',
),
Expand Down Expand Up @@ -708,6 +717,9 @@ export function getStitchedSchemaFromSupergraphSdl(opts: GetSubschemasFromSuperg
subschemas: [...subschemaMap.values()],
assumeValid: true,
assumeValidSDL: true,
typeMergingOptions: {
useNonNullableFieldOnConflict: true,
},
});
return filterInternalFieldsAndTypes(supergraphSchema);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/federation/test/federation-compatibility.test.ts
Expand Up @@ -43,7 +43,7 @@ describe('Federation Compatibility', () => {
} else {
if ('errors' in result && result.errors) {
for (const error of result.errors) {
console.error(error.message);
console.error(error.stack);
}
}
expect(result).toMatchObject({
Expand Down

0 comments on commit 9bca9e0

Please sign in to comment.