diff --git a/.changeset/proud-days-press.md b/.changeset/proud-days-press.md new file mode 100644 index 000000000..8330cbf9a --- /dev/null +++ b/.changeset/proud-days-press.md @@ -0,0 +1,7 @@ +--- +"@apollo/federation-internals": patch +"@apollo/gateway": patch +"@apollo/composition": patch +--- + +Fix bugs in composition when merging nulls in directive applications and when handling renames. diff --git a/composition-js/src/__tests__/compose.directiveArgumentMergeStrategies.test.ts b/composition-js/src/__tests__/compose.directiveArgumentMergeStrategies.test.ts index b1be5c54c..c83466e93 100644 --- a/composition-js/src/__tests__/compose.directiveArgumentMergeStrategies.test.ts +++ b/composition-js/src/__tests__/compose.directiveArgumentMergeStrategies.test.ts @@ -89,17 +89,19 @@ describe('composition of directive with non-trivial argument strategies', () => t: 2, k: 1, b: 4, }, }, { - name: 'sum', - type: (schema: Schema) => new NonNullType(schema.intType()), - compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.SUM, - argValues: { - s1: { t: 3, k: 1 }, - s2: { t: 2, k: 5, b: 4 }, - }, - resultValues: { - t: 5, k: 6, b: 4, - }, - }, { + // NOTE: See the note for the SUM strategy in argumentCompositionStrategies.ts + // for more information on why this is commented out. + // name: 'sum', + // type: (schema: Schema) => new NonNullType(schema.intType()), + // compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.SUM, + // argValues: { + // s1: { t: 3, k: 1 }, + // s2: { t: 2, k: 5, b: 4 }, + // }, + // resultValues: { + // t: 5, k: 6, b: 4, + // }, + // }, { name: 'intersection', type: (schema: Schema) => new NonNullType(new ListType(new NonNullType(schema.stringType()))), compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.INTERSECTION, @@ -219,7 +221,7 @@ describe('composition of directive with non-trivial argument strategies', () => const s = result.schema; expect(directiveStrings(s.schemaDefinition, name)).toStrictEqual([ - `@link(url: "https://specs.apollo.dev/${name}/v0.1", import: ["@${name}"])` + `@link(url: "https://specs.apollo.dev/${name}/v0.1")` ]); const t = s.type('T') as ObjectType; diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 294e54cdc..01b3430dd 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -31,7 +31,6 @@ import { CompositeType, Subgraphs, JOIN_VERSIONS, - INACCESSIBLE_VERSIONS, NamedSchemaElement, errorCauses, isObjectType, @@ -69,7 +68,6 @@ import { CoreSpecDefinition, FeatureVersion, FEDERATION_VERSIONS, - InaccessibleSpecDefinition, LinkDirectiveArgs, sourceIdentity, FeatureUrl, @@ -81,6 +79,10 @@ import { isNullableType, isFieldDefinition, Post20FederationDirectiveDefinition, + DirectiveCompositionSpecification, + FeatureDefinition, + CoreImport, + inaccessibleIdentity, } from "@apollo/federation-internals"; import { ASTNode, GraphQLError, DirectiveLocation } from "graphql"; import { @@ -345,6 +347,12 @@ interface OverrideArgs { label?: string; } +interface MergedDirectiveInfo { + definition: DirectiveDefinition; + argumentsMerger?: ArgumentMerger; + staticArgumentTransform?: StaticArgumentsTransform; +} + class Merger { readonly names: readonly string[]; readonly subgraphsSchema: readonly Schema[]; @@ -353,7 +361,8 @@ class Merger { readonly merged: Schema = new Schema(); readonly subgraphNamesToJoinSpecName: Map; readonly mergedFederationDirectiveNames = new Set(); - readonly mergedFederationDirectiveInSupergraph = new Map(); + readonly mergedFederationDirectiveInSupergraphByDirectiveName = + new Map(); readonly enumUsages = new Map(); private composeDirectiveManager: ComposeDirectiveManager; private mismatchReporter: MismatchReporter; @@ -364,7 +373,7 @@ class Merger { }[]; private joinSpec: JoinSpecDefinition; private linkSpec: CoreSpecDefinition; - private inaccessibleSpec: InaccessibleSpecDefinition; + private inaccessibleDirectiveInSupergraph?: DirectiveDefinition; private latestFedVersionUsed: FeatureVersion; private joinDirectiveIdentityURLs = new Set(); private schemaToImportNameToFeatureUrl = new Map>(); @@ -375,7 +384,6 @@ class Merger { this.latestFedVersionUsed = this.getLatestFederationVersionUsed(); this.joinSpec = JOIN_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed); this.linkSpec = LINK_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed); - this.inaccessibleSpec = INACCESSIBLE_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed); this.fieldsWithFromContext = this.getFieldsWithFromContextDirective(); this.fieldsWithOverride = this.getFieldsWithOverrideDirective(); @@ -470,59 +478,127 @@ class Merger { assert(errors.length === 0, "We shouldn't have errors adding the join spec to the (still empty) supergraph schema"); const directivesMergeInfo = collectCoreDirectivesToCompose(this.subgraphs); - for (const mergeInfo of directivesMergeInfo) { - this.validateAndMaybeAddSpec(mergeInfo); - } - + this.validateAndMaybeAddSpecs(directivesMergeInfo); return this.joinSpec.populateGraphEnum(this.merged, this.subgraphs); } - private validateAndMaybeAddSpec({url, name, definitionsPerSubgraph, compositionSpec}: CoreDirectiveInSubgraphs) { - // Not composition specification means that it shouldn't be composed. - if (!compositionSpec) { - return; - } - - let nameInSupergraph: string | undefined; - for (const subgraph of this.subgraphs) { - const directive = definitionsPerSubgraph.get(subgraph.name); - if (!directive) { - continue; + private validateAndMaybeAddSpecs(directivesMergeInfo: CoreDirectiveInSubgraphs[]) { + const supergraphInfoByIdentity = new Map< + string, + { + specInSupergraph: FeatureDefinition; + directives: { + nameInFeature: string; + nameInSupergraph: string; + compositionSpec: DirectiveCompositionSpecification; + }[]; } + >; - if (!nameInSupergraph) { - nameInSupergraph = directive.name; - } else if (nameInSupergraph !== directive.name) { - this.mismatchReporter.reportMismatchError( - ERRORS.LINK_IMPORT_NAME_MISMATCH, - `The "@${name}" directive (from ${url}) is imported with mismatched name between subgraphs: it is imported as `, - directive, - sourcesFromArray(this.subgraphs.values().map((s) => definitionsPerSubgraph.get(s.name))), - (def) => `"@${def.name}"`, - ); + for (const {url, name, definitionsPerSubgraph, compositionSpec} of directivesMergeInfo) { + // No composition specification means that it shouldn't be composed. + if (!compositionSpec) { return; } + + let nameInSupergraph: string | undefined; + for (const subgraph of this.subgraphs) { + const directive = definitionsPerSubgraph.get(subgraph.name); + if (!directive) { + continue; + } + + if (!nameInSupergraph) { + nameInSupergraph = directive.name; + } else if (nameInSupergraph !== directive.name) { + this.mismatchReporter.reportMismatchError( + ERRORS.LINK_IMPORT_NAME_MISMATCH, + `The "@${name}" directive (from ${url}) is imported with mismatched name between subgraphs: it is imported as `, + directive, + sourcesFromArray(this.subgraphs.values().map((s) => definitionsPerSubgraph.get(s.name))), + (def) => `"@${def.name}"`, + ); + return; + } + } + + // If we get here with `nameInSupergraph` unset, it means there is no usage for the directive at all and we + // don't bother adding the spec to the supergraph. + if (nameInSupergraph) { + const specInSupergraph = compositionSpec.supergraphSpecification(this.latestFedVersionUsed); + let supergraphInfo = supergraphInfoByIdentity.get(specInSupergraph.url.identity); + if (supergraphInfo) { + assert( + specInSupergraph.url.equals(supergraphInfo.specInSupergraph.url), + `Spec ${specInSupergraph.url} directives disagree on version for supergraph`, + ); + } else { + supergraphInfo = { + specInSupergraph, + directives: [], + }; + supergraphInfoByIdentity.set(specInSupergraph.url.identity, supergraphInfo); + } + supergraphInfo.directives.push({ + nameInFeature: name, + nameInSupergraph, + compositionSpec, + }); + } } - // If we get here with `nameInSupergraph` unset, it means there is no usage for the directive at all and we - // don't bother adding the spec to the supergraph. - if (nameInSupergraph) { - const specInSupergraph = compositionSpec.supergraphSpecification(this.latestFedVersionUsed); - const errors = this.linkSpec.applyFeatureAsLink(this.merged, specInSupergraph, specInSupergraph.defaultCorePurpose, [{ name, as: name === nameInSupergraph ? undefined : nameInSupergraph }], ); - assert(errors.length === 0, "We shouldn't have errors adding the join spec to the (still empty) supergraph schema"); - const feature = this.merged?.coreFeatures?.getByIdentity(specInSupergraph.url.identity); + for (const { specInSupergraph, directives } of supergraphInfoByIdentity.values()) { + const imports: CoreImport[] = []; + for (const { nameInFeature, nameInSupergraph } of directives) { + const defaultNameInSupergraph = CoreFeature.directiveNameInSchemaForCoreArguments( + specInSupergraph.url, + specInSupergraph.url.name, + [], + nameInFeature, + ); + if (nameInSupergraph !== defaultNameInSupergraph) { + imports.push(nameInFeature === nameInSupergraph + ? { name: `@${nameInFeature}` } + : { name: `@${nameInFeature}`, as: `@${nameInSupergraph}` } + ); + } + } + const errors = this.linkSpec.applyFeatureToSchema( + this.merged, + specInSupergraph, + undefined, + specInSupergraph.defaultCorePurpose, + imports, + ); + assert( + errors.length === 0, + "We shouldn't have errors adding the join spec to the (still empty) supergraph schema" + ); + const feature = this.merged.coreFeatures?.getByIdentity(specInSupergraph.url.identity); assert(feature, 'Should have found the feature we just added'); - const argumentsMerger = compositionSpec.argumentsMerger?.call(null, this.merged, feature); - if (argumentsMerger instanceof GraphQLError) { - // That would mean we made a mistake in the declaration of a hard-coded directive, so we just throw right away so this can be caught and corrected. - throw argumentsMerger; - } - this.mergedFederationDirectiveNames.add(nameInSupergraph); - this.mergedFederationDirectiveInSupergraph.set(name, { - definition: this.merged.directive(nameInSupergraph)!, - argumentsMerger, - staticArgumentTransform: compositionSpec.staticArgumentTransform, - }); + for (const { nameInFeature, nameInSupergraph, compositionSpec } of directives) { + const argumentsMerger = compositionSpec.argumentsMerger?.call(null, this.merged, feature); + if (argumentsMerger instanceof GraphQLError) { + // That would mean we made a mistake in the declaration of a hard-coded directive, + // so we just throw right away so this can be caught and corrected. + throw argumentsMerger; + } + this.mergedFederationDirectiveNames.add(nameInSupergraph); + this.mergedFederationDirectiveInSupergraphByDirectiveName.set(nameInSupergraph, { + definition: this.merged.directive(nameInSupergraph)!, + argumentsMerger, + staticArgumentTransform: compositionSpec.staticArgumentTransform, + }); + // If we encounter the @inaccessible directive, we need to record its + // definition so certain merge validations that care about @inaccessible + // can act accordingly. + if ( + specInSupergraph.identity === inaccessibleIdentity + && nameInFeature === specInSupergraph.url.name + ) { + this.inaccessibleDirectiveInSupergraph = this.merged.directive(nameInSupergraph)!; + } + } } } @@ -2464,8 +2540,8 @@ class Merger { this.recordAppliedDirectivesToMerge(valueSources, value); this.addJoinEnumValue(valueSources, value); - const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name); - const isInaccessible = inaccessibleInSupergraph && value.hasAppliedDirective(inaccessibleInSupergraph.definition); + const isInaccessible = this.inaccessibleDirectiveInSupergraph + && value.hasAppliedDirective(this.inaccessibleDirectiveInSupergraph); // The merging strategy depends on the enum type usage: // - if it is _only_ used in position of Input type, we merge it with an "intersection" strategy (like other input types/things). // - if it is _only_ used in position of Output type, we merge it with an "union" strategy (like other output types/things). @@ -2562,8 +2638,6 @@ class Merger { } private mergeInput(inputSources: Sources, dest: InputObjectType) { - const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name); - // Like for other inputs, we add all the fields found in any subgraphs initially as a simple mean to have a complete list of // field to iterate over, but we will remove those that are not in all subgraphs. const added = this.addFieldsShallow(inputSources, dest); @@ -2572,7 +2646,8 @@ class Merger { // compatibility between definitions and 2) we actually want to see if the result is marked inaccessible or not and it makes // that easier. this.mergeInputField(subgraphFields, destField); - const isInaccessible = inaccessibleInSupergraph && destField.hasAppliedDirective(inaccessibleInSupergraph.definition); + const isInaccessible = this.inaccessibleDirectiveInSupergraph + && destField.hasAppliedDirective(this.inaccessibleDirectiveInSupergraph); // Note that if the field is manually marked @inaccessible, we can always accept it to be inconsistent between subgraphs since // it won't be exposed in the API, and we don't hint about it because we're just doing what the user is explicitely asking. if (!isInaccessible && someSources(subgraphFields, field => !field)) { @@ -2840,8 +2915,7 @@ class Merger { // is @inaccessible, which is necessary to exist in the supergraph for EnumValues to properly // determine whether the fact that a value is both input / output will matter private recordAppliedDirectivesToMerge(sources: Sources>, dest: SchemaElement) { - const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name); - const inaccessibleName = inaccessibleInSupergraph?.definition.name; + const inaccessibleName = this.inaccessibleDirectiveInSupergraph?.name; const names = this.gatherAppliedDirectiveNames(sources); if (inaccessibleName && names.has(inaccessibleName)) { @@ -2905,7 +2979,7 @@ class Merger { return; } - const directiveInSupergraph = this.mergedFederationDirectiveInSupergraph.get(name); + const directiveInSupergraph = this.mergedFederationDirectiveInSupergraphByDirectiveName.get(name); if (dest.schema().directive(name)?.repeatable) { // For repeatable directives, we simply include each application found but with exact duplicates removed @@ -2945,7 +3019,7 @@ class Merger { if (differentApplications.length === 1) { dest.applyDirective(name, differentApplications[0].arguments(false)); } else { - const info = this.mergedFederationDirectiveInSupergraph.get(name); + const info = this.mergedFederationDirectiveInSupergraphByDirectiveName.get(name); if (info && info.argumentsMerger) { const mergedArguments = Object.create(null); const applicationsArguments = differentApplications.map((a) => a.arguments(true)); diff --git a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts index 458175876..43b65e309 100644 --- a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts +++ b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts @@ -149,7 +149,7 @@ describe('lifecycle hooks', () => { // the supergraph (even just formatting differences), this ID will change // and this test will have to updated. expect(secondCall[0]!.compositionId).toMatchInlineSnapshot( - `"4aa2278e35df345ff5959a30546d2e9ef9e997204b4ffee4a42344b578b36068"`, + `"6dc1bde2b9818fabec62208c5d8825abaa1bae89635fa6f3a5ffea7b78fc6d82"`, ); // second call should have previous info in the second arg expect(secondCall[1]!.compositionId).toEqual(expectedFirstId); diff --git a/internals-js/src/__tests__/values.test.ts b/internals-js/src/__tests__/values.test.ts index eaab39aa7..6e1d167fb 100644 --- a/internals-js/src/__tests__/values.test.ts +++ b/internals-js/src/__tests__/values.test.ts @@ -414,5 +414,6 @@ describe('objectEquals tests', () => { expect(valueEquals({ foo: 'foo', bar: undefined }, { foo: 'foo' })).toBe( false, ); + expect(valueEquals({}, null)).toBe(false); }); }); diff --git a/internals-js/src/argumentCompositionStrategies.ts b/internals-js/src/argumentCompositionStrategies.ts index 85a870773..f9d60ef3f 100644 --- a/internals-js/src/argumentCompositionStrategies.ts +++ b/internals-js/src/argumentCompositionStrategies.ts @@ -13,87 +13,105 @@ export type ArgumentCompositionStrategy = { function supportFixedTypes(types: (schema: Schema) => InputType[]): TypeSupportValidator { return (schema, type) => { const supported = types(schema); - if (!supported.some((t) => sameType(t, type))) { - return { valid: false, supportedMsg: `type(s) ${supported.join(', ')}` }; - } - return { valid: true }; + return supported.some((t) => sameType(t, type)) + ? { valid: true } + : { valid: false, supportedMsg: `type(s) ${supported.join(', ')}` }; }; } function supportAnyNonNullArray(): TypeSupportValidator { - return (_, type) => { - if (!isNonNullType(type) || !isListType(type.ofType)) { - return { valid: false, supportedMsg: 'non nullable list types of any type'}; - } - return { valid: true }; + return (_, type) => isNonNullType(type) && isListType(type.ofType) + ? { valid: true } + : { valid: false, supportedMsg: 'non nullable list types of any type' } +} + +function supportAnyArray(): TypeSupportValidator { + return (_, type) => isListType(type) || (isNonNullType(type) && isListType(type.ofType)) + ? { valid: true } + : { valid: false, supportedMsg: 'list types of any type' }; +} + +// NOTE: This function makes the assumption that for the directive argument +// being merged, it is not "nullable with non-null default" in the supergraph +// schema (this kind of type/default combo is confusing and should be avoided, +// if possible). This assumption allows this function to replace null with +// undefined, which makes for a cleaner supergraph schema. +function mergeNullableValues( + mergeValues: (values: T[]) => T +): (values: (T | null | undefined)[]) => T | undefined { + return (values: (T | null | undefined)[]) => { + const nonNullValues = values.filter((v) => v !== null && v !== undefined) as T[]; + return nonNullValues.length > 0 + ? mergeValues(nonNullValues) + : undefined; }; } +function unionValues(values: any[]): any { + return values.reduce((acc, next) => { + const newValues = next.filter((v1: any) => !acc.some((v2: any) => valueEquals(v1, v2))); + return acc.concat(newValues); + }, []); +} + export const ARGUMENT_COMPOSITION_STRATEGIES = { MAX: { name: 'MAX', isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]), - mergeValues: (values: any[]) => Math.max(...values), + mergeValues: (values: number[]) => Math.max(...values), }, MIN: { name: 'MIN', isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]), - mergeValues: (values: any[]) => Math.min(...values), - }, - SUM: { - name: 'SUM', - isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]), - mergeValues: (values: any[]) => values.reduce((acc, val) => acc + val, 0), + mergeValues: (values: number[]) => Math.min(...values), }, + // NOTE: This doesn't work today because directive applications are de-duped + // before being merged, we'd need to modify merge logic if we need this kind + // of behavior. + // SUM: { + // name: 'SUM', + // isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]), + // mergeValues: (values: any[]) => values.reduce((acc, val) => acc + val, 0), + // }, INTERSECTION: { name: 'INTERSECTION', isTypeSupported: supportAnyNonNullArray(), - mergeValues: (values: any[]) => values.reduce((acc, val) => acc.filter((v1: any) => val.some((v2: any) => valueEquals(v1, v2))), values[0]), + mergeValues: (values: any[]) => values.reduce((acc, next) => { + if (acc === undefined) { + return next; + } else { + return acc.filter((v1: any) => next.some((v2: any) => valueEquals(v1, v2))); + } + }, undefined) ?? [], }, UNION: { name: 'UNION', isTypeSupported: supportAnyNonNullArray(), - mergeValues: (values: any[]) => - values.reduce((acc, val) => { - const newValues = val.filter((v1: any) => !acc.some((v2: any) => valueEquals(v1, v2))); - return acc.concat(newValues); - }, []), + mergeValues: unionValues, }, NULLABLE_AND: { name: 'NULLABLE_AND', - isTypeSupported: supportFixedTypes((schema: Schema) => [schema.booleanType()]), - mergeValues: (values: (boolean | null | undefined)[]) => values.reduce((acc, next) => { - if (acc === null || acc === undefined) { - return next; - } else if (next === null || next === undefined) { - return acc; - } else { - return acc && next; - } - }, undefined), + isTypeSupported: supportFixedTypes((schema: Schema) => [ + schema.booleanType(), + new NonNullType(schema.booleanType()) + ]), + mergeValues: mergeNullableValues( + (values: boolean[]) => values.every((v) => v) + ), }, NULLABLE_MAX: { name: 'NULLABLE_MAX', - isTypeSupported: supportFixedTypes((schema: Schema) => [schema.intType(), new NonNullType(schema.intType())]), - mergeValues: (values: any[]) => values.reduce((a: any, b: any) => a !== undefined && b !== undefined ? Math.max(a, b) : a ?? b, undefined), + isTypeSupported: supportFixedTypes((schema: Schema) => [ + schema.intType(), + new NonNullType(schema.intType()) + ]), + mergeValues: mergeNullableValues( + (values: number[]) => Math.max(...values) + ) }, NULLABLE_UNION: { name: 'NULLABLE_UNION', - isTypeSupported: (_: Schema, type: InputType) => ({ valid: isListType(type) }), - mergeValues: (values: any[]) => { - if (values.every((v) => v === undefined)) { - return undefined; - } - - const combined = new Set(); - for (const subgraphValues of values) { - if (Array.isArray(subgraphValues)) { - for (const value of subgraphValues) { - combined.add(value); - } - } - } - return Array.from(combined); - } + isTypeSupported: supportAnyArray(), + mergeValues: mergeNullableValues(unionValues), } } diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index e52013047..cdd8e09dd 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -984,12 +984,28 @@ export class CoreFeature { } directiveNameInSchema(name: string): string { - const elementImport = this.imports.find((i) => i.name.charAt(0) === '@' && i.name.slice(1) === name); + return CoreFeature.directiveNameInSchemaForCoreArguments( + this.url, + this.nameInSchema, + this.imports, + name, + ); + } + + static directiveNameInSchemaForCoreArguments( + specUrl: FeatureUrl, + specNameInSchema: string, + imports: CoreImport[], + directiveNameInSpec: string, + ): string { + const elementImport = imports.find((i) => + i.name.charAt(0) === '@' && i.name.slice(1) === directiveNameInSpec + ); return elementImport - ? (elementImport.as?.slice(1) ?? name) - : (name === this.url.name - ? this.nameInSchema - : this.nameInSchema + '__' + name + ? (elementImport.as?.slice(1) ?? directiveNameInSpec) + : (directiveNameInSpec === specUrl.name + ? specNameInSchema + : specNameInSchema + '__' + directiveNameInSpec ); } @@ -1064,7 +1080,7 @@ export class CoreFeatures { const feature = this.byAlias.get(splitted[0]); return feature ? { feature, - nameInFeature: splitted[1], + nameInFeature: splitted.slice(1).join('__'), isImported: false, } : undefined; } else { @@ -1076,7 +1092,7 @@ export class CoreFeatures { if ((as ?? name) === importName) { return { feature, - nameInFeature: name.slice(1), + nameInFeature: isDirective ? name.slice(1) : name, isImported: true, }; } @@ -1088,8 +1104,8 @@ export class CoreFeatures { if (directFeature && isDirective) { return { feature: directFeature, - nameInFeature: directFeature.imports.find(imp => imp.as === `@${element.name}`)?.name.slice(1) ?? element.name, - isImported: true, + nameInFeature: element.name, + isImported: false, }; } diff --git a/internals-js/src/specs/coreSpec.ts b/internals-js/src/specs/coreSpec.ts index 909769ef4..aaeb155cb 100644 --- a/internals-js/src/specs/coreSpec.ts +++ b/internals-js/src/specs/coreSpec.ts @@ -195,7 +195,8 @@ export type CoreDirectiveArgs = { url: undefined, feature: string, as?: string, - for?: string + for?: string, + import: undefined, } export type LinkDirectiveArgs = { @@ -203,7 +204,7 @@ export type LinkDirectiveArgs = { feature: undefined, as?: string, for?: string, - import?: (string | CoreImport)[] + import?: (string | CoreImport)[], } export type CoreOrLinkDirectiveArgs = CoreDirectiveArgs | LinkDirectiveArgs; @@ -539,36 +540,36 @@ export class CoreSpecDefinition extends FeatureDefinition { return feature.url.version; } - applyFeatureToSchema(schema: Schema, feature: FeatureDefinition, as?: string, purpose?: CorePurpose): GraphQLError[] { + applyFeatureToSchema( + schema: Schema, + feature: FeatureDefinition, + as?: string, + purpose?: CorePurpose, + imports?: CoreImport[], + ): GraphQLError[] { const coreDirective = this.coreDirective(schema); const args = { [this.urlArgName()]: feature.toString(), as, - } as CoreDirectiveArgs; - if (this.supportPurposes() && purpose) { - args.for = purpose; - } - schema.schemaDefinition.applyDirective(coreDirective, args); - return feature.addElementsToSchema(schema); - } - - applyFeatureAsLink(schema: Schema, feature: FeatureDefinition, purpose?: CorePurpose, imports?: CoreImport[]): GraphQLError[] { - const existing = schema.schemaDefinition.appliedDirectivesOf(linkDirectiveDefaultName).find((link) => link.arguments().url === feature.toString()); - if (existing) { - existing.remove(); + } as CoreOrLinkDirectiveArgs; + if (purpose) { + if (this.supportPurposes()) { + args.for = purpose; + } else { + return [new GraphQLError( + `Cannot apply feature ${feature} with purpose since the schema's @core/@link version does not support it.` + )]; + } } - - const coreDirective = this.coreDirective(schema); - const args: LinkDirectiveArgs = { - url: feature.toString(), - import: (existing?.arguments().import ?? []).concat(imports?.map((i) => i.as ? { name: `@${i.name}`, as: `@${i.as}` } : `@${i.name}`)), - feature: undefined, - }; - - if (this.supportPurposes() && purpose) { - args.for = purpose; + if (imports && imports.length > 0) { + if (this.supportImport()) { + args.import = imports.map(i => i.as ? i : i.name); + } else { + return [new GraphQLError( + `Cannot apply feature ${feature} with imports since the schema's @core/@link version does not support it.` + )]; + } } - schema.schemaDefinition.applyDirective(coreDirective, args); return feature.addElementsToSchema(schema); } diff --git a/internals-js/src/values.ts b/internals-js/src/values.ts index 11b2968e5..62de2a08e 100644 --- a/internals-js/src/values.ts +++ b/internals-js/src/values.ts @@ -135,8 +135,10 @@ export function valueEquals(a: any, b: any): boolean { if (Array.isArray(a)) { return Array.isArray(b) && arrayValueEquals(a, b) ; } - if (typeof a === 'object') { - return typeof b === 'object' && objectEquals(a, b); + // Note that typeof null === 'object', so we have to manually rule that out + // here. + if (a !== null && typeof a === 'object') { + return b !== null && typeof b === 'object' && objectEquals(a, b); } return a === b; } @@ -224,8 +226,10 @@ function applyDefaultValues(value: any, type: InputType): any { if (fieldValue === undefined) { if (field.defaultValue !== undefined) { updated[field.name] = applyDefaultValues(field.defaultValue, field.type); - } else if (isNonNullType(field.type)) { - throw ERRORS.INVALID_GRAPHQL.err(`Field "${field.name}" of required type ${type} was not provided.`); + } else if (!isNonNullType(field.type)) { + updated[field.name] = null; + } else { + throw ERRORS.INVALID_GRAPHQL.err(`Required field "${field.name}" of type ${type} was not provided.`); } } else { updated[field.name] = applyDefaultValues(fieldValue, field.type); @@ -249,8 +253,12 @@ export function withDefaultValues(value: any, argument: ArgumentDefinition) throw buildError(`Cannot compute default value for argument ${argument} as the type is undefined`); } if (value === undefined) { - if (argument.defaultValue) { + if (argument.defaultValue !== undefined) { return applyDefaultValues(argument.defaultValue, argument.type); + } else if (!isNonNullType(argument.type)) { + return null; + } else { + throw ERRORS.INVALID_GRAPHQL.err(`Required argument "${argument.coordinate}" was not provided.`); } } return applyDefaultValues(value, argument.type);