Skip to content

Commit

Permalink
standardize error messages prior to introducing schema coordinates (#…
Browse files Browse the repository at this point in the history
…4177)

extracted from #3808

PR #3808 uses schema coordinates to improve GraphQL-JS error messages.
To reduce the size of the PR, this commit standardizes error messages
according to the general pattern that will be introduced with schema
coordinates without introducing the coordinates themselves, in the hopes
of aiding review of the later PR.

EDITED 8/26/2024:

I was able to reproduce all of the standardized error messages from
#3808 except for the ones in getArgumentValues when it is passed a Field
Definition, because the parent type is not passed. Everything else can
be calculated for the error messages we are currently printing, although
schema coordinates simplifies things.

Extracting these changes out of #3808 and rebasing #3808 on main will
therefore will better demonstrate how schema coordinates improves the
clarity of some of our error messages (namely, getArgumentValues) and
simplifies printing them.
  • Loading branch information
yaacovCR authored Sep 4, 2024
1 parent 67afee4 commit 426b017
Show file tree
Hide file tree
Showing 24 changed files with 227 additions and 199 deletions.
13 changes: 7 additions & 6 deletions src/execution/__tests__/variables-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Argument "input" has invalid value ["foo", "bar", "baz"].',
'Argument "input" of type "TestInputObject" has invalid value ["foo", "bar", "baz"].',
path: ['fieldWithObjectInput'],
locations: [{ line: 3, column: 41 }],
},
Expand Down Expand Up @@ -262,7 +262,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Argument "input" has invalid value { c: "foo", e: "bar" }.',
'Argument "input" of type "TestInputObject" has invalid value { c: "foo", e: "bar" }.',
path: ['fieldWithObjectInput'],
locations: [{ line: 3, column: 41 }],
},
Expand Down Expand Up @@ -462,7 +462,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value { a: "foo", b: "bar" }; Field "c" of required type "String!" was not provided.',
'Variable "$input" got invalid value { a: "foo", b: "bar" }; Field "TestInputObject.c" of required type "String!" was not provided.',
locations: [{ line: 2, column: 16 }],
},
],
Expand All @@ -481,12 +481,12 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value { a: "foo" } at "input.na"; Field "c" of required type "String!" was not provided.',
'Variable "$input" got invalid value { a: "foo" } at "input.na"; Field "TestInputObject.c" of required type "String!" was not provided.',
locations: [{ line: 2, column: 18 }],
},
{
message:
'Variable "$input" got invalid value { na: { a: "foo" } }; Field "nb" of required type "String!" was not provided.',
'Variable "$input" got invalid value { na: { a: "foo" } }; Field "TestNestedInputObject.nb" of required type "String!" was not provided.',
locations: [{ line: 2, column: 18 }],
},
],
Expand Down Expand Up @@ -1042,7 +1042,8 @@ describe('Execute: Handles inputs', () => {
},
errors: [
{
message: 'Argument "input" has invalid value WRONG_TYPE.',
message:
'Argument "input" of type "String" has invalid value WRONG_TYPE.',
locations: [{ line: 3, column: 48 }],
path: ['fieldWithDefaultArgumentValue'],
},
Expand Down
16 changes: 8 additions & 8 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,7 @@ function completeValue(
);
if ((completed as GraphQLWrappedResult<unknown>)[0] === null) {
throw new Error(
`Cannot return null for non-nullable field ${info.parentType.name}.${info.fieldName}.`,
`Cannot return null for non-nullable field ${info.parentType}.${info.fieldName}.`,
);
}
return completed;
Expand Down Expand Up @@ -1258,7 +1258,7 @@ function completeListValue(

if (!isIterableObject(result)) {
throw new GraphQLError(
`Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`,
`Expected Iterable, but did not find one for field "${info.parentType}.${info.fieldName}".`,
);
}

Expand Down Expand Up @@ -1565,7 +1565,7 @@ function ensureValidRuntimeType(
): GraphQLObjectType {
if (runtimeTypeName == null) {
throw new GraphQLError(
`Abstract type "${returnType.name}" must resolve to an Object type at runtime for field "${info.parentType.name}.${info.fieldName}". Either the "${returnType.name}" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.`,
`Abstract type "${returnType}" must resolve to an Object type at runtime for field "${info.parentType}.${info.fieldName}". Either the "${returnType}" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.`,
{ nodes: toNodes(fieldGroup) },
);
}
Expand All @@ -1580,29 +1580,29 @@ function ensureValidRuntimeType(

if (typeof runtimeTypeName !== 'string') {
throw new GraphQLError(
`Abstract type "${returnType.name}" must resolve to an Object type at runtime for field "${info.parentType.name}.${info.fieldName}" with ` +
`Abstract type "${returnType}" must resolve to an Object type at runtime for field "${info.parentType}.${info.fieldName}" with ` +
`value ${inspect(result)}, received "${inspect(runtimeTypeName)}".`,
);
}

const runtimeType = exeContext.schema.getType(runtimeTypeName);
if (runtimeType == null) {
throw new GraphQLError(
`Abstract type "${returnType.name}" was resolved to a type "${runtimeTypeName}" that does not exist inside the schema.`,
`Abstract type "${returnType}" was resolved to a type "${runtimeTypeName}" that does not exist inside the schema.`,
{ nodes: toNodes(fieldGroup) },
);
}

if (!isObjectType(runtimeType)) {
throw new GraphQLError(
`Abstract type "${returnType.name}" was resolved to a non-object type "${runtimeTypeName}".`,
`Abstract type "${returnType}" was resolved to a non-object type "${runtimeTypeName}".`,
{ nodes: toNodes(fieldGroup) },
);
}

if (!exeContext.schema.isSubType(returnType, runtimeType)) {
throw new GraphQLError(
`Runtime Object type "${runtimeType.name}" is not a possible type for "${returnType.name}".`,
`Runtime Object type "${runtimeType}" is not a possible type for "${returnType}".`,
{ nodes: toNodes(fieldGroup) },
);
}
Expand Down Expand Up @@ -1668,7 +1668,7 @@ function invalidReturnTypeError(
fieldGroup: FieldGroup,
): GraphQLError {
return new GraphQLError(
`Expected value of type "${returnType.name}" but got: ${inspect(result)}.`,
`Expected value of type "${returnType}" but got: ${inspect(result)}.`,
{ nodes: toNodes(fieldGroup) },
);
}
Expand Down
4 changes: 3 additions & 1 deletion src/execution/values.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,9 @@ export function getArgumentValues(
// execution. This is a runtime check to ensure execution does not
// continue with an invalid argument value.
throw new GraphQLError(
`Argument "${name}" has invalid value ${print(valueNode)}.`,
`Argument "${name}" of type "${inspect(
argType,
)}" has invalid value ${print(valueNode)}.`,
{ nodes: valueNode },
);
}
Expand Down
6 changes: 3 additions & 3 deletions src/type/__tests__/introspection-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1644,7 +1644,7 @@ describe('Introspection', () => {
errors: [
{
message:
'Field "__type" argument "name" of type "String!" is required, but it was not provided.',
'Argument "<meta>.__type(name:)" of type "String!" is required, but it was not provided.',
locations: [{ line: 3, column: 9 }],
},
],
Expand Down Expand Up @@ -1738,11 +1738,11 @@ describe('Introspection', () => {
_3: any,
info: GraphQLResolveInfo,
): never {
expect.fail(`Called on ${info.parentType.name}::${info.fieldName}`);
expect.fail(`Called on ${info.parentType}::${info.fieldName}`);
}

function typeResolver(_1: any, _2: any, info: GraphQLResolveInfo): never {
expect.fail(`Called on ${info.parentType.name}::${info.fieldName}`);
expect.fail(`Called on ${info.parentType}::${info.fieldName}`);
}
/* c8 ignore stop */

Expand Down
4 changes: 2 additions & 2 deletions src/type/__tests__/validation-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2089,7 +2089,7 @@ describe('Objects must adhere to Interface they implement', () => {
expectJSON(validateSchema(schema)).toDeepEqual([
{
message:
'Object field AnotherObject.field includes required argument requiredArg that is missing from the Interface field AnotherInterface.field.',
'Argument "AnotherObject.field(requiredArg:)" must not be required type "String!" if not provided by the Interface field "AnotherInterface.field".',
locations: [
{ line: 13, column: 11 },
{ line: 7, column: 9 },
Expand Down Expand Up @@ -2546,7 +2546,7 @@ describe('Interfaces must adhere to Interface they implement', () => {
expectJSON(validateSchema(schema)).toDeepEqual([
{
message:
'Object field ChildInterface.field includes required argument requiredArg that is missing from the Interface field ParentInterface.field.',
'Argument "ChildInterface.field(requiredArg:)" must not be required type "String!" if not provided by the Interface field "ParentInterface.field".',
locations: [
{ line: 13, column: 11 },
{ line: 7, column: 9 },
Expand Down
48 changes: 26 additions & 22 deletions src/type/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ function validateRootTypes(context: SchemaValidationContext): void {
if (operationTypes.length > 1) {
const operationList = andList(operationTypes);
context.reportError(
`All root types must be different, "${rootType.name}" type is used as ${operationList} root types.`,
`All root types must be different, "${rootType}" type is used as ${operationList} root types.`,
operationTypes.map((operationType) =>
getOperationTypeNode(schema, operationType),
),
Expand Down Expand Up @@ -185,7 +185,7 @@ function validateDirectives(context: SchemaValidationContext): void {

if (directive.locations.length === 0) {
context.reportError(
`Directive @${directive.name} must include 1 or more locations.`,
`Directive ${directive} must include 1 or more locations.`,
directive.astNode,
);
}
Expand All @@ -198,15 +198,15 @@ function validateDirectives(context: SchemaValidationContext): void {
// Ensure the type is an input type.
if (!isInputType(arg.type)) {
context.reportError(
`The type of @${directive.name}(${arg.name}:) must be Input Type ` +
`The type of ${directive}(${arg.name}:) must be Input Type ` +
`but got: ${inspect(arg.type)}.`,
arg.astNode,
);
}

if (isRequiredArgument(arg) && arg.deprecationReason != null) {
context.reportError(
`Required argument @${directive.name}(${arg.name}:) cannot be deprecated.`,
`Required argument ${directive}(${arg.name}:) cannot be deprecated.`,
[getDeprecatedDirectiveNode(arg.astNode), arg.astNode?.type],
);
}
Expand Down Expand Up @@ -282,7 +282,7 @@ function validateFields(

// Objects and Interfaces both must define one or more fields.
if (fields.length === 0) {
context.reportError(`Type ${type.name} must define one or more fields.`, [
context.reportError(`Type ${type} must define one or more fields.`, [
type.astNode,
...type.extensionASTNodes,
]);
Expand All @@ -295,7 +295,7 @@ function validateFields(
// Ensure the type is an output type
if (!isOutputType(field.type)) {
context.reportError(
`The type of ${type.name}.${field.name} must be Output Type ` +
`The type of ${type}.${field.name} must be Output Type ` +
`but got: ${inspect(field.type)}.`,
field.astNode?.type,
);
Expand All @@ -311,15 +311,15 @@ function validateFields(
// Ensure the type is an input type
if (!isInputType(arg.type)) {
context.reportError(
`The type of ${type.name}.${field.name}(${argName}:) must be Input ` +
`The type of ${type}.${field.name}(${argName}:) must be Input ` +
`Type but got: ${inspect(arg.type)}.`,
arg.astNode?.type,
);
}

if (isRequiredArgument(arg) && arg.deprecationReason != null) {
context.reportError(
`Required argument ${type.name}.${field.name}(${argName}:) cannot be deprecated.`,
`Required argument ${type}.${field.name}(${argName}:) cannot be deprecated.`,
[getDeprecatedDirectiveNode(arg.astNode), arg.astNode?.type],
);
}
Expand All @@ -344,15 +344,15 @@ function validateInterfaces(

if (type === iface) {
context.reportError(
`Type ${type.name} cannot implement itself because it would create a circular reference.`,
`Type ${type} cannot implement itself because it would create a circular reference.`,
getAllImplementsInterfaceNodes(type, iface),
);
continue;
}

if (ifaceTypeNames.has(iface.name)) {
context.reportError(
`Type ${type.name} can only implement ${iface.name} once.`,
`Type ${type} can only implement ${iface.name} once.`,
getAllImplementsInterfaceNodes(type, iface),
);
continue;
Expand Down Expand Up @@ -380,7 +380,7 @@ function validateTypeImplementsInterface(
// Assert interface field exists on type.
if (typeField == null) {
context.reportError(
`Interface field ${iface.name}.${fieldName} expected but ${type.name} does not provide it.`,
`Interface field ${iface.name}.${fieldName} expected but ${type} does not provide it.`,
[ifaceField.astNode, type.astNode, ...type.extensionASTNodes],
);
continue;
Expand All @@ -391,7 +391,7 @@ function validateTypeImplementsInterface(
if (!isTypeSubTypeOf(context.schema, typeField.type, ifaceField.type)) {
context.reportError(
`Interface field ${iface.name}.${fieldName} expects type ` +
`${inspect(ifaceField.type)} but ${type.name}.${fieldName} ` +
`${inspect(ifaceField.type)} but ${type}.${fieldName} ` +
`is type ${inspect(typeField.type)}.`,
[ifaceField.astNode?.type, typeField.astNode?.type],
);
Expand All @@ -405,7 +405,7 @@ function validateTypeImplementsInterface(
// Assert interface field arg exists on object field.
if (!typeArg) {
context.reportError(
`Interface field argument ${iface.name}.${fieldName}(${argName}:) expected but ${type.name}.${fieldName} does not provide it.`,
`Interface field argument ${iface.name}.${fieldName}(${argName}:) expected but ${type}.${fieldName} does not provide it.`,
[ifaceArg.astNode, typeField.astNode],
);
continue;
Expand All @@ -418,7 +418,7 @@ function validateTypeImplementsInterface(
context.reportError(
`Interface field argument ${iface.name}.${fieldName}(${argName}:) ` +
`expects type ${inspect(ifaceArg.type)} but ` +
`${type.name}.${fieldName}(${argName}:) is type ` +
`${type}.${fieldName}(${argName}:) is type ` +
`${inspect(typeArg.type)}.`,
[ifaceArg.astNode?.type, typeArg.astNode?.type],
);
Expand All @@ -433,7 +433,11 @@ function validateTypeImplementsInterface(
const ifaceArg = ifaceField.args.find((arg) => arg.name === argName);
if (!ifaceArg && isRequiredArgument(typeArg)) {
context.reportError(
`Object field ${type.name}.${fieldName} includes required argument ${argName} that is missing from the Interface field ${iface.name}.${fieldName}.`,
`Argument "${type}.${fieldName}(${argName}:)" must not be required type "${inspect(
typeArg.type,
)}" if not provided by the Interface field "${
iface.name
}.${fieldName}".`,
[typeArg.astNode, ifaceField.astNode],
);
}
Expand All @@ -451,8 +455,8 @@ function validateTypeImplementsAncestors(
if (!ifaceInterfaces.includes(transitive)) {
context.reportError(
transitive === type
? `Type ${type.name} cannot implement ${iface.name} because it would create a circular reference.`
: `Type ${type.name} must implement ${transitive.name} because it is implemented by ${iface.name}.`,
? `Type ${type} cannot implement ${iface.name} because it would create a circular reference.`
: `Type ${type} must implement ${transitive.name} because it is implemented by ${iface.name}.`,
[
...getAllImplementsInterfaceNodes(iface, transitive),
...getAllImplementsInterfaceNodes(type, iface),
Expand All @@ -479,7 +483,7 @@ function validateUnionMembers(
for (const memberType of memberTypes) {
if (includedTypeNames.has(memberType.name)) {
context.reportError(
`Union type ${union.name} can only include type ${memberType.name} once.`,
`Union type ${union.name} can only include type ${memberType} once.`,
getUnionMemberTypeNodes(union, memberType.name),
);
continue;
Expand All @@ -503,7 +507,7 @@ function validateEnumValues(

if (enumValues.length === 0) {
context.reportError(
`Enum type ${enumType.name} must define one or more values.`,
`Enum type ${enumType} must define one or more values.`,
[enumType.astNode, ...enumType.extensionASTNodes],
);
}
Expand Down Expand Up @@ -561,14 +565,14 @@ function validateOneOfInputObjectField(
): void {
if (isNonNullType(field.type)) {
context.reportError(
`OneOf input field ${type.name}.${field.name} must be nullable.`,
`OneOf input field ${type}.${field.name} must be nullable.`,
field.astNode?.type,
);
}

if (field.defaultValue !== undefined) {
context.reportError(
`OneOf input field ${type.name}.${field.name} cannot have a default value.`,
`OneOf input field ${type}.${field.name} cannot have a default value.`,
field.astNode,
);
}
Expand Down Expand Up @@ -614,7 +618,7 @@ function createInputObjectCircularRefsValidator(
const cyclePath = fieldPath.slice(cycleIndex);
const pathStr = cyclePath.map((fieldObj) => fieldObj.name).join('.');
context.reportError(
`Cannot reference Input Object "${fieldType.name}" within itself through a series of non-null fields: "${pathStr}".`,
`Cannot reference Input Object "${fieldType}" within itself through a series of non-null fields: "${pathStr}".`,
cyclePath.map((fieldObj) => fieldObj.astNode),
);
}
Expand Down
3 changes: 2 additions & 1 deletion src/utilities/__tests__/coerceInputValue-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ describe('coerceInputValue', () => {
const result = coerceValue({ bar: 123 }, TestInputObject);
expectErrors(result).to.deep.equal([
{
error: 'Field "foo" of required type "Int!" was not provided.',
error:
'Field "TestInputObject.foo" of required type "Int!" was not provided.',
path: [],
value: { bar: 123 },
},
Expand Down
Loading

0 comments on commit 426b017

Please sign in to comment.