Skip to content

Commit 04b0475

Browse files
yaacovCRleebyron
andcommitted
Preserve defaultValue literals (graphql#3810)
[graphql#3074 rebased on main](graphql#3074). Depends on graphql#3809 @leebyron comments from original PR (edited, hopefully correctly): > Fixes graphql#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 (EDIT: but not both!) "value" and "literal" fields. > > 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 graphql#3049. > > **After this change:** > > ``` > (SDL) --parse-> (defaultValue literal config) --print --> (SDL) > ``` Co-authored-by: Lee Byron <lee.byron@robinhood.com>
1 parent dc6c370 commit 04b0475

19 files changed

+247
-57
lines changed

src/execution/getVariableSignature.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,12 @@ import type { VariableDefinitionNode } from '../language/ast.js';
44
import { print } from '../language/printer.js';
55

66
import { isInputType } from '../type/definition.js';
7-
import type { GraphQLInputType, GraphQLSchema } from '../type/index.js';
7+
import type {
8+
GraphQLDefaultValueUsage,
9+
GraphQLInputType,
10+
GraphQLSchema,
11+
} from '../type/index.js';
812

9-
import { coerceInputLiteral } from '../utilities/coerceInputValue.js';
1013
import { typeFromAST } from '../utilities/typeFromAST.js';
1114

1215
/**
@@ -18,7 +21,7 @@ import { typeFromAST } from '../utilities/typeFromAST.js';
1821
export interface GraphQLVariableSignature {
1922
name: string;
2023
type: GraphQLInputType;
21-
defaultValue: unknown;
24+
defaultValue: GraphQLDefaultValueUsage | undefined;
2225
}
2326

2427
export function getVariableSignature(
@@ -43,8 +46,6 @@ export function getVariableSignature(
4346
return {
4447
name: varName,
4548
type: varType,
46-
defaultValue: defaultValue
47-
? coerceInputLiteral(varDefNode.defaultValue, varType)
48-
: undefined,
49+
defaultValue: defaultValue ? { literal: defaultValue } : undefined,
4950
};
5051
}

src/execution/values.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import type { GraphQLDirective } from '../type/directives.js';
2020
import type { GraphQLSchema } from '../type/schema.js';
2121

2222
import {
23+
coerceDefaultValue,
2324
coerceInputLiteral,
2425
coerceInputValue,
2526
} from '../utilities/coerceInputValue.js';
@@ -90,8 +91,11 @@ function coerceVariableValues(
9091

9192
const { name: varName, type: varType } = varSignature;
9293
if (!Object.hasOwn(inputs, varName)) {
93-
if (varDefNode.defaultValue) {
94-
coercedValues[varName] = varSignature.defaultValue;
94+
if (varSignature.defaultValue) {
95+
coercedValues[varName] = coerceDefaultValue(
96+
varSignature.defaultValue,
97+
varType,
98+
);
9599
} else if (isNonNullType(varType)) {
96100
const varTypeStr = inspect(varType);
97101
onError(
@@ -173,8 +177,11 @@ export function experimentalGetArgumentValues(
173177
const argumentNode = argNodeMap.get(name);
174178

175179
if (argumentNode == null) {
176-
if (argDef.defaultValue !== undefined) {
177-
coercedValues[name] = argDef.defaultValue;
180+
if (argDef.defaultValue) {
181+
coercedValues[name] = coerceDefaultValue(
182+
argDef.defaultValue,
183+
argDef.type,
184+
);
178185
} else if (isNonNullType(argType)) {
179186
throw new GraphQLError(
180187
`Argument "${name}" of required type "${inspect(argType)}" ` +
@@ -197,8 +204,11 @@ export function experimentalGetArgumentValues(
197204
scopedVariableValues == null ||
198205
!Object.hasOwn(scopedVariableValues, variableName)
199206
) {
200-
if (argDef.defaultValue !== undefined) {
201-
coercedValues[name] = argDef.defaultValue;
207+
if (argDef.defaultValue) {
208+
coercedValues[name] = coerceDefaultValue(
209+
argDef.defaultValue,
210+
argDef.type,
211+
);
202212
} else if (isNonNullType(argType)) {
203213
throw new GraphQLError(
204214
`Argument "${name}" of required type "${inspect(argType)}" ` +

src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ export type {
197197
GraphQLScalarSerializer,
198198
GraphQLScalarValueParser,
199199
GraphQLScalarLiteralParser,
200+
GraphQLDefaultValueUsage,
200201
} from './type/index.js';
201202

202203
// Parse and operate on GraphQL language source files.

src/type/__tests__/definition-test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { describe, it } from 'mocha';
44
import { identityFunc } from '../../jsutils/identityFunc.js';
55
import { inspect } from '../../jsutils/inspect.js';
66

7+
import { Kind } from '../../language/kinds.js';
78
import { parseValue } from '../../language/parser.js';
89

910
import type { GraphQLNullableType, GraphQLType } from '../definition.js';
@@ -581,6 +582,63 @@ describe('Type System: Input Objects', () => {
581582
'not used anymore',
582583
);
583584
});
585+
586+
describe('Input Object fields may have default values', () => {
587+
it('accepts an Input Object type with a default value', () => {
588+
const inputObjType = new GraphQLInputObjectType({
589+
name: 'SomeInputObject',
590+
fields: {
591+
f: { type: ScalarType, defaultValue: 3 },
592+
},
593+
});
594+
expect(inputObjType.getFields().f).to.deep.include({
595+
name: 'f',
596+
description: undefined,
597+
type: ScalarType,
598+
defaultValue: { value: 3 },
599+
deprecationReason: undefined,
600+
extensions: {},
601+
astNode: undefined,
602+
});
603+
});
604+
605+
it('accepts an Input Object type with a default value literal', () => {
606+
const inputObjType = new GraphQLInputObjectType({
607+
name: 'SomeInputObject',
608+
fields: {
609+
f: {
610+
type: ScalarType,
611+
defaultValueLiteral: { kind: Kind.INT, value: '3' },
612+
},
613+
},
614+
});
615+
expect(inputObjType.getFields().f).to.deep.include({
616+
name: 'f',
617+
description: undefined,
618+
type: ScalarType,
619+
defaultValue: { literal: { kind: 'IntValue', value: '3' } },
620+
deprecationReason: undefined,
621+
extensions: {},
622+
astNode: undefined,
623+
});
624+
});
625+
626+
it('rejects an Input Object type with potentially conflicting default values', () => {
627+
const inputObjType = new GraphQLInputObjectType({
628+
name: 'SomeInputObject',
629+
fields: {
630+
f: {
631+
type: ScalarType,
632+
defaultValue: 3,
633+
defaultValueLiteral: { kind: Kind.INT, value: '3' },
634+
},
635+
},
636+
});
637+
expect(() => inputObjType.getFields()).to.throw(
638+
'Argument "f" has both a defaultValue and a defaultValueLiteral property, but only one must be provided.',
639+
);
640+
});
641+
});
584642
});
585643

586644
describe('Type System: List', () => {

src/type/__tests__/predicate-test.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,10 @@ describe('Type predicates', () => {
574574
name: 'someArg',
575575
type: config.type,
576576
description: undefined,
577-
defaultValue: config.defaultValue,
577+
defaultValue:
578+
config.defaultValue !== undefined
579+
? { value: config.defaultValue }
580+
: undefined,
578581
deprecationReason: null,
579582
extensions: Object.create(null),
580583
astNode: undefined,
@@ -622,7 +625,10 @@ describe('Type predicates', () => {
622625
name: 'someInputField',
623626
type: config.type,
624627
description: undefined,
625-
defaultValue: config.defaultValue,
628+
defaultValue:
629+
config.defaultValue !== undefined
630+
? { value: config.defaultValue }
631+
: undefined,
626632
deprecationReason: null,
627633
extensions: Object.create(null),
628634
astNode: undefined,

src/type/definition.ts

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { toObjMap } from '../jsutils/toObjMap.js';
1616
import { GraphQLError } from '../error/GraphQLError.js';
1717

1818
import type {
19+
ConstValueNode,
1920
EnumTypeDefinitionNode,
2021
EnumTypeExtensionNode,
2122
EnumValueDefinitionNode,
@@ -799,7 +800,7 @@ export function defineArguments(
799800
name: assertName(argName),
800801
description: argConfig.description,
801802
type: argConfig.type,
802-
defaultValue: argConfig.defaultValue,
803+
defaultValue: defineDefaultValue(argName, argConfig),
803804
deprecationReason: argConfig.deprecationReason,
804805
extensions: toObjMap(argConfig.extensions),
805806
astNode: argConfig.astNode,
@@ -833,7 +834,8 @@ export function argsToArgsConfig(
833834
(arg) => ({
834835
description: arg.description,
835836
type: arg.type,
836-
defaultValue: arg.defaultValue,
837+
defaultValue: arg.defaultValue?.value,
838+
defaultValueLiteral: arg.defaultValue?.literal,
837839
deprecationReason: arg.deprecationReason,
838840
extensions: arg.extensions,
839841
astNode: arg.astNode,
@@ -946,6 +948,7 @@ export interface GraphQLArgumentConfig {
946948
description?: Maybe<string>;
947949
type: GraphQLInputType;
948950
defaultValue?: unknown;
951+
defaultValueLiteral?: ConstValueNode | undefined;
949952
deprecationReason?: Maybe<string>;
950953
extensions?: Maybe<Readonly<GraphQLArgumentExtensions>>;
951954
astNode?: Maybe<InputValueDefinitionNode>;
@@ -971,7 +974,7 @@ export interface GraphQLArgument {
971974
name: string;
972975
description: Maybe<string>;
973976
type: GraphQLInputType;
974-
defaultValue: unknown;
977+
defaultValue: GraphQLDefaultValueUsage | undefined;
975978
deprecationReason: Maybe<string>;
976979
extensions: Readonly<GraphQLArgumentExtensions>;
977980
astNode: Maybe<InputValueDefinitionNode>;
@@ -985,6 +988,26 @@ export type GraphQLFieldMap<TSource, TContext> = ObjMap<
985988
GraphQLField<TSource, TContext>
986989
>;
987990

991+
export type GraphQLDefaultValueUsage =
992+
| { value: unknown; literal?: never }
993+
| { literal: ConstValueNode; value?: never };
994+
995+
export function defineDefaultValue(
996+
argName: string,
997+
config: GraphQLArgumentConfig | GraphQLInputFieldConfig,
998+
): GraphQLDefaultValueUsage | undefined {
999+
if (config.defaultValue === undefined && !config.defaultValueLiteral) {
1000+
return;
1001+
}
1002+
devAssert(
1003+
!(config.defaultValue !== undefined && config.defaultValueLiteral),
1004+
`Argument "${argName}" has both a defaultValue and a defaultValueLiteral property, but only one must be provided.`,
1005+
);
1006+
return config.defaultValueLiteral
1007+
? { literal: config.defaultValueLiteral }
1008+
: { value: config.defaultValue };
1009+
}
1010+
9881011
/**
9891012
* Custom extensions
9901013
*
@@ -1540,7 +1563,8 @@ export class GraphQLInputObjectType {
15401563
const fields = mapValue(this.getFields(), (field) => ({
15411564
description: field.description,
15421565
type: field.type,
1543-
defaultValue: field.defaultValue,
1566+
defaultValue: field.defaultValue?.value,
1567+
defaultValueLiteral: field.defaultValue?.literal,
15441568
deprecationReason: field.deprecationReason,
15451569
extensions: field.extensions,
15461570
astNode: field.astNode,
@@ -1574,7 +1598,7 @@ function defineInputFieldMap(
15741598
name: assertName(fieldName),
15751599
description: fieldConfig.description,
15761600
type: fieldConfig.type,
1577-
defaultValue: fieldConfig.defaultValue,
1601+
defaultValue: defineDefaultValue(fieldName, fieldConfig),
15781602
deprecationReason: fieldConfig.deprecationReason,
15791603
extensions: toObjMap(fieldConfig.extensions),
15801604
astNode: fieldConfig.astNode,
@@ -1615,6 +1639,7 @@ export interface GraphQLInputFieldConfig {
16151639
description?: Maybe<string>;
16161640
type: GraphQLInputType;
16171641
defaultValue?: unknown;
1642+
defaultValueLiteral?: ConstValueNode | undefined;
16181643
deprecationReason?: Maybe<string>;
16191644
extensions?: Maybe<Readonly<GraphQLInputFieldExtensions>>;
16201645
astNode?: Maybe<InputValueDefinitionNode>;
@@ -1626,7 +1651,7 @@ export interface GraphQLInputField {
16261651
name: string;
16271652
description: Maybe<string>;
16281653
type: GraphQLInputType;
1629-
defaultValue: unknown;
1654+
defaultValue: GraphQLDefaultValueUsage | undefined;
16301655
deprecationReason: Maybe<string>;
16311656
extensions: Readonly<GraphQLInputFieldExtensions>;
16321657
astNode: Maybe<InputValueDefinitionNode>;

src/type/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ export type {
119119
GraphQLScalarSerializer,
120120
GraphQLScalarValueParser,
121121
GraphQLScalarLiteralParser,
122+
GraphQLDefaultValueUsage,
122123
} from './definition.js';
123124

124125
export {

src/type/introspection.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -406,8 +406,13 @@ export const __InputValue: GraphQLObjectType = new GraphQLObjectType({
406406
'A GraphQL-formatted string representing the default value for this input value.',
407407
resolve(inputValue) {
408408
const { type, defaultValue } = inputValue;
409-
const valueAST = astFromValue(defaultValue, type);
410-
return valueAST ? print(valueAST) : null;
409+
if (!defaultValue) {
410+
return null;
411+
}
412+
const literal =
413+
defaultValue.literal ?? astFromValue(defaultValue.value, type);
414+
invariant(literal != null, 'Invalid default value');
415+
return print(literal);
411416
},
412417
},
413418
isDeprecated: {

src/utilities/TypeInfo.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { getEnterLeaveForKind } from '../language/visitor.js';
1414
import type {
1515
GraphQLArgument,
1616
GraphQLCompositeType,
17+
GraphQLDefaultValueUsage,
1718
GraphQLEnumValue,
1819
GraphQLField,
1920
GraphQLInputField,
@@ -53,7 +54,7 @@ export class TypeInfo {
5354
private _parentTypeStack: Array<Maybe<GraphQLCompositeType>>;
5455
private _inputTypeStack: Array<Maybe<GraphQLInputType>>;
5556
private _fieldDefStack: Array<Maybe<GraphQLField<unknown, unknown>>>;
56-
private _defaultValueStack: Array<Maybe<unknown>>;
57+
private _defaultValueStack: Array<GraphQLDefaultValueUsage | undefined>;
5758
private _directive: Maybe<GraphQLDirective>;
5859
private _argument: Maybe<GraphQLArgument>;
5960
private _enumValue: Maybe<GraphQLEnumValue>;
@@ -124,7 +125,7 @@ export class TypeInfo {
124125
return this._fieldDefStack.at(-1);
125126
}
126127

127-
getDefaultValue(): Maybe<unknown> {
128+
getDefaultValue(): GraphQLDefaultValueUsage | undefined {
128129
return this._defaultValueStack.at(-1);
129130
}
130131

src/utilities/__tests__/buildClientSchema-test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,7 @@ describe('Type System: build schema from introspection', () => {
439439
}
440440
441441
type Query {
442+
defaultID(intArg: ID = "123"): String
442443
defaultInt(intArg: Int = 30): String
443444
defaultList(listArg: [Int] = [1, 2, 3]): String
444445
defaultObject(objArg: Geo = { lat: 37.485, lon: -122.148 }): String
@@ -609,6 +610,28 @@ describe('Type System: build schema from introspection', () => {
609610
expect(result.data).to.deep.equal({ foo: 'bar' });
610611
});
611612

613+
it('can use client schema for execution if resolvers are added', () => {
614+
const schema = buildSchema(`
615+
type Query {
616+
foo(bar: String = "abc"): String
617+
}
618+
`);
619+
620+
const introspection = introspectionFromSchema(schema);
621+
const clientSchema = buildClientSchema(introspection);
622+
623+
const QueryType: GraphQLObjectType = clientSchema.getType('Query') as any;
624+
QueryType.getFields().foo.resolve = (_value, args) => args.bar;
625+
626+
const result = graphqlSync({
627+
schema: clientSchema,
628+
source: '{ foo }',
629+
});
630+
631+
expect(result.data).to.deep.equal({ foo: 'abc' });
632+
expect(result.data).to.deep.equal({ foo: 'abc' });
633+
});
634+
612635
it('can build invalid schema', () => {
613636
const schema = buildSchema('type Query', { assumeValid: true });
614637

0 commit comments

Comments
 (0)