Skip to content

Commit

Permalink
Preserve defaultValue literals
Browse files Browse the repository at this point in the history 
Fixes #3051

This change solves the problem of default values defined via SDL not always resolving correctly through introspection by preserving the original GraphQL literal in the schema definition. This changes argument and input field definitions `defaultValue` field from just the "value" to a new `GraphQLDefaultValueUsage` type which contains either or both "value" and "literal" fields.

Since either of these fields may be set, new functions for resolving a value or literal from either have been added - `getLiteralDefaultValue` and `getCoercedDefaultValue` - these replace uses that either assumed a set value or were manually converting a value back to a literal.

Here is the flow for how a default value defined in an SDL would be converted into a functional schema and back to an SDL:

**Before this change:**

```
(SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config) --valueToAST--> (AST) --print --> (SDL)
```

`coerceInputLiteral` performs coercion which is a one-way function, and `valueToAST` is unsafe and set to be deprecated in #3049.

**After this change:**

```
(SDL) --parse-> (defaultValue literal config) --print --> (SDL)
```
  • Loading branch information
@leebyron @yaacovCR
leebyron authored and yaacovCR committed Sep 17, 2024
1 parent ed149db commit 487d0a2
Show file tree
Hide file tree
Showing 18 changed files with 246 additions and 56 deletions.
13 changes: 7 additions & 6 deletions src/execution/getVariableSignature.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ import type { VariableDefinitionNode } from '../language/ast.js';
import { print } from '../language/printer.js';

import { isInputType } from '../type/definition.js';
import type { GraphQLInputType, GraphQLSchema } from '../type/index.js';
import type {
GraphQLDefaultValueUsage,
GraphQLInputType,
GraphQLSchema,
} from '../type/index.js';

import { coerceInputLiteral } from '../utilities/coerceInputValue.js';
import { typeFromAST } from '../utilities/typeFromAST.js';

