-
-
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
[deploy_website] enhance(stitch) canonical merged type/field definitions #2417
Changes from all commits
7c72fae
3a303e9
2749b0b
748f9e5
a1de939
5c42631
a195b8b
bdef2fb
f1b6d0f
f798573
58e476c
04c209a
dafdfa7
a0a626f
1a1b332
4738b19
1d0138e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
--- | ||
"@graphql-tools/delegate": patch | ||
"@graphql-tools/merge": patch | ||
"@graphql-tools/stitch": minor | ||
"@graphql-tools/stitching-directives": minor | ||
"@graphql-tools/website": patch | ||
--- | ||
|
||
enhance(stitch) canonical merged type and field definitions. Use the @canonical directive to promote preferred type and field descriptions into the combined gateway schema. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,10 @@ import { | |
isUnionType, | ||
isEnumType, | ||
isInputObjectType, | ||
GraphQLFieldConfig, | ||
GraphQLFieldConfigMap, | ||
GraphQLInputObjectType, | ||
GraphQLInputFieldConfig, | ||
GraphQLInputFieldConfigMap, | ||
ObjectTypeDefinitionNode, | ||
InputObjectTypeDefinitionNode, | ||
|
@@ -33,8 +35,10 @@ import { | |
TypeMergingOptions, | ||
MergeFieldConfigCandidate, | ||
MergeInputFieldConfigCandidate, | ||
MergeEnumValueConfigCandidate, | ||
} from './types'; | ||
import { fieldToFieldConfig, inputFieldToFieldConfig } from '@graphql-tools/utils'; | ||
import { isSubschemaConfig } from '@graphql-tools/delegate'; | ||
|
||
export function mergeCandidates( | ||
typeName: string, | ||
|
@@ -68,6 +72,8 @@ function mergeObjectTypeCandidates( | |
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): GraphQLObjectType<any, any> { | ||
candidates = orderedTypeCandidates(candidates, typeMergingOptions); | ||
|
||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
const fields = fieldConfigMapFromTypeCandidates(candidates, typeMergingOptions); | ||
const typeConfigs = candidates.map(candidate => (candidate.type as GraphQLObjectType).toConfig()); | ||
|
@@ -84,6 +90,17 @@ function mergeObjectTypeCandidates( | |
const interfaces = Object.keys(interfaceMap).map(interfaceName => interfaceMap[interfaceName]); | ||
|
||
const astNodes = pluck<ObjectTypeDefinitionNode>('astNode', candidates); | ||
const fieldAstNodes = canonicalFieldNamesForType(candidates) | ||
.map(fieldName => fields[fieldName]?.astNode) | ||
.filter(n => n != null); | ||
|
||
if (astNodes.length > 1 && fieldAstNodes.length) { | ||
astNodes.push({ | ||
...astNodes[astNodes.length - 1], | ||
fields: JSON.parse(JSON.stringify(fieldAstNodes)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to recreate? Are we modifying these? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the reason this is done is because logic now selects all the final field definition values that we want (directive arg values, field nullability, etc) up front. We still need to go through the merge process so that all fields have their characteristics combined, and that is an incrementally destructive process that modifies the original AST nodes. So, this clean copy is made before merging and gets added as the final merger... All fields will be combined as normal, and then that original selection of preferred values will be applied at the end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't follow this. So I just tried removing it, and sure enough, your (comprehensive!) test suite fails. It seems like the mergeType (et al) functions did not quite do what I expected, i.e. modify the passed in fields, not just in the target of the merge, but also the sources? And this is a work-around for that? @ardatan is this the work-around you would suggest? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is a work-around for the fact that merge methods modify the original AST objects. By creating an AST copy of those fields up front, we can reapply them at the end. I just added an update that makes this way more efficient... Now subschema config is checked for canonical field overrides (which should be fairly uncommon), and only those specific field overrides are reapplied via this copy patch. So, it's a lot more efficient now than always reapplying a copy of all fields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am ready to merge this, it looks great, my last concern is to wait for @ardatan if he has time to take a look, if these merge functions are "supposed" to work like this, and if there should be another fix. Or we could create a separate issue to track like for mergeEnums There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. going to merge for now, we might need to revisit if @ardatan has a chance to weigh in, but i think that will be just internal implementation details. long-term, we might want to actually figure out a way to remove the |
||
}); | ||
} | ||
|
||
const astNode = astNodes | ||
.slice(1) | ||
.reduce( | ||
|
@@ -113,10 +130,23 @@ function mergeInputObjectTypeCandidates( | |
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): GraphQLInputObjectType { | ||
candidates = orderedTypeCandidates(candidates, typeMergingOptions); | ||
|
||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
const fields = inputFieldConfigMapFromTypeCandidates(candidates, typeMergingOptions); | ||
|
||
const astNodes = pluck<InputObjectTypeDefinitionNode>('astNode', candidates); | ||
const fieldAstNodes = canonicalFieldNamesForType(candidates) | ||
.map(fieldName => fields[fieldName]?.astNode) | ||
.filter(n => n != null); | ||
|
||
if (astNodes.length > 1 && fieldAstNodes.length) { | ||
astNodes.push({ | ||
...astNodes[astNodes.length - 1], | ||
fields: JSON.parse(JSON.stringify(fieldAstNodes)), | ||
}); | ||
} | ||
|
||
const astNode = astNodes | ||
.slice(1) | ||
.reduce( | ||
|
@@ -149,6 +179,8 @@ function mergeInterfaceTypeCandidates( | |
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): GraphQLInterfaceType { | ||
candidates = orderedTypeCandidates(candidates, typeMergingOptions); | ||
|
||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
const fields = fieldConfigMapFromTypeCandidates(candidates, typeMergingOptions); | ||
const typeConfigs = candidates.map(candidate => (candidate.type as GraphQLInterfaceType).toConfig()); | ||
|
@@ -165,6 +197,17 @@ function mergeInterfaceTypeCandidates( | |
const interfaces = Object.keys(interfaceMap).map(interfaceName => interfaceMap[interfaceName]); | ||
|
||
const astNodes = pluck<InterfaceTypeDefinitionNode>('astNode', candidates); | ||
const fieldAstNodes = canonicalFieldNamesForType(candidates) | ||
.map(fieldName => fields[fieldName]?.astNode) | ||
.filter(n => n != null); | ||
|
||
if (astNodes.length > 1 && fieldAstNodes.length) { | ||
astNodes.push({ | ||
...astNodes[astNodes.length - 1], | ||
fields: JSON.parse(JSON.stringify(fieldAstNodes)), | ||
}); | ||
} | ||
|
||
const astNode = astNodes | ||
.slice(1) | ||
.reduce( | ||
|
@@ -194,6 +237,7 @@ function mergeUnionTypeCandidates( | |
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): GraphQLUnionType { | ||
candidates = orderedTypeCandidates(candidates, typeMergingOptions); | ||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
|
||
const typeConfigs = candidates.map(candidate => (candidate.type as GraphQLUnionType).toConfig()); | ||
|
@@ -234,16 +278,10 @@ function mergeEnumTypeCandidates( | |
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): GraphQLEnumType { | ||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
candidates = orderedTypeCandidates(candidates, typeMergingOptions); | ||
|
||
const typeConfigs = candidates.map(candidate => (candidate.type as GraphQLEnumType).toConfig()); | ||
const values = typeConfigs.reduce<GraphQLEnumValueConfigMap>( | ||
(acc, typeConfig) => ({ | ||
...acc, | ||
...typeConfig.values, | ||
}), | ||
{} | ||
); | ||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
const values = enumValueConfigMapFromTypeCandidates(candidates, typeMergingOptions); | ||
|
||
const astNodes = pluck<EnumTypeDefinitionNode>('astNode', candidates); | ||
const astNode = astNodes | ||
|
@@ -266,13 +304,56 @@ function mergeEnumTypeCandidates( | |
return new GraphQLEnumType(typeConfig); | ||
} | ||
|
||
function enumValueConfigMapFromTypeCandidates( | ||
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): GraphQLEnumValueConfigMap { | ||
const enumValueConfigCandidatesMap: Record<string, Array<MergeEnumValueConfigCandidate>> = Object.create(null); | ||
|
||
candidates.forEach(candidate => { | ||
const valueMap = (candidate.type as GraphQLEnumType).toConfig().values; | ||
Object.keys(valueMap).forEach(enumValue => { | ||
const enumValueConfigCandidate = { | ||
enumValueConfig: valueMap[enumValue], | ||
enumValue, | ||
type: candidate.type as GraphQLEnumType, | ||
subschema: candidate.subschema, | ||
transformedSubschema: candidate.transformedSubschema, | ||
}; | ||
|
||
if (enumValue in enumValueConfigCandidatesMap) { | ||
enumValueConfigCandidatesMap[enumValue].push(enumValueConfigCandidate); | ||
} else { | ||
enumValueConfigCandidatesMap[enumValue] = [enumValueConfigCandidate]; | ||
} | ||
}); | ||
}); | ||
|
||
const enumValueConfigMap = Object.create(null); | ||
|
||
Object.keys(enumValueConfigCandidatesMap).forEach(enumValue => { | ||
const enumValueConfigMerger = typeMergingOptions?.enumValueConfigMerger ?? defaultEnumValueConfigMerger; | ||
enumValueConfigMap[enumValue] = enumValueConfigMerger(enumValueConfigCandidatesMap[enumValue]); | ||
}); | ||
|
||
return enumValueConfigMap; | ||
} | ||
|
||
function defaultEnumValueConfigMerger(candidates: Array<MergeEnumValueConfigCandidate>) { | ||
const preferred = candidates.find( | ||
({ type, subschema }) => isSubschemaConfig(subschema) && subschema.merge?.[type.name]?.canonical | ||
); | ||
return (preferred || candidates[candidates.length - 1]).enumValueConfig; | ||
} | ||
|
||
function mergeScalarTypeCandidates( | ||
typeName: string, | ||
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): GraphQLScalarType { | ||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
candidates = orderedTypeCandidates(candidates, typeMergingOptions); | ||
|
||
const description = mergeTypeDescriptions(candidates, typeMergingOptions); | ||
const serializeFns = pluck<GraphQLScalarSerializer<any>>('serialize', candidates); | ||
const serialize = serializeFns[serializeFns.length - 1]; | ||
|
||
|
@@ -285,7 +366,10 @@ function mergeScalarTypeCandidates( | |
const astNodes = pluck<ScalarTypeDefinitionNode>('astNode', candidates); | ||
const astNode = astNodes | ||
.slice(1) | ||
.reduce((acc, astNode) => mergeScalar(acc, astNode), astNodes[0]) as ScalarTypeDefinitionNode; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update to match the other mergers. This one was setup slightly differently and was producing different AST results. |
||
.reduce( | ||
(acc, astNode) => mergeScalar(astNode, acc as ScalarTypeDefinitionNode) as ScalarTypeDefinitionNode, | ||
astNodes[0] | ||
); | ||
|
||
const extensionASTNodes = [].concat(pluck<Record<string, any>>('extensionASTNodes', candidates)); | ||
|
||
|
@@ -305,6 +389,29 @@ function mergeScalarTypeCandidates( | |
return new GraphQLScalarType(typeConfig); | ||
} | ||
|
||
function orderedTypeCandidates( | ||
candidates: Array<MergeTypeCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
): Array<MergeTypeCandidate> { | ||
const typeCandidateMerger = typeMergingOptions?.typeCandidateMerger ?? defaultTypeCandidateMerger; | ||
const candidate = typeCandidateMerger(candidates); | ||
return candidates.filter(c => c !== candidate).concat([candidate]); | ||
} | ||
|
||
function defaultTypeCandidateMerger(candidates: Array<MergeTypeCandidate>): MergeTypeCandidate { | ||
const canonical: Array<MergeTypeCandidate> = candidates.filter(({ type, subschema }) => | ||
isSubschemaConfig(subschema) ? subschema.merge?.[type.name]?.canonical : false | ||
); | ||
|
||
if (canonical.length > 1) { | ||
throw new Error(`Multiple canonical definitions for "${canonical[0].type.name}"`); | ||
} else if (canonical.length) { | ||
return canonical[0]; | ||
} | ||
|
||
return candidates[candidates.length - 1]; | ||
} | ||
|
||
function mergeTypeDescriptions(candidates: Array<MergeTypeCandidate>, typeMergingOptions: TypeMergingOptions): string { | ||
const typeDescriptionsMerger = typeMergingOptions?.typeDescriptionsMerger ?? defaultTypeDescriptionMerger; | ||
return typeDescriptionsMerger(candidates); | ||
|
@@ -354,6 +461,26 @@ function mergeFieldConfigs(candidates: Array<MergeFieldConfigCandidate>, typeMer | |
} | ||
|
||
function defaultFieldConfigMerger(candidates: Array<MergeFieldConfigCandidate>) { | ||
const canonicalByField: Array<GraphQLFieldConfig<any, any>> = []; | ||
const canonicalByType: Array<GraphQLFieldConfig<any, any>> = []; | ||
|
||
candidates.forEach(({ type, fieldName, fieldConfig, subschema }) => { | ||
if (!isSubschemaConfig(subschema)) return; | ||
if (subschema.merge?.[type.name]?.fields?.[fieldName]?.canonical) { | ||
canonicalByField.push(fieldConfig); | ||
} else if (subschema.merge?.[type.name]?.canonical) { | ||
canonicalByType.push(fieldConfig); | ||
} | ||
}); | ||
|
||
if (canonicalByField.length > 1) { | ||
throw new Error(`Multiple canonical definitions for "${candidates[0].type.name}.${candidates[0].fieldName}"`); | ||
} else if (canonicalByField.length) { | ||
return canonicalByField[0]; | ||
} else if (canonicalByType.length) { | ||
return canonicalByType[0]; | ||
} | ||
|
||
return candidates[candidates.length - 1].fieldConfig; | ||
} | ||
|
||
|
@@ -385,23 +512,49 @@ function inputFieldConfigMapFromTypeCandidates( | |
const inputFieldConfigMap = Object.create(null); | ||
|
||
Object.keys(inputFieldConfigCandidatesMap).forEach(fieldName => { | ||
inputFieldConfigMap[fieldName] = mergeInputFieldConfigs( | ||
inputFieldConfigCandidatesMap[fieldName], | ||
typeMergingOptions | ||
); | ||
const inputFieldConfigMerger = typeMergingOptions?.inputFieldConfigMerger ?? defaultInputFieldConfigMerger; | ||
inputFieldConfigMap[fieldName] = inputFieldConfigMerger(inputFieldConfigCandidatesMap[fieldName]); | ||
}); | ||
|
||
return inputFieldConfigMap; | ||
} | ||
|
||
function mergeInputFieldConfigs( | ||
candidates: Array<MergeInputFieldConfigCandidate>, | ||
typeMergingOptions: TypeMergingOptions | ||
) { | ||
const inputFieldConfigMerger = typeMergingOptions?.inputFieldConfigMerger ?? defaultInputFieldConfigMerger; | ||
return inputFieldConfigMerger(candidates); | ||
} | ||
|
||
function defaultInputFieldConfigMerger(candidates: Array<MergeInputFieldConfigCandidate>) { | ||
const canonicalByField: Array<GraphQLInputFieldConfig> = []; | ||
const canonicalByType: Array<GraphQLInputFieldConfig> = []; | ||
|
||
candidates.forEach(({ type, fieldName, inputFieldConfig, subschema }) => { | ||
if (!isSubschemaConfig(subschema)) return; | ||
if (subschema.merge?.[type.name]?.fields?.[fieldName]?.canonical) { | ||
canonicalByField.push(inputFieldConfig); | ||
} else if (subschema.merge?.[type.name]?.canonical) { | ||
canonicalByType.push(inputFieldConfig); | ||
} | ||
}); | ||
|
||
if (canonicalByField.length > 1) { | ||
throw new Error(`Multiple canonical definitions for "${candidates[0].type.name}.${candidates[0].fieldName}"`); | ||
} else if (canonicalByField.length) { | ||
return canonicalByField[0]; | ||
} else if (canonicalByType.length) { | ||
return canonicalByType[0]; | ||
} | ||
|
||
return candidates[candidates.length - 1].inputFieldConfig; | ||
} | ||
|
||
function canonicalFieldNamesForType(candidates: Array<MergeTypeCandidate>): Array<string> { | ||
const canonicalFieldNames: Record<string, boolean> = Object.create(null); | ||
|
||
candidates.forEach(({ type, subschema }) => { | ||
if (isSubschemaConfig(subschema) && subschema.merge?.[type.name]?.fields && !subschema.merge[type.name].canonical) { | ||
Object.entries(subschema.merge[type.name].fields).forEach(([fieldName, mergedFieldConfig]) => { | ||
if (mergedFieldConfig.canonical) { | ||
canonicalFieldNames[fieldName] = true; | ||
} | ||
}); | ||
} | ||
}); | ||
|
||
return Object.keys(canonicalFieldNames); | ||
} |
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.
It appears that the enum type and value mergers are implemented with reversed argument order from each other. While the enum type merger assigns
A -> B
, the enum value merger assignsB -> A
. That means you'll end up with type settings from one candidate and value settings from the other... :sigh:This seems like the least invasive correction to make these align.
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.
Makes sense to me for now, might make sense to open up separate issue on that for consistency, defer to @ardatan
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.
#2464