diff --git a/.changeset/smart-crabs-jump.md b/.changeset/smart-crabs-jump.md new file mode 100644 index 000000000..355706ec1 --- /dev/null +++ b/.changeset/smart-crabs-jump.md @@ -0,0 +1,5 @@ +--- +"@apollo/federation-internals": patch +--- + +Fixed demand control validations to unwrap non-nullable composite types and fields when performing validations. \ No newline at end of file diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index 9c8414815..af69f5850 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -1,6 +1,6 @@ import { DocumentNode } from 'graphql'; import gql from 'graphql-tag'; -import { Subgraph } from '..'; +import { asFed2SubgraphDocument, Subgraph } from '..'; import { buildSubgraph } from '../federation'; import { defaultPrintOptions, printSchema } from '../print'; import { buildForErrors } from './testUtils'; @@ -733,9 +733,18 @@ describe('custom error message for misnamed directives', () => { }); }); -function buildAndValidate(doc: DocumentNode): Subgraph { - const name = 'S'; - return buildSubgraph(name, `http://${name}`, doc).validate(); +function buildAndValidate( + doc: DocumentNode, + options?: { + subgraphName?: string; + asFed2?: boolean; + includeAllImports?: boolean; + }, +): Subgraph { + const subgraphDoc = + options?.asFed2 ?? true ? asFed2SubgraphDocument(doc, { ...options }) : doc; + const name = options?.subgraphName ?? 'S'; + return buildSubgraph(name, `http://${name}`, subgraphDoc).validate(); } describe('@core/@link handling', () => { @@ -824,7 +833,7 @@ describe('@core/@link handling', () => { } `; - validateFullSchema(buildAndValidate(doc)); + validateFullSchema(buildAndValidate(doc, { asFed2: false })); }); it('expands definitions if both the federation spec and link spec are linked', () => { @@ -838,11 +847,13 @@ describe('@core/@link handling', () => { } `; - validateFullSchema(buildAndValidate(doc)); + validateFullSchema(buildAndValidate(doc, { asFed2: false })); }); it('is valid if a schema is complete from the get-go', () => { - validateFullSchema(buildAndValidate(gql(expectedFullSchema))); + validateFullSchema( + buildAndValidate(gql(expectedFullSchema), { asFed2: false }), + ); }); it('expands missing definitions when some are partially provided', () => { @@ -929,7 +940,7 @@ describe('@core/@link handling', () => { // calls the graphQL-js validation, so we can be somewhat sure that if something necessary wasn't expanded // properly, we would have an issue. The main reason we did validate the full schema in prior tests is // so we had at least one full example of a subgraph expansion in the tests. - docs.forEach((doc) => buildAndValidate(doc)); + docs.forEach((doc) => buildAndValidate(doc, { asFed2: false })); }); it('allows known directives with incomplete but compatible definitions', () => { @@ -1033,7 +1044,7 @@ describe('@core/@link handling', () => { ]; // Like above, we really only care that the examples validate. - docs.forEach((doc) => buildAndValidate(doc)); + docs.forEach((doc) => buildAndValidate(doc, { asFed2: false })); }); it('errors on invalid known directive location', () => { @@ -1187,7 +1198,7 @@ describe('@core/@link handling', () => { `; // Just making sure this don't error out. - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('allows defining a repeatable directive as non-repeatable but validates usages', () => { @@ -1229,7 +1240,7 @@ describe('federation 1 schema', () => { directive @requires on FIELD_DEFINITION `; - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('accepts federation directive definitions with nullable arguments', () => { @@ -1251,7 +1262,7 @@ describe('federation 1 schema', () => { directive @requires(fields: String) on FIELD_DEFINITION `; - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('accepts federation directive definitions with "FieldSet" type instead of "_FieldSet"', () => { @@ -1268,7 +1279,7 @@ describe('federation 1 schema', () => { directive @key(fields: FieldSet) on OBJECT | INTERFACE `; - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('rejects federation directive definition with unknown arguments', () => { @@ -1507,10 +1518,54 @@ describe('@cost', () => { }); describe('@listSize', () => { + it('works on lists', () => { + const doc = gql` + type Query { + a1: [A]! @listSize(assumedSize: 5) + a2: [A] @listSize(assumedSize: 5) + } + + type A { + field: String + } + `; + + expect(buildAndValidate(doc, { includeAllImports: true })); + }); + + it('works on valid sizedFields', () => { + const doc = gql` + type Query { + a1: A @listSize(assumedSize: 10, sizedFields: ["nullableList"]) + a2: A + @listSize( + assumedSize: 10 + sizedFields: ["nullableListOfNullableInts"] + ) + a3: A @listSize(assumedSize: 10, sizedFields: ["ints"]) + a4: A! @listSize(assumedSize: 10, sizedFields: ["nullableList"]) + a5: A! + @listSize( + assumedSize: 10 + sizedFields: ["nullableListOfNullableInts"] + ) + a6: A! @listSize(assumedSize: 10, sizedFields: ["ints"]) + } + + type A { + nullableListOfNullableInts: [Int] + nullableList: [Int!] + ints: [Int!]! + } + `; + + expect(buildAndValidate(doc, { includeAllImports: true })); + }); + it('rejects applications on non-lists (unless it uses sizedFields)', () => { const doc = gql` type Query { - a1: A @listSize(assumedSize: 5) + a1: A! @listSize(assumedSize: 5) a2: A @listSize(assumedSize: 10, sizedFields: ["ints"]) } diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index a9caf67af..0244cb32a 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -1071,7 +1071,7 @@ function validateListSizeAppliedToList( ) { const { sizedFields = [] } = application.arguments(); // @listSize must be applied to a list https://ibm.github.io/graphql-specs/cost-spec.html#sec-Valid-List-Size-Target - if (!sizedFields.length && parent.type && !isListType(parent.type)) { + if (!sizedFields.length && parent.type && !isListType(parent.type) && !isNonNullListType(parent.type)) { errorCollector.push(ERRORS.LIST_SIZE_APPLIED_TO_NON_LIST.err( `"${parent.coordinate}" is not a list`, { nodes: sourceASTs(application, parent) }, @@ -1141,8 +1141,9 @@ function validateSizedFieldsAreValidLists( ) { const { sizedFields = [] } = application.arguments(); // Validate sizedFields https://ibm.github.io/graphql-specs/cost-spec.html#sec-Valid-Sized-Fields-Target - if (sizedFields.length) { - if (!parent.type || !isCompositeType(parent.type)) { + if (sizedFields.length && parent.type) { + const baseParentType = baseType(parent.type); + if (!isCompositeType(baseParentType)) { // The output type must have fields errorCollector.push(ERRORS.LIST_SIZE_INVALID_SIZED_FIELD.err( `Sized fields cannot be used because "${parent.type}" is not a composite type`, @@ -1150,11 +1151,11 @@ function validateSizedFieldsAreValidLists( )); } else { for (const sizedFieldName of sizedFields) { - const sizedField = parent.type.field(sizedFieldName); + const sizedField = baseParentType.field(sizedFieldName); if (!sizedField) { // Sized fields must be present on the output type errorCollector.push(ERRORS.LIST_SIZE_INVALID_SIZED_FIELD.err( - `Sized field "${sizedFieldName}" is not a field on type "${parent.type.coordinate}"`, + `Sized field "${sizedFieldName}" is not a field on type "${baseParentType.coordinate}"`, { nodes: sourceASTs(application, parent) } )); } else if (!sizedField.type || !(isListType(sizedField.type) || isNonNullListType(sizedField.type))) {