Skip to content

Commit

Permalink
ease upgrade path for programmatic default values (#4296)
Browse files Browse the repository at this point in the history
#3814 added default value validation and coercion, deprecating `astFromValue()` (which was unsafe, used `serialize()` to uncoerce input values provided in the internal format) and replacing it with a call to `valueToLiteral()` which safely operates on external values.

This PR makes that change backwards compatible by reintroducing it as a new config option of `default` instead of replacing the existing option of `defaultValue`, where the type of `default` is:

```ts
export type GraphQLDefaultInput =
  | { value: unknown; literal?: never }
  | { literal: ConstValueNode; value?: never };
```

`default.value` is the external default value, while old config option of `defaultValue` is the internal value. Instead of removing it in v17, it is deprecated, and will be removed in v18. 

Side note: `GraphqlDefaultInput` renamed from `GraphQlDefaultValueUsage` introduced within the #3814 PR stack.

Co-authored-by: Michael Hayes <[email protected]>
  • Loading branch information
yaacovCR and hayes authored Dec 17, 2024
1 parent 5303cf7 commit 2a1786c
Show file tree
Hide file tree
Showing 31 changed files with 315 additions and 265 deletions.
2 changes: 1 addition & 1 deletion integrationTests/ts/basic-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const queryType: GraphQLObjectType = new GraphQLObjectType({
args: {
who: {
type: GraphQLString,
defaultValue: 'World',
default: { value: 'World' },
},
},
resolve(_root, args: { who: string }) {
Expand Down
2 changes: 1 addition & 1 deletion integrationTests/ts/esm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const queryType: GraphQLObjectType = new GraphQLObjectType({
args: {
who: {
type: GraphQLString,
defaultValue: 'World',
default: { value: 'World' },
},
},
resolve(_root, args: { who: string }) {
Expand Down
2 changes: 1 addition & 1 deletion src/execution/__tests__/executor-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ describe('Execute: Handles basic execution tasks', () => {
signature: {
name: 'var',
type: GraphQLString,
defaultValue: undefined,
default: undefined,
},
value: 'abc',
},
Expand Down
6 changes: 3 additions & 3 deletions src/execution/__tests__/variables-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,11 @@ const TestType = new GraphQLObjectType({
}),
fieldWithDefaultArgumentValue: fieldWithInputArg({
type: GraphQLString,
defaultValue: 'Hello World',
default: { value: 'Hello World' },
}),
fieldWithNonNullableStringInputAndDefaultArgumentValue: fieldWithInputArg({
type: new GraphQLNonNull(GraphQLString),
defaultValue: 'Hello World',
default: { value: 'Hello World' },
}),
fieldWithNestedInputObject: fieldWithInputArg({
type: TestNestedInputObject,
Expand Down Expand Up @@ -187,7 +187,7 @@ const schema = new GraphQLSchema({
type: new GraphQLNonNull(GraphQLBoolean),
description: 'Skipped when true.',
// default values will override operation variables in the setting of defined fragment variables that are not provided
defaultValue: true,
default: { value: true },
},
},
}),
Expand Down
16 changes: 8 additions & 8 deletions src/execution/getVariableSignature.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import { GraphQLError } from '../error/GraphQLError.js';

import type { VariableDefinitionNode } from '../language/ast.js';
import type {
ConstValueNode,
VariableDefinitionNode,
} from '../language/ast.js';
import { print } from '../language/printer.js';

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

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

Expand All @@ -21,7 +20,8 @@ import { typeFromAST } from '../utilities/typeFromAST.js';
export interface GraphQLVariableSignature {
name: string;
type: GraphQLInputType;
defaultValue: GraphQLDefaultValueUsage | undefined;
defaultValue?: never;
default: { literal: ConstValueNode } | undefined;
}

export function getVariableSignature(
Expand All @@ -46,6 +46,6 @@ export function getVariableSignature(
return {
name: varName,
type: varType,
defaultValue: defaultValue ? { literal: defaultValue } : undefined,
default: defaultValue && { literal: defaultValue },
};
}
16 changes: 6 additions & 10 deletions src/execution/values.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,9 @@ export function experimentalGetArgumentValues(
{ nodes: node },
);
}
if (argDef.defaultValue) {
coercedValues[name] = coerceDefaultValue(
argDef.defaultValue,
argDef.type,
);
const coercedDefaultValue = coerceDefaultValue(argDef);
if (coercedDefaultValue !== undefined) {
coercedValues[name] = coercedDefaultValue;
}
continue;
}
Expand All @@ -254,11 +252,9 @@ export function experimentalGetArgumentValues(
!Object.hasOwn(scopedVariableValues.coerced, variableName)) &&
!isRequiredArgument(argDef)
) {
if (argDef.defaultValue) {
coercedValues[name] = coerceDefaultValue(
argDef.defaultValue,
argDef.type,
);
const coercedDefaultValue = coerceDefaultValue(argDef);
if (coercedDefaultValue !== undefined) {
coercedValues[name] = coercedDefaultValue;
}
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ export type {
GraphQLScalarOutputValueCoercer,
GraphQLScalarInputValueCoercer,
GraphQLScalarInputLiteralCoercer,
GraphQLDefaultValueUsage,
GraphQLDefaultInput,
} from './type/index.js';

// Parse and operate on GraphQL language source files.
Expand Down
39 changes: 14 additions & 25 deletions src/type/__tests__/definition-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@ describe('Type System: Objects', () => {
input: {
description: 'Argument description.',
type: ScalarType,
defaultValue: 'DefaultValue',
defaultValueLiteral: undefined,
defaultValue: undefined,
default: { value: 'DefaultValue' },
deprecationReason: 'Argument deprecation reason.',
extensions: { someExtension: 'extension' },
astNode: dummyAny,
Expand Down Expand Up @@ -377,6 +377,7 @@ describe('Type System: Objects', () => {
description: undefined,
type: ScalarType,
defaultValue: undefined,
default: undefined,
deprecationReason: undefined,
extensions: {},
astNode: undefined,
Expand Down Expand Up @@ -493,7 +494,7 @@ describe('Type System: Interfaces', () => {
description: 'Argument description.',
type: ScalarType,
defaultValue: undefined,
defaultValueLiteral: dummyAny,
default: { literal: dummyAny },
deprecationReason: 'Argument deprecation reason.',
extensions: { someExtension: 'extension' },
astNode: dummyAny,
Expand Down Expand Up @@ -830,8 +831,8 @@ describe('Type System: Input Objects', () => {
input: {
description: 'Argument description.',
type: ScalarType,
defaultValue: 'DefaultValue',
defaultValueLiteral: undefined,
defaultValue: undefined,
default: { value: 'DefaultValue' },
deprecationReason: 'Argument deprecation reason.',
extensions: { someExtension: 'extension' },
astNode: dummyAny,
Expand Down Expand Up @@ -860,6 +861,7 @@ describe('Type System: Input Objects', () => {
description: undefined,
type: ScalarType,
defaultValue: undefined,
default: undefined,
deprecationReason: undefined,
extensions: {},
astNode: undefined,
Expand All @@ -879,6 +881,7 @@ describe('Type System: Input Objects', () => {
description: undefined,
type: ScalarType,
defaultValue: undefined,
default: undefined,
extensions: {},
deprecationReason: undefined,
astNode: undefined,
Expand Down Expand Up @@ -927,14 +930,15 @@ describe('Type System: Input Objects', () => {
const inputObjType = new GraphQLInputObjectType({
name: 'SomeInputObject',
fields: {
f: { type: ScalarType, defaultValue: 3 },
f: { type: ScalarType, default: { value: 3 } },
},
});
expect(inputObjType.getFields().f).to.deep.include({
name: 'f',
description: undefined,
type: ScalarType,
defaultValue: { value: 3 },
defaultValue: undefined,
default: { value: 3 },
deprecationReason: undefined,
extensions: {},
astNode: undefined,
Expand All @@ -947,36 +951,21 @@ describe('Type System: Input Objects', () => {
fields: {
f: {
type: ScalarType,
defaultValueLiteral: { kind: Kind.INT, value: '3' },
default: { literal: { kind: Kind.INT, value: '3' } },
},
},
});
expect(inputObjType.getFields().f).to.deep.include({
name: 'f',
description: undefined,
type: ScalarType,
defaultValue: { literal: { kind: 'IntValue', value: '3' } },
defaultValue: undefined,
default: { 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.',
);
});
});
});

Expand Down
2 changes: 2 additions & 0 deletions src/type/__tests__/directive-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ describe('Type System: Directive', () => {
description: undefined,
type: GraphQLString,
defaultValue: undefined,
default: undefined,
deprecationReason: undefined,
extensions: {},
astNode: undefined,
Expand All @@ -56,6 +57,7 @@ describe('Type System: Directive', () => {
description: undefined,
type: GraphQLInt,
defaultValue: undefined,
default: undefined,
deprecationReason: undefined,
extensions: {},
astNode: undefined,
Expand Down
28 changes: 27 additions & 1 deletion src/type/__tests__/enumType-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ const QueryType = new GraphQLObjectType({
args: {
fromEnum: {
type: ComplexEnum,
defaultValue: 'ONE',
default: { value: 'ONE' },
},
provideGoodValue: { type: GraphQLBoolean },
provideBadValue: { type: GraphQLBoolean },
Expand All @@ -90,6 +90,18 @@ const QueryType = new GraphQLObjectType({
return fromEnum;
},
},
complexEnumWithLegacyDefault: {
type: ComplexEnum,
args: {
fromEnum: {
type: ComplexEnum,
defaultValue: Complex1,
},
},
resolve(_source, { fromEnum }) {
return fromEnum;
},
},
thunkValuesString: {
type: GraphQLString,
args: {
Expand Down Expand Up @@ -442,6 +454,20 @@ describe('Type System: Enum Values', () => {
});
});

it('may be internally represented with complex values using legacy internal defaults', () => {
const result = executeQuery(`
{
complexEnumWithLegacyDefault
}
`);

expectJSON(result).toDeepEqual({
data: {
complexEnumWithLegacyDefault: 'ONE',
},
});
});

it('may have values specified via a callback', () => {
const result = executeQuery('{ thunkValuesString(fromEnum: B) }');

Expand Down
13 changes: 7 additions & 6 deletions src/type/__tests__/predicate-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { DirectiveLocation } from '../../language/directiveLocation.js';

import type {
GraphQLArgument,
GraphQLDefaultInput,
GraphQLInputField,
GraphQLInputType,
} from '../definition.js';
Expand Down Expand Up @@ -634,7 +635,7 @@ describe('Type predicates', () => {
describe('isRequiredArgument', () => {
function buildArg(config: {
type: GraphQLInputType;
defaultValue?: unknown;
default?: GraphQLDefaultInput;
}): GraphQLArgument {
const objectType = new GraphQLObjectType({
name: 'SomeType',
Expand All @@ -660,7 +661,7 @@ describe('Type predicates', () => {

const optArg2 = buildArg({
type: GraphQLString,
defaultValue: null,
default: { value: null },
});
expect(isRequiredArgument(optArg2)).to.equal(false);

Expand All @@ -671,7 +672,7 @@ describe('Type predicates', () => {

const optArg4 = buildArg({
type: new GraphQLNonNull(GraphQLString),
defaultValue: 'default',
default: { value: 'default' },
});
expect(isRequiredArgument(optArg4)).to.equal(false);
});
Expand All @@ -680,7 +681,7 @@ describe('Type predicates', () => {
describe('isRequiredInputField', () => {
function buildInputField(config: {
type: GraphQLInputType;
defaultValue?: unknown;
default?: GraphQLDefaultInput;
}): GraphQLInputField {
const inputObjectType = new GraphQLInputObjectType({
name: 'SomeType',
Expand All @@ -706,7 +707,7 @@ describe('Type predicates', () => {

const optField2 = buildInputField({
type: GraphQLString,
defaultValue: null,
default: { value: null },
});
expect(isRequiredInputField(optField2)).to.equal(false);

Expand All @@ -717,7 +718,7 @@ describe('Type predicates', () => {

const optField4 = buildInputField({
type: new GraphQLNonNull(GraphQLString),
defaultValue: 'default',
default: { value: 'default' },
});
expect(isRequiredInputField(optField4)).to.equal(false);
});
Expand Down
Loading

0 comments on commit 2a1786c

Please sign in to comment.