/**
Expand All @@ -18,7 +21,7 @@ import { typeFromAST } from '../utilities/typeFromAST.js';
export interface GraphQLVariableSignature {
name: string;
type: GraphQLInputType;
defaultValue: unknown;
defaultValue: GraphQLDefaultValueUsage | undefined;
}

export function getVariableSignature(
Expand All @@ -43,8 +46,6 @@ export function getVariableSignature(
return {
name: varName,
type: varType,
defaultValue: defaultValue
? coerceInputLiteral(varDefNode.defaultValue, varType)
: undefined,
defaultValue: defaultValue ? { literal: defaultValue } : undefined,
};
}
22 changes: 16 additions & 6 deletions src/execution/values.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type { GraphQLDirective } from '../type/directives.js';
import type { GraphQLSchema } from '../type/schema.js';

import {
coerceDefaultValue,
coerceInputLiteral,
coerceInputValue,
} from '../utilities/coerceInputValue.js';
Expand Down Expand Up @@ -90,8 +91,11 @@ function coerceVariableValues(

const { name: varName, type: varType } = varSignature;
if (!Object.hasOwn(inputs, varName)) {
if (varDefNode.defaultValue) {
coercedValues[varName] = varSignature.defaultValue;
if (varSignature.defaultValue) {
coercedValues[varName] = coerceDefaultValue(
varSignature.defaultValue,
varType,
);
} else if (isNonNullType(varType)) {
const varTypeStr = inspect(varType);
onError(
Expand Down Expand Up @@ -173,8 +177,11 @@ export function experimentalGetArgumentValues(
const argumentNode = argNodeMap.get(name);

if (argumentNode == null) {
if (argDef.defaultValue !== undefined) {
coercedValues[name] = argDef.defaultValue;
if (argDef.defaultValue) {
coercedValues[name] = coerceDefaultValue(
argDef.defaultValue,
argDef.type,
);
} else if (isNonNullType(argType)) {
throw new GraphQLError(
`Argument "${name}" of required type "${inspect(argType)}" ` +
Expand All @@ -197,8 +204,11 @@ export function experimentalGetArgumentValues(
scopedVariableValues == null ||
!Object.hasOwn(scopedVariableValues, variableName)
) {
if (argDef.defaultValue !== undefined) {
coercedValues[name] = argDef.defaultValue;
if (argDef.defaultValue) {
coercedValues[name] = coerceDefaultValue(
argDef.defaultValue,
argDef.type,
);
} else if (isNonNullType(argType)) {
throw new GraphQLError(
`Argument "${name}" of required type "${inspect(argType)}" ` +
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ export type {
GraphQLScalarSerializer,
GraphQLScalarValueParser,
GraphQLScalarLiteralParser,
GraphQLDefaultValueUsage,
} from './type/index.js';

// Parse and operate on GraphQL language source files.
Expand Down
58 changes: 58 additions & 0 deletions src/type/__tests__/definition-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { describe, it } from 'mocha';
import { identityFunc } from '../../jsutils/identityFunc.js';
import { inspect } from '../../jsutils/inspect.js';

import { Kind } from '../../language/kinds.js';
import { parseValue } from '../../language/parser.js';

import type { GraphQLNullableType, GraphQLType } from '../definition.js';
Expand Down Expand Up @@ -581,6 +582,63 @@ describe('Type System: Input Objects', () => {
'not used anymore',
);
});

describe('Input Object fields may have default values', () => {
it('accepts an Input Object type with a default value', () => {
const inputObjType = new GraphQLInputObjectType({
name: 'SomeInputObject',
fields: {
f: { type: ScalarType, defaultValue: 3 },
},
});
expect(inputObjType.getFields().f).to.deep.include({
name: 'f',
description: undefined,
type: ScalarType,
defaultValue: { value: 3 },
deprecationReason: undefined,
extensions: {},
astNode: undefined,
});
});

it('accepts an Input Object type with a default value literal', () => {
const inputObjType = new GraphQLInputObjectType({
name: 'SomeInputObject',
fields: {
f: {
type: ScalarType,
defaultValueLiteral: { kind: Kind.INT, value: '3' },
},
},
});
expect(inputObjType.getFields().f).to.deep.include({
name: 'f',
description: undefined,
type: ScalarType,
defaultValue: { literal: { kind: 'IntValue', value: '3' } },
deprecationReason: undefined,
extensions: {},
astNode: undefined,
});
});

it('rejects an Input Object type with potentially conflicting default values', () => {
const inputObjType = new GraphQLInputObjectType({
name: 'SomeInputObject',
fields: {
f: {
type: ScalarType,
defaultValue: 3,
defaultValueLiteral: { kind: Kind.INT, value: '3' },
},
},
});
expect(() => inputObjType.getFields()).to.throw(
'Argument "f" has both a defaultValue and a defaultValueLiteral property, but only one must be provided.',
);
});
});
});

describe('Type System: List', () => {
Expand Down
10 changes: 8 additions & 2 deletions src/type/__tests__/predicate-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,10 @@ describe('Type predicates', () => {
name: 'someArg',
type: config.type,
description: undefined,
defaultValue: config.defaultValue,
defaultValue:
config.defaultValue !== undefined
? { value: config.defaultValue }
: undefined,
deprecationReason: null,
extensions: Object.create(null),
astNode: undefined,
Expand Down Expand Up @@ -622,7 +625,10 @@ describe('Type predicates', () => {
name: 'someInputField',
type: config.type,
description: undefined,
defaultValue: config.defaultValue,
defaultValue:
config.defaultValue !== undefined
? { value: config.defaultValue }
: undefined,
deprecationReason: null,
extensions: Object.create(null),
astNode: undefined,
Expand Down
37 changes: 31 additions & 6 deletions src/type/definition.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { toObjMap } from '../jsutils/toObjMap.js';
import { GraphQLError } from '../error/GraphQLError.js';

import type {
ConstValueNode,
EnumTypeDefinitionNode,
EnumTypeExtensionNode,
EnumValueDefinitionNode,
Expand Down Expand Up @@ -799,7 +800,7 @@ export function defineArguments(
name: assertName(argName),
description: argConfig.description,
type: argConfig.type,
defaultValue: argConfig.defaultValue,
defaultValue: defineDefaultValue(argName, argConfig),
deprecationReason: argConfig.deprecationReason,
extensions: toObjMap(argConfig.extensions),
astNode: argConfig.astNode,
Expand Down Expand Up @@ -833,7 +834,8 @@ export function argsToArgsConfig(
(arg) => ({
description: arg.description,
type: arg.type,
defaultValue: arg.defaultValue,
defaultValue: arg.defaultValue?.value,
defaultValueLiteral: arg.defaultValue?.literal,
deprecationReason: arg.deprecationReason,
extensions: arg.extensions,
astNode: arg.astNode,
Expand Down Expand Up @@ -946,6 +948,7 @@ export interface GraphQLArgumentConfig {
description?: Maybe<string>;
type: GraphQLInputType;
defaultValue?: unknown;
defaultValueLiteral?: ConstValueNode | undefined;
deprecationReason?: Maybe<string>;
extensions?: Maybe<Readonly<GraphQLArgumentExtensions>>;
astNode?: Maybe<InputValueDefinitionNode>;
Expand All @@ -971,7 +974,7 @@ export interface GraphQLArgument {
name: string;
description: Maybe<string>;
type: GraphQLInputType;
defaultValue: unknown;
defaultValue: GraphQLDefaultValueUsage | undefined;
deprecationReason: Maybe<string>;
extensions: Readonly<GraphQLArgumentExtensions>;
astNode: Maybe<InputValueDefinitionNode>;
Expand All @@ -985,6 +988,26 @@ export type GraphQLFieldMap<TSource, TContext> = ObjMap<
GraphQLField<TSource, TContext>
>;

export type GraphQLDefaultValueUsage =
| { value: unknown; literal?: never }
| { literal: ConstValueNode; value?: never };

export function defineDefaultValue(
argName: string,
config: GraphQLArgumentConfig | GraphQLInputFieldConfig,
): GraphQLDefaultValueUsage | undefined {
if (config.defaultValue === undefined && !config.defaultValueLiteral) {
return;
}
devAssert(
!(config.defaultValue !== undefined && config.defaultValueLiteral),
`Argument "${argName}" has both a defaultValue and a defaultValueLiteral property, but only one must be provided.`,
);
return config.defaultValueLiteral
? { literal: config.defaultValueLiteral }
: { value: config.defaultValue };
}

/**
* Custom extensions
*
Expand Down Expand Up @@ -1538,7 +1561,8 @@ export class GraphQLInputObjectType {
const fields = mapValue(this.getFields(), (field) => ({
description: field.description,
type: field.type,
defaultValue: field.defaultValue,
defaultValue: field.defaultValue?.value,
defaultValueLiteral: field.defaultValue?.literal,
deprecationReason: field.deprecationReason,
extensions: field.extensions,
astNode: field.astNode,
Expand Down Expand Up @@ -1572,7 +1596,7 @@ function defineInputFieldMap(
name: assertName(fieldName),
description: fieldConfig.description,
type: fieldConfig.type,
defaultValue: fieldConfig.defaultValue,
defaultValue: defineDefaultValue(fieldName, fieldConfig),
deprecationReason: fieldConfig.deprecationReason,
extensions: toObjMap(fieldConfig.extensions),
astNode: fieldConfig.astNode,
Expand Down Expand Up @@ -1613,6 +1637,7 @@ export interface GraphQLInputFieldConfig {
description?: Maybe<string>;
type: GraphQLInputType;
defaultValue?: unknown;
defaultValueLiteral?: ConstValueNode | undefined;
deprecationReason?: Maybe<string>;
extensions?: Maybe<Readonly<GraphQLInputFieldExtensions>>;
astNode?: Maybe<InputValueDefinitionNode>;
Expand All @@ -1624,7 +1649,7 @@ export interface GraphQLInputField {
name: string;
description: Maybe<string>;
type: GraphQLInputType;
defaultValue: unknown;
defaultValue: GraphQLDefaultValueUsage | undefined;
deprecationReason: Maybe<string>;
extensions: Readonly<GraphQLInputFieldExtensions>;
astNode: Maybe<InputValueDefinitionNode>;
Expand Down
1 change: 1 addition & 0 deletions src/type/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export type {
GraphQLScalarSerializer,
GraphQLScalarValueParser,
GraphQLScalarLiteralParser,
GraphQLDefaultValueUsage,
} from './definition.js';

export {
Expand Down
9 changes: 7 additions & 2 deletions src/type/introspection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,8 +407,13 @@ export const __InputValue: GraphQLObjectType = new GraphQLObjectType({
'A GraphQL-formatted string representing the default value for this input value.',
resolve(inputValue) {
const { type, defaultValue } = inputValue;
const valueAST = astFromValue(defaultValue, type);
return valueAST ? print(valueAST) : null;
if (!defaultValue) {
return null;
}
const literal =
defaultValue.literal ?? astFromValue(defaultValue.value, type);
invariant(literal != null, 'Invalid default value');
return print(literal);
},
},
isDeprecated: {
Expand Down
5 changes: 3 additions & 2 deletions src/utilities/TypeInfo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { getEnterLeaveForKind } from '../language/visitor.js';
import type {
GraphQLArgument,
GraphQLCompositeType,
GraphQLDefaultValueUsage,
GraphQLEnumValue,
GraphQLField,
GraphQLInputField,
Expand Down Expand Up @@ -53,7 +54,7 @@ export class TypeInfo {
private _parentTypeStack: Array<Maybe<GraphQLCompositeType>>;
private _inputTypeStack: Array<Maybe<GraphQLInputType>>;
private _fieldDefStack: Array<Maybe<GraphQLField<unknown, unknown>>>;
private _defaultValueStack: Array<Maybe<unknown>>;
private _defaultValueStack: Array<GraphQLDefaultValueUsage | undefined>;
private _directive: Maybe<GraphQLDirective>;
private _argument: Maybe<GraphQLArgument>;
private _enumValue: Maybe<GraphQLEnumValue>;
Expand Down Expand Up @@ -124,7 +125,7 @@ export class TypeInfo {
return this._fieldDefStack.at(-1);
}

getDefaultValue(): Maybe<unknown> {
getDefaultValue(): GraphQLDefaultValueUsage | undefined {
return this._defaultValueStack.at(-1);
}

Expand Down
23 changes: 23 additions & 0 deletions src/utilities/__tests__/buildClientSchema-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ describe('Type System: build schema from introspection', () => {
}
type Query {
defaultID(intArg: ID = "123"): String
defaultInt(intArg: Int = 30): String
defaultList(listArg: [Int] = [1, 2, 3]): String
defaultObject(objArg: Geo = { lat: 37.485, lon: -122.148 }): String
Expand Down Expand Up @@ -609,6 +610,28 @@ describe('Type System: build schema from introspection', () => {
expect(result.data).to.deep.equal({ foo: 'bar' });
});

it('can use client schema for execution if resolvers are added', () => {
const schema = buildSchema(`
type Query {
foo(bar: String = "abc"): String
}
`);

const introspection = introspectionFromSchema(schema);
const clientSchema = buildClientSchema(introspection);

const QueryType: GraphQLObjectType = clientSchema.getType('Query') as any;
QueryType.getFields().foo.resolve = (_value, args) => args.bar;

const result = graphqlSync({
schema: clientSchema,
source: '{ foo }',
});

expect(result.data).to.deep.equal({ foo: 'abc' });
expect(result.data).to.deep.equal({ foo: 'abc' });
});

it('can build invalid schema', () => {
const schema = buildSchema('type Query', { assumeValid: true });

Expand Down
Loading

0 comments on commit 487d0a2

Please sign in to comment.