Better handling of wrapped types in list size validations (#3157)

Better handling of wrapped types in list size validations
apollographql · Sep 26, 2024 · cda078c · cda078c
1 parent e1e2605
commit cda078c
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 9 deletions.
diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts
@@ -1584,8 +1584,16 @@ describe('@listSize', () => {
         @link(url: "https://specs.apollo.dev/cost/v0.1", import: ["@listSize"])
 
       type Query {
-        sliced(first: String, second: Int, third: Int!): [String]
-          @listSize(slicingArguments: ["first", "second", "third"])
+        sliced(
+          first: String
+          second: Int
+          third: Int!
+          fourth: [Int]
+          fifth: [Int]!
+        ): [String]
+          @listSize(
+            slicingArguments: ["first", "second", "third", "fourth", "fifth"]
+          )
       }
     `;
 
@@ -1594,6 +1602,14 @@ describe('@listSize', () => {
         'LIST_SIZE_INVALID_SLICING_ARGUMENT',
         `[S] Slicing argument "Query.sliced(first:)" must be Int or Int!`,
       ],
+      [
+        'LIST_SIZE_INVALID_SLICING_ARGUMENT',
+        `[S] Slicing argument "Query.sliced(fourth:)" must be Int or Int!`,
+      ],
+      [
+        'LIST_SIZE_INVALID_SLICING_ARGUMENT',
+        `[S] Slicing argument "Query.sliced(fifth:)" must be Int or Int!`,
+      ],
     ]);
   });
 
@@ -1620,7 +1636,7 @@ describe('@listSize', () => {
     expect(buildForErrors(doc)).toStrictEqual([
       [
         'LIST_SIZE_INVALID_SIZED_FIELD',
-        `[S] Sized fields cannot be used because "Int" is not an object type`,
+        `[S] Sized fields cannot be used because "Int" is not a composite type`,
       ],
     ]);
   });
@@ -1653,10 +1669,16 @@ describe('@listSize', () => {
         @link(url: "https://specs.apollo.dev/cost/v0.1", import: ["@listSize"])
 
       type Query {
-        a: A @listSize(assumedSize: 5, sizedFields: ["notList"])
+        a: A
+          @listSize(
+            assumedSize: 5
+            sizedFields: ["list", "nonNullList", "notList"]
+          )
       }
 
       type A {
+        list: [String]
+        nonNullList: [String]!
         notList: String
       }
     `;

diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts
@@ -37,8 +37,9 @@ import {
   isWrapperType,
   possibleRuntimeTypes,
   isIntType,
+  Type,
 } from "./definitions";
-import { assert, MultiMap, printHumanReadableList, OrderedMap, mapValues, assertUnreachable, isDefined } from "./utils";
+import { assert, MultiMap, printHumanReadableList, OrderedMap, mapValues, assertUnreachable } from "./utils";
 import { SDLValidationRule } from "graphql/validation/ValidationContext";
 import { specifiedSDLRules } from "graphql/validation/specifiedRules";
 import {
@@ -1097,14 +1098,18 @@ function validateAssumedSizeNotNegative(
   // We omit this check to keep the validations to those that will otherwise cause runtime failures.
   //
   // With all that said, assumed size should not be negative.
-  if (isDefined(assumedSize) && assumedSize < 0) {
+  if (assumedSize !== undefined && assumedSize !== null && assumedSize < 0) {
     errorCollector.push(ERRORS.LIST_SIZE_INVALID_ASSUMED_SIZE.err(
       `Assumed size of "${parent.coordinate}" cannot be negative`,
       { nodes: sourceASTs(application, parent) },
     ));
   }
 }
 
+function isNonNullIntType(ty: Type): boolean {
+  return isNonNullType(ty) && isIntType(ty.ofType)
+}
+
 function validateSlicingArgumentsAreValidIntegers(
   application: Directive<SchemaElement<any, any>, ListSizeDirectiveArguments>,
   parent: FieldDefinition<CompositeType>,
@@ -1120,7 +1125,7 @@ function validateSlicingArgumentsAreValidIntegers(
         `Slicing argument "${slicingArgumentName}" is not an argument of "${parent.coordinate}"`,
         { nodes: sourceASTs(application, parent) }
       ));
-    } else if (!isIntType(slicingArgument.type) && !(isNonNullType(slicingArgument.type) && isIntType(slicingArgument.type.baseType()))) {
+    } else if (!isIntType(slicingArgument.type) && !isNonNullIntType(slicingArgument.type)) {
       // Slicing arguments must be Int or Int!
       errorCollector.push(ERRORS.LIST_SIZE_INVALID_SLICING_ARGUMENT.err(
         `Slicing argument "${slicingArgument.coordinate}" must be Int or Int!`,
@@ -1130,6 +1135,10 @@ function validateSlicingArgumentsAreValidIntegers(
   }
 }
 
+function isNonNullListType(ty: Type): boolean {
+  return isNonNullType(ty) && isListType(ty.ofType)
+}
+
 function validateSizedFieldsAreValidLists(
   application: Directive<SchemaElement<any, any>, ListSizeDirectiveArguments>,
   parent: FieldDefinition<CompositeType>,
@@ -1141,7 +1150,7 @@ function validateSizedFieldsAreValidLists(
     if (!parent.type || !isCompositeType(parent.type)) {
       // 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 an object type`,
+        `Sized fields cannot be used because "${parent.type}" is not a composite type`,
         { nodes: sourceASTs(application, parent)}
       ));
     } else {
@@ -1153,7 +1162,7 @@ function validateSizedFieldsAreValidLists(
             `Sized field "${sizedFieldName}" is not a field on type "${parent.type.coordinate}"`,
             { nodes: sourceASTs(application, parent) }
           ));
-        } else if (!sizedField.type || !isListType(sizedField.type)) {
+        } else if (!sizedField.type || !(isListType(sizedField.type) || isNonNullListType(sizedField.type))) {
           // Sized fields must be lists
           errorCollector.push(ERRORS.LIST_SIZE_APPLIED_TO_NON_LIST.err(
             `Sized field "${sizedField.coordinate}" is not a list`,