From d60813ebaf5aaaaa092008ccb1840876854a2589 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 4 Jun 2025 12:39:06 +0300 Subject: [PATCH 1/3] Use no-op for production instanceOf Re-implements #4386 with a configurable function variable initialized to a no-op function in production builds. This is apparently optimizable by v8, as it has no performance hit when setEnv('development') is not called. This should also allow for tree-shaking in production builds, as the development instanceOf function should not be included within the production bundle if setEnv is not called, although this has not been confirmed. To avoid a performance hit, setEnv('development') calls setInstanceOfCheckForEnv('development') which sets the function variable to developmentInstanceOfCheck. Setting a property on globalThis was tested, but reading the property led to a small performance hit. Co-Authored-By: Yaacov Rydzinski --- src/index.ts | 2 + src/jsutils/__tests__/instanceOf-test.ts | 55 +++++++++++++----- src/jsutils/instanceOf.ts | 72 +++++++++++++----------- src/setEnv.ts | 7 +++ 4 files changed, 90 insertions(+), 46 deletions(-) create mode 100644 src/setEnv.ts diff --git a/src/index.ts b/src/index.ts index 7bba9d8eee..e2ac15f444 100644 --- a/src/index.ts +++ b/src/index.ts @@ -28,6 +28,8 @@ // The GraphQL.js version info. export { version, versionInfo } from './version.js'; +export { setEnv } from './setEnv.js'; +export type { Env } from './setEnv.js'; // The primary entry point into fulfilling a GraphQL request. export type { GraphQLArgs } from './graphql.js'; diff --git a/src/jsutils/__tests__/instanceOf-test.ts b/src/jsutils/__tests__/instanceOf-test.ts index 5a54a641e5..498946dced 100644 --- a/src/jsutils/__tests__/instanceOf-test.ts +++ b/src/jsutils/__tests__/instanceOf-test.ts @@ -1,9 +1,25 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; +import { setEnv } from '../../setEnv.js'; + import { instanceOf } from '../instanceOf.js'; describe('instanceOf', () => { + function oldVersion() { + class Foo {} + return Foo; + } + + function newVersion() { + class Foo { + get [Symbol.toStringTag]() { + return 'Foo'; + } + } + return Foo; + } + it('do not throw on values without prototype', () => { class Foo { get [Symbol.toStringTag]() { @@ -16,28 +32,39 @@ describe('instanceOf', () => { expect(instanceOf(Object.create(null), Foo)).to.equal(false); }); - it('detect name clashes with older versions of this lib', () => { - function oldVersion() { - class Foo {} - return Foo; - } + it('ignore name clashes with older versions of this lib by default', () => { + const NewClass = newVersion(); + const OldClass = oldVersion(); + expect(instanceOf(new NewClass(), NewClass)).to.equal(true); + expect(instanceOf(new OldClass(), NewClass)).to.equal(false); + }); - function newVersion() { - class Foo { - get [Symbol.toStringTag]() { - return 'Foo'; - } - } - return Foo; - } + it('detect name clashes with older versions of this lib when set', () => { + setEnv('development'); + + const NewClass = newVersion(); + const OldClass = oldVersion(); + expect(instanceOf(new NewClass(), NewClass)).to.equal(true); + expect(() => instanceOf(new OldClass(), NewClass)).to.throw(); + }); + + it('ignore name clashes with older versions of this lib when reset', () => { + setEnv('development'); const NewClass = newVersion(); const OldClass = oldVersion(); expect(instanceOf(new NewClass(), NewClass)).to.equal(true); expect(() => instanceOf(new OldClass(), NewClass)).to.throw(); + + setEnv('production'); + + expect(instanceOf(new NewClass(), NewClass)).to.equal(true); + expect(instanceOf(new OldClass(), NewClass)).to.equal(false); }); it('allows instances to have share the same constructor name', () => { + setEnv('development'); + function getMinifiedClass(tag: string) { class SomeNameAfterMinification { get [Symbol.toStringTag]() { @@ -58,6 +85,8 @@ describe('instanceOf', () => { }); it('fails with descriptive error message', () => { + setEnv('development'); + function getFoo() { class Foo { get [Symbol.toStringTag]() { diff --git a/src/jsutils/instanceOf.ts b/src/jsutils/instanceOf.ts index 66811433ae..f0e155fac1 100644 --- a/src/jsutils/instanceOf.ts +++ b/src/jsutils/instanceOf.ts @@ -1,10 +1,12 @@ +import type { Env } from '../setEnv.js'; + import { inspect } from './inspect.js'; -/* c8 ignore next 3 */ -const isProduction = - globalThis.process != null && - // eslint-disable-next-line no-undef - process.env.NODE_ENV === 'production'; +const noOp = (_value: unknown, _constructor: Constructor): void => { + /* no-op */ +}; + +let check = noOp; /** * A replacement for instanceof which includes an error warning when multi-realm @@ -12,29 +14,30 @@ const isProduction = * See: https://expressjs.com/en/advanced/best-practice-performance.html#set-node_env-to-production * See: https://webpack.js.org/guides/production/ */ -export const instanceOf: (value: unknown, constructor: Constructor) => boolean = - /* c8 ignore next 6 */ - // FIXME: https://github.com/graphql/graphql-js/issues/2317 - isProduction - ? function instanceOf(value: unknown, constructor: Constructor): boolean { - return value instanceof constructor; - } - : function instanceOf(value: unknown, constructor: Constructor): boolean { - if (value instanceof constructor) { - return true; - } - if (typeof value === 'object' && value !== null) { - // Prefer Symbol.toStringTag since it is immune to minification. - const className = constructor.prototype[Symbol.toStringTag]; - const valueClassName = - // We still need to support constructor's name to detect conflicts with older versions of this library. - Symbol.toStringTag in value - ? value[Symbol.toStringTag] - : value.constructor?.name; - if (className === valueClassName) { - const stringifiedValue = inspect(value); - throw new Error( - `Cannot use ${className} "${stringifiedValue}" from another module or realm. +export function instanceOf(value: unknown, constructor: Constructor): boolean { + if (value instanceof constructor) { + return true; + } + check(value, constructor); + return false; +} + +function developmentInstanceOfCheck( + value: unknown, + constructor: Constructor, +): void { + if (typeof value === 'object' && value !== null) { + // Prefer Symbol.toStringTag since it is immune to minification. + const className = constructor.prototype[Symbol.toStringTag]; + const valueClassName = + // We still need to support constructor's name to detect conflicts with older versions of this library. + Symbol.toStringTag in value + ? value[Symbol.toStringTag] + : value.constructor?.name; + if (className === valueClassName) { + const stringifiedValue = inspect(value); + throw new Error( + `Cannot use ${className} "${stringifiedValue}" from another module or realm. Ensure that there is only one instance of "graphql" in the node_modules directory. If different versions of "graphql" are the dependencies of other @@ -46,11 +49,14 @@ Duplicate "graphql" modules cannot be used at the same time since different versions may have different capabilities and behavior. The data from one version used in the function from another could produce confusing and spurious results.`, - ); - } - } - return false; - }; + ); + } + } +} + +export function setInstanceOfCheckForEnv(newEnv: Env): void { + check = newEnv === 'development' ? developmentInstanceOfCheck : noOp; +} interface Constructor { prototype: { diff --git a/src/setEnv.ts b/src/setEnv.ts new file mode 100644 index 0000000000..390df06f97 --- /dev/null +++ b/src/setEnv.ts @@ -0,0 +1,7 @@ +import { setInstanceOfCheckForEnv } from './jsutils/instanceOf.js'; + +export type Env = 'production' | 'development'; + +export function setEnv(newEnv: Env): void { + setInstanceOfCheckForEnv(newEnv); +} From d18a8d934e1c54052d08746da94437db2689e15f Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 6 Jun 2025 12:00:24 +0300 Subject: [PATCH 2/3] add ability to check env --- src/__tests__/env-test.ts | 20 ++++++++++++++++++++ src/index.ts | 2 +- src/setEnv.ts | 7 +++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/env-test.ts diff --git a/src/__tests__/env-test.ts b/src/__tests__/env-test.ts new file mode 100644 index 0000000000..cfc27eaee3 --- /dev/null +++ b/src/__tests__/env-test.ts @@ -0,0 +1,20 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import { getEnv, setEnv } from '../setEnv.js'; + +describe('Env', () => { + it('should return undefined if environment is not set', () => { + expect(getEnv()).to.equal(undefined); + }); + + it('should set the environment to development', () => { + setEnv('development'); + expect(getEnv()).to.equal('development'); + }); + + it('should set the environment to production', () => { + setEnv('production'); + expect(getEnv()).to.equal('production'); + }); +}); diff --git a/src/index.ts b/src/index.ts index e2ac15f444..885bd13417 100644 --- a/src/index.ts +++ b/src/index.ts @@ -28,7 +28,7 @@ // The GraphQL.js version info. export { version, versionInfo } from './version.js'; -export { setEnv } from './setEnv.js'; +export { setEnv, getEnv } from './setEnv.js'; export type { Env } from './setEnv.js'; // The primary entry point into fulfilling a GraphQL request. diff --git a/src/setEnv.ts b/src/setEnv.ts index 390df06f97..e4277b0792 100644 --- a/src/setEnv.ts +++ b/src/setEnv.ts @@ -2,6 +2,13 @@ import { setInstanceOfCheckForEnv } from './jsutils/instanceOf.js'; export type Env = 'production' | 'development'; +let env: Env | undefined; + export function setEnv(newEnv: Env): void { + env = newEnv; setInstanceOfCheckForEnv(newEnv); } + +export function getEnv(): Env | undefined { + return env; +} From 27447396915c73a857f20c9e9dea6325b128a737 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 6 Jun 2025 12:43:16 +0300 Subject: [PATCH 3/3] defer name validation in production to validation step --- src/index.ts | 1 + src/language/__tests__/schema-parser-test.ts | 22 ++ src/setEnv.ts | 5 + src/type/__tests__/assertHasValidName-test.ts | 88 +++++ src/type/__tests__/definition-test.ts | 114 +++++- src/type/__tests__/directive-test.ts | 33 +- src/type/__tests__/validation-test.ts | 355 ++++++++++++++---- src/type/assertHasValidName.ts | 86 +++++ src/type/definition.ts | 48 ++- src/type/directives.ts | 15 +- src/type/index.ts | 1 + src/type/validate.ts | 47 +-- website/pages/upgrade-guides/v16-v17.mdx | 2 +- 13 files changed, 691 insertions(+), 126 deletions(-) create mode 100644 src/type/__tests__/assertHasValidName-test.ts create mode 100644 src/type/assertHasValidName.ts diff --git a/src/index.ts b/src/index.ts index 885bd13417..6bb71ab7c6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -153,6 +153,7 @@ export { validateSchema, assertValidSchema, // Upholds the spec rules about naming. + assertHasValidName, assertName, assertEnumValueName, } from './type/index.js'; diff --git a/src/language/__tests__/schema-parser-test.ts b/src/language/__tests__/schema-parser-test.ts index 3780c8330f..b893056315 100644 --- a/src/language/__tests__/schema-parser-test.ts +++ b/src/language/__tests__/schema-parser-test.ts @@ -109,6 +109,28 @@ describe('Schema Parser', () => { }); }); + it('parses type with reserved name', () => { + const doc = parse(dedent` + type __Reserved + `); + + expectJSON(doc).toDeepEqual({ + kind: 'Document', + definitions: [ + { + kind: 'ObjectTypeDefinition', + name: nameNode('__Reserved', { start: 5, end: 15 }), + description: undefined, + interfaces: undefined, + directives: undefined, + fields: undefined, + loc: { start: 0, end: 15 }, + }, + ], + loc: { start: 0, end: 15 }, + }); + }); + it('parses type with description string', () => { const doc = parse(dedent` "Description" diff --git a/src/setEnv.ts b/src/setEnv.ts index e4277b0792..0a51a6f959 100644 --- a/src/setEnv.ts +++ b/src/setEnv.ts @@ -1,5 +1,8 @@ import { setInstanceOfCheckForEnv } from './jsutils/instanceOf.js'; +import { setDefinitionNameCheckForEnv } from './type/definition.js'; +import { setDirectiveNameCheckForEnv } from './type/directives.js'; + export type Env = 'production' | 'development'; let env: Env | undefined; @@ -7,6 +10,8 @@ let env: Env | undefined; export function setEnv(newEnv: Env): void { env = newEnv; setInstanceOfCheckForEnv(newEnv); + setDefinitionNameCheckForEnv(newEnv); + setDirectiveNameCheckForEnv(newEnv); } export function getEnv(): Env | undefined { diff --git a/src/type/__tests__/assertHasValidName-test.ts b/src/type/__tests__/assertHasValidName-test.ts new file mode 100644 index 0000000000..714bd16b56 --- /dev/null +++ b/src/type/__tests__/assertHasValidName-test.ts @@ -0,0 +1,88 @@ +import { expect } from 'chai'; +import { beforeEach, describe, it } from 'mocha'; + +import { setEnv } from '../../setEnv.js'; + +import { assertHasValidName } from '../assertHasValidName.js'; +import type { GraphQLEnumValue } from '../index.js'; +import { __Type, GraphQLEnumType, GraphQLObjectType } from '../index.js'; + +describe(assertHasValidName.name, () => { + beforeEach(() => { + // In 'development', schema element construction will throw on an invalid name. + setEnv('production'); + }); + it('passthrough valid name', () => { + expect( + assertHasValidName( + new GraphQLObjectType({ + name: '_ValidName123', + fields: {}, + }), + ), + ).to.equal('_ValidName123'); + }); + + it('throws on empty strings', () => { + expect(() => + assertHasValidName( + new GraphQLObjectType({ + name: '', + fields: {}, + }), + ), + ).to.throw('Expected name of type "" to be a non-empty string.'); + }); + + it('throws for names with invalid characters', () => { + expect(() => + assertHasValidName( + new GraphQLObjectType({ + name: 'Some-Object', + fields: {}, + }), + ), + ).to.throw('Name of type "Some-Object" must only contain [_a-zA-Z0-9].'); + }); + + it('throws for names starting with invalid characters', () => { + expect(() => + assertHasValidName( + new GraphQLObjectType({ + name: '1_ObjectType', + fields: {}, + }), + ), + ).to.throw('Name of type "1_ObjectType" must start with [_a-zA-Z].'); + }); + + it('throws for reserved names', () => { + expect(() => + assertHasValidName( + new GraphQLObjectType({ + name: '__SomeObject', + fields: {}, + }), + ), + ).to.throw( + 'Name of type "__SomeObject" must not begin with "__", which is reserved by GraphQL introspection.', + ); + }); + + it('allows reserved names when specified', () => { + expect(assertHasValidName(__Type, true)).to.equal('__Type'); + }); + + it('throws for reserved names in enum values', () => { + const someEnum = new GraphQLEnumType({ + name: 'SomeEnum', + values: { + true: {}, + }, + }); + const value = someEnum.getValue('true') as GraphQLEnumValue; + expect(() => assertHasValidName(value)).to.throw( + 'Name "true" of enum value "SomeEnum.true" cannot be: true.', + ); + }); +}); diff --git a/src/type/__tests__/definition-test.ts b/src/type/__tests__/definition-test.ts index 8d9eeb4044..ae5c5be341 100644 --- a/src/type/__tests__/definition-test.ts +++ b/src/type/__tests__/definition-test.ts @@ -7,6 +7,8 @@ import { inspect } from '../../jsutils/inspect.js'; import { Kind } from '../../language/kinds.js'; import { parseConstValue } from '../../language/parser.js'; +import { setEnv } from '../../setEnv.js'; + import type { GraphQLEnumTypeConfig, GraphQLInputObjectTypeConfig, @@ -415,13 +417,22 @@ describe('Type System: Objects', () => { expect(() => objType.getFields()).to.not.throw(); }); - it('rejects an Object type with invalid name', () => { + it('rejects an Object type with invalid name in development', () => { + setEnv('development'); expect( () => new GraphQLObjectType({ name: 'bad-name', fields: {} }), ).to.throw('Names must only contain [_a-zA-Z0-9] but "bad-name" does not.'); }); - it('rejects an Object type with incorrectly named fields', () => { + it('accepts an Object type with invalid name in production', () => { + setEnv('production'); + expect( + () => new GraphQLObjectType({ name: 'bad-name', fields: {} }), + ).not.to.throw(); + }); + + it('rejects an Object type with incorrectly named fields in development', () => { + setEnv('development'); const objType = new GraphQLObjectType({ name: 'SomeObject', fields: { @@ -433,18 +444,19 @@ describe('Type System: Objects', () => { ); }); - it('rejects an Object type with a field function that returns incorrect type', () => { + it('accepts an Object type with incorrectly named fields in production', () => { + setEnv('production'); const objType = new GraphQLObjectType({ name: 'SomeObject', - // @ts-expect-error (Wrong type of return) - fields() { - return [{ field: ScalarType }]; + fields: { + 'bad-name': { type: ScalarType }, }, }); - expect(() => objType.getFields()).to.throw(); + expect(() => objType.getFields()).not.to.throw(); }); - it('rejects an Object type with incorrectly named field args', () => { + it('rejects an Object type with incorrectly named field args in development', () => { + setEnv('development'); const objType = new GraphQLObjectType({ name: 'SomeObject', fields: { @@ -460,6 +472,22 @@ describe('Type System: Objects', () => { 'Names must only contain [_a-zA-Z0-9] but "bad-name" does not.', ); }); + + it('accepts an Object type with incorrectly named field args in production', () => { + setEnv('production'); + const objType = new GraphQLObjectType({ + name: 'SomeObject', + fields: { + badField: { + type: ScalarType, + args: { + 'bad-name': { type: ScalarType }, + }, + }, + }, + }); + expect(() => objType.getFields()).not.to.throw(); + }); }); describe('Type System: Interfaces', () => { @@ -564,11 +592,19 @@ describe('Type System: Interfaces', () => { expect(implementing.getInterfaces()).to.deep.equal([InterfaceType]); }); - it('rejects an Interface type with invalid name', () => { + it('rejects an Interface type with invalid name in development', () => { + setEnv('development'); expect( () => new GraphQLInterfaceType({ name: 'bad-name', fields: {} }), ).to.throw('Names must only contain [_a-zA-Z0-9] but "bad-name" does not.'); }); + + it('accepts an Interface type with invalid name in production', () => { + setEnv('production'); + expect( + () => new GraphQLInterfaceType({ name: 'bad-name', fields: {} }), + ).not.to.throw(); + }); }); describe('Type System: Unions', () => { @@ -636,11 +672,19 @@ describe('Type System: Unions', () => { expect(unionType.getTypes()).to.deep.equal([]); }); - it('rejects an Union type with invalid name', () => { + it('rejects an Union type with invalid name in development', () => { + setEnv('development'); expect( () => new GraphQLUnionType({ name: 'bad-name', types: [] }), ).to.throw('Names must only contain [_a-zA-Z0-9] but "bad-name" does not.'); }); + + it('accepts an Union type with invalid name in production', () => { + setEnv('production'); + expect( + () => new GraphQLUnionType({ name: 'bad-name', types: [] }), + ).not.to.throw(); + }); }); describe('Type System: Enums', () => { @@ -787,13 +831,22 @@ describe('Type System: Enums', () => { expect(enumType.getValue('BAR')).has.property('value', 20); }); - it('rejects an Enum type with invalid name', () => { + it('rejects an Enum type with invalid name in development', () => { + setEnv('development'); expect( () => new GraphQLEnumType({ name: 'bad-name', values: {} }), ).to.throw('Names must only contain [_a-zA-Z0-9] but "bad-name" does not.'); }); - it('rejects an Enum type with incorrectly named values', () => { + it('rejects an Enum type with invalid name in production', () => { + setEnv('production'); + expect( + () => new GraphQLEnumType({ name: 'bad-name', values: {} }), + ).not.to.throw(); + }); + + it('rejects an Enum type with incorrectly named values in development', () => { + setEnv('development'); expect( () => new GraphQLEnumType({ @@ -804,6 +857,19 @@ describe('Type System: Enums', () => { }), ).to.throw('Names must only contain [_a-zA-Z0-9] but "bad-name" does not.'); }); + + it('accepts an Enum type with incorrectly named values in production', () => { + setEnv('production'); + expect( + () => + new GraphQLEnumType({ + name: 'SomeEnum', + values: { + 'bad-name': {}, + }, + }), + ).not.to.throw(); + }); }); describe('Type System: Input Objects', () => { @@ -888,7 +954,8 @@ describe('Type System: Input Objects', () => { }); }); - it('rejects an Input Object type with invalid name', () => { + it('rejects an Input Object type with invalid name in development', () => { + setEnv('development'); expect( () => new GraphQLInputObjectType({ name: 'bad-name', fields: {} }), ).to.throw( @@ -896,7 +963,15 @@ describe('Type System: Input Objects', () => { ); }); - it('rejects an Input Object type with incorrectly named fields', () => { + it('accepts an Input Object type with invalid name in production', () => { + setEnv('production'); + expect( + () => new GraphQLInputObjectType({ name: 'bad-name', fields: {} }), + ).not.to.throw(); + }); + + it('rejects an Input Object type with incorrectly named fields in development', () => { + setEnv('development'); const inputObjType = new GraphQLInputObjectType({ name: 'SomeInputObject', fields: { @@ -907,6 +982,17 @@ describe('Type System: Input Objects', () => { 'Names must only contain [_a-zA-Z0-9] but "bad-name" does not.', ); }); + + it('accepts an Input Object type with incorrectly named fields in production', () => { + setEnv('production'); + const inputObjType = new GraphQLInputObjectType({ + name: 'SomeInputObject', + fields: { + 'bad-name': { type: ScalarType }, + }, + }); + expect(() => inputObjType.getFields()).not.to.throw(); + }); }); it('Deprecation reason is preserved on fields', () => { diff --git a/src/type/__tests__/directive-test.ts b/src/type/__tests__/directive-test.ts index 90510bd0f9..12fb89b2ed 100644 --- a/src/type/__tests__/directive-test.ts +++ b/src/type/__tests__/directive-test.ts @@ -3,6 +3,8 @@ import { describe, it } from 'mocha'; import { DirectiveLocation } from '../../language/directiveLocation.js'; +import { setEnv } from '../../setEnv.js'; + import { GraphQLDirective } from '../directives.js'; import { GraphQLInt, GraphQLString } from '../scalars.js'; @@ -92,7 +94,8 @@ describe('Type System: Directive', () => { ); }); - it('rejects a directive with invalid name', () => { + it('rejects a directive with invalid name in development', () => { + setEnv('development'); expect( () => new GraphQLDirective({ @@ -102,7 +105,19 @@ describe('Type System: Directive', () => { ).to.throw('Names must only contain [_a-zA-Z0-9] but "bad-name" does not.'); }); - it('rejects a directive with incorrectly named arg', () => { + it('accepts a directive with invalid name in production', () => { + setEnv('production'); + expect( + () => + new GraphQLDirective({ + name: 'bad-name', + locations: [DirectiveLocation.QUERY], + }), + ).not.to.throw(); + }); + + it('rejects a directive with incorrectly named arg in development', () => { + setEnv('development'); expect( () => new GraphQLDirective({ @@ -114,4 +129,18 @@ describe('Type System: Directive', () => { }), ).to.throw('Names must only contain [_a-zA-Z0-9] but "bad-name" does not.'); }); + + it('accepts a directive with incorrectly named arg in production', () => { + setEnv('production'); + expect( + () => + new GraphQLDirective({ + name: 'Foo', + locations: [DirectiveLocation.QUERY], + args: { + 'bad-name': { type: GraphQLString }, + }, + }), + ).not.to.throw(); + }); }); diff --git a/src/type/__tests__/validation-test.ts b/src/type/__tests__/validation-test.ts index c1ed85d7b6..196f475a58 100644 --- a/src/type/__tests__/validation-test.ts +++ b/src/type/__tests__/validation-test.ts @@ -531,6 +531,284 @@ describe('Type System: Root types must all be different if provided', () => { }); }); +describe('Type System: Schema elements must be properly named', () => { + it('accepts types with valid names', () => { + const schema = buildSchema(` + directive @SomeDirective(someArg: String) on QUERY + + type Query { + f1: SomeObject + f2: SomeInterface + f3: SomeUnion + f4: SomeEnum + f5: SomeScalar + } + + input SomeInputObject { + someField: String + } + + type SomeObject { + someField(someArg: SomeInputObject): String + } + + interface SomeInterface { + someField: String + } + + type SomeUnionMember { + field: String + } + + union SomeUnion = SomeUnionMember + + enum SomeEnum { + SOME_ENUM_VALUE + } + + scalar SomeScalar + `); + + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('rejects types with invalid names', () => { + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + f1: { + type: new GraphQLObjectType({ + name: 'Some-Object', + fields: { + 'some-Field': { + type: GraphQLString, + args: { + 'some-Arg': { + type: new GraphQLInputObjectType({ + name: 'Some-Input-Object', + fields: { + 'some-Field': { type: GraphQLString }, + }, + }), + }, + }, + }, + }, + }), + }, + f2: { + type: new GraphQLInterfaceType({ + name: 'Some-Interface', + fields: { 'some-Field': { type: GraphQLString } }, + }), + }, + f3: { + type: new GraphQLUnionType({ + name: 'Some-Union', + types: [ + new GraphQLObjectType({ + name: 'SomeUnionMember', + fields: { field: { type: GraphQLString } }, + }), + ], + }), + }, + f4: { + type: new GraphQLEnumType({ + name: 'Some-Enum', + values: { 'SOME-ENUM-VALUE': {}, true: {}, false: {}, null: {} }, + }), + }, + f5: { + type: new GraphQLScalarType({ + name: 'Some-Scalar', + }), + }, + }, + }), + directives: [ + new GraphQLDirective({ + name: 'Some-Directive', + args: { + 'some-Arg': { + type: GraphQLString, + }, + }, + locations: [DirectiveLocation.QUERY], + }), + ], + }); + + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Name of directive "@Some-Directive" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name "some-Arg" of argument "@Some-Directive(some-Arg:)" must only contain [_a-zA-Z0-9].', + }, + { + message: 'Name of type "Some-Object" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name "some-Field" of field "Some-Object.some-Field" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name "some-Arg" of argument "Some-Object.some-Field(some-Arg:)" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name of type "Some-Input-Object" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name "some-Field" of input field "Some-Input-Object.some-Field" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name of type "Some-Interface" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name "some-Field" of field "Some-Interface.some-Field" must only contain [_a-zA-Z0-9].', + }, + { + message: 'Name of type "Some-Union" must only contain [_a-zA-Z0-9].', + }, + { + message: 'Name of type "Some-Enum" must only contain [_a-zA-Z0-9].', + }, + { + message: + 'Name "SOME-ENUM-VALUE" of enum value "Some-Enum.SOME-ENUM-VALUE" must only contain [_a-zA-Z0-9].', + }, + { + message: 'Name "true" of enum value "Some-Enum.true" cannot be: true.', + }, + { + message: + 'Name "false" of enum value "Some-Enum.false" cannot be: false.', + }, + { + message: 'Name "null" of enum value "Some-Enum.null" cannot be: null.', + }, + { + message: 'Name of type "Some-Scalar" must only contain [_a-zA-Z0-9].', + }, + ]); + }); + + it('rejects types with reserved names', () => { + const schema = buildSchema(` + directive @__SomeDirective(__someArg: String) on QUERY + + type Query { + f1: __SomeObject + f2: __SomeInterface + f3: __SomeUnion + f4: __SomeEnum + f5: __SomeScalar + } + + input __SomeInputObject { + __someField: String + } + + type __SomeObject { + __someField(__someArg: __SomeInputObject): String + } + + interface __SomeInterface { + __someField: String + } + + type SomeUnionMember { + field: String + } + + union __SomeUnion = SomeUnionMember + + enum __SomeEnum { + __SOME_ENUM_VALUE + } + + scalar __SomeScalar + `); + + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Name of directive "@__SomeDirective" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 2, column: 7 }], + }, + { + message: + 'Name "__someArg" of argument "@__SomeDirective(__someArg:)" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 2, column: 34 }], + }, + { + message: + 'Name of type "__SomeInputObject" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 12, column: 7 }], + }, + { + message: + 'Name "__someField" of input field "__SomeInputObject.__someField" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 13, column: 9 }], + }, + { + message: + 'Name of type "__SomeObject" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 16, column: 7 }], + }, + { + message: + 'Name "__someField" of field "__SomeObject.__someField" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 17, column: 9 }], + }, + { + message: + 'Name "__someArg" of argument "__SomeObject.__someField(__someArg:)" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 17, column: 21 }], + }, + { + message: + 'Name of type "__SomeInterface" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 20, column: 7 }], + }, + { + message: + 'Name "__someField" of field "__SomeInterface.__someField" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 21, column: 9 }], + }, + { + message: + 'Name of type "__SomeUnion" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 28, column: 7 }], + }, + { + message: + 'Name of type "__SomeEnum" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 30, column: 7 }], + }, + { + message: + 'Name "__SOME_ENUM_VALUE" of enum value "__SomeEnum.__SOME_ENUM_VALUE" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 31, column: 9 }], + }, + { + message: + 'Name of type "__SomeScalar" must not begin with "__", which is reserved by GraphQL introspection.', + locations: [{ line: 34, column: 7 }], + }, + ]); + }); +}); + describe('Type System: Objects must have fields', () => { it('accepts an Object type with fields object', () => { const schema = buildSchema(` @@ -586,65 +864,6 @@ describe('Type System: Objects must have fields', () => { }, ]); }); - - it('rejects an Object type with incorrectly named fields', () => { - const schema = schemaWithFieldType( - new GraphQLObjectType({ - name: 'SomeObject', - fields: { - __badName: { type: GraphQLString }, - }, - }), - ); - expectJSON(validateSchema(schema)).toDeepEqual([ - { - message: - 'Name "__badName" must not begin with "__", which is reserved by GraphQL introspection.', - }, - ]); - }); -}); - -describe('Type System: Fields args must be properly named', () => { - it('accepts field args with valid names', () => { - const schema = schemaWithFieldType( - new GraphQLObjectType({ - name: 'SomeObject', - fields: { - goodField: { - type: GraphQLString, - args: { - goodArg: { type: GraphQLString }, - }, - }, - }, - }), - ); - expectJSON(validateSchema(schema)).toDeepEqual([]); - }); - - it('rejects field arg with invalid names', () => { - const schema = schemaWithFieldType( - new GraphQLObjectType({ - name: 'SomeObject', - fields: { - badField: { - type: GraphQLString, - args: { - __badName: { type: GraphQLString }, - }, - }, - }, - }), - ); - - expectJSON(validateSchema(schema)).toDeepEqual([ - { - message: - 'Name "__badName" must not begin with "__", which is reserved by GraphQL introspection.', - }, - ]); - }); }); describe('Type System: Union types must be valid', () => { @@ -1376,24 +1595,6 @@ describe('Type System: Enum types must be well defined', () => { }, ]); }); - - it('rejects an Enum type with incorrectly named values', () => { - const schema = schemaWithFieldType( - new GraphQLEnumType({ - name: 'SomeEnum', - values: { - __badName: {}, - }, - }), - ); - - expectJSON(validateSchema(schema)).toDeepEqual([ - { - message: - 'Name "__badName" must not begin with "__", which is reserved by GraphQL introspection.', - }, - ]); - }); }); describe('Type System: Object fields must have output types', () => { diff --git a/src/type/assertHasValidName.ts b/src/type/assertHasValidName.ts new file mode 100644 index 0000000000..e42d876649 --- /dev/null +++ b/src/type/assertHasValidName.ts @@ -0,0 +1,86 @@ +import { inspect } from '../jsutils/inspect.js'; +import { invariant } from '../jsutils/invariant.js'; + +import { isNameContinue, isNameStart } from '../language/characterClasses.js'; + +import type { GraphQLSchemaElement } from './definition.js'; +import { + isArgument, + isEnumValue, + isField, + isInputField, + isNamedType, +} from './definition.js'; +import { isDirective } from './directives.js'; + +function getDescriptor( + schemaElement: GraphQLSchemaElement & { + readonly name: string; + }, +): string { + if (isNamedType(schemaElement)) { + return `of type "${schemaElement.name}"`; + } + if (isField(schemaElement)) { + return `"${schemaElement.name}" of field "${schemaElement}"`; + } + if (isArgument(schemaElement)) { + return `"${schemaElement.name}" of argument "${schemaElement}"`; + } + if (isInputField(schemaElement)) { + return `"${schemaElement.name}" of input field "${schemaElement}"`; + } + if (isEnumValue(schemaElement)) { + return `"${schemaElement.name}" of enum value "${schemaElement}"`; + } + if (isDirective(schemaElement)) { + return `of directive "${schemaElement}"`; + } + /* c8 ignore next 3 */ + // Not reachable, all possible inputs have been considered) + invariant(false, `Unexpected schema element: "${inspect(schemaElement)}".`); +} + +export function assertHasValidName( + schemaElement: GraphQLSchemaElement & { + readonly name: string; + }, + allowReservedNames = false, +): string { + const name = schemaElement.name; + if (name.length === 0) { + throw new Error( + `Expected name ${getDescriptor(schemaElement)} to be a non-empty string.`, + ); + } + + for (let i = 1; i < name.length; ++i) { + if (!isNameContinue(name.charCodeAt(i))) { + throw new Error( + `Name ${getDescriptor(schemaElement)} must only contain [_a-zA-Z0-9].`, + ); + } + } + + if (!isNameStart(name.charCodeAt(0))) { + throw new Error( + `Name ${getDescriptor(schemaElement)} must start with [_a-zA-Z].`, + ); + } + + if (!allowReservedNames && name.startsWith('__')) { + throw new Error( + `Name ${getDescriptor(schemaElement)} must not begin with "__", which is reserved by GraphQL introspection.`, + ); + } + + if (isEnumValue(schemaElement)) { + if (name === 'true' || name === 'false' || name === 'null') { + throw new Error( + `Name ${getDescriptor(schemaElement)} cannot be: ${name}.`, + ); + } + } + + return name; +} diff --git a/src/type/definition.ts b/src/type/definition.ts index ea96be5153..769673a31d 100644 --- a/src/type/definition.ts +++ b/src/type/definition.ts @@ -45,10 +45,28 @@ import type { VariableValues } from '../execution/values.js'; import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped.js'; +import type { Env } from '../setEnv.js'; + import { assertEnumValueName, assertName } from './assertName.js'; import type { GraphQLDirective } from './directives.js'; import type { GraphQLSchema } from './schema.js'; +const noOp = (_name: string): void => { + /* no-op */ +}; + +let nameCheck = noOp; +let enumValueNameCheck = noOp; + +export function setDefinitionNameCheckForEnv(newEnv: Env): void { + if (newEnv === 'development') { + nameCheck = assertName; + enumValueNameCheck = assertEnumValueName; + return; + } + nameCheck = enumValueNameCheck = noOp; +} + // Predicates & Assertions /** @@ -669,7 +687,8 @@ export class GraphQLScalarType extensionASTNodes: ReadonlyArray; constructor(config: Readonly>) { - this.name = assertName(config.name); + this.name = config.name; + nameCheck(this.name); this.description = config.description; this.specifiedByURL = config.specifiedByURL; this.serialize = @@ -880,7 +899,8 @@ export class GraphQLObjectType private _interfaces: ThunkReadonlyArray; constructor(config: Readonly>) { - this.name = assertName(config.name); + this.name = config.name; + nameCheck(this.name); this.description = config.description; this.isTypeOf = config.isTypeOf; this.extensions = toObjMapWithSymbols(config.extensions); @@ -1116,7 +1136,8 @@ export class GraphQLField config: GraphQLFieldConfig, ) { this.parentType = parentType; - this.name = assertName(name); + this.name = name; + nameCheck(this.name); this.description = config.description; this.type = config.type; @@ -1182,7 +1203,8 @@ export class GraphQLArgument implements GraphQLSchemaElement { config: GraphQLArgumentConfig, ) { this.parent = parent; - this.name = assertName(name); + this.name = name; + nameCheck(this.name); this.description = config.description; this.type = config.type; this.defaultValue = config.defaultValue; @@ -1281,7 +1303,8 @@ export class GraphQLInterfaceType private _interfaces: ThunkReadonlyArray; constructor(config: Readonly>) { - this.name = assertName(config.name); + this.name = config.name; + nameCheck(this.name); this.description = config.description; this.resolveType = config.resolveType; this.extensions = toObjMapWithSymbols(config.extensions); @@ -1407,7 +1430,8 @@ export class GraphQLUnionType implements GraphQLSchemaElement { private _types: ThunkReadonlyArray; constructor(config: Readonly>) { - this.name = assertName(config.name); + this.name = config.name; + nameCheck(this.name); this.description = config.description; this.resolveType = config.resolveType; this.extensions = toObjMapWithSymbols(config.extensions); @@ -1528,7 +1552,8 @@ export class GraphQLEnumType /* */ implements GraphQLSchemaElement { private _nameLookup: ObjMap | null; constructor(config: Readonly */>) { - this.name = assertName(config.name); + this.name = config.name; + nameCheck(this.name); this.description = config.description; this.extensions = toObjMapWithSymbols(config.extensions); this.astNode = config.astNode; @@ -1756,7 +1781,8 @@ export class GraphQLEnumValue implements GraphQLSchemaElement { config: GraphQLEnumValueConfig, ) { this.parentEnum = parentEnum; - this.name = assertEnumValueName(name); + this.name = name; + enumValueNameCheck(this.name); this.description = config.description; this.value = config.value !== undefined ? config.value : name; this.deprecationReason = config.deprecationReason; @@ -1832,7 +1858,8 @@ export class GraphQLInputObjectType implements GraphQLSchemaElement { private _fields: ThunkObjMap; constructor(config: Readonly) { - this.name = assertName(config.name); + this.name = config.name; + nameCheck(this.name); this.description = config.description; this.extensions = toObjMapWithSymbols(config.extensions); this.astNode = config.astNode; @@ -1960,7 +1987,8 @@ export class GraphQLInputField implements GraphQLSchemaElement { ); this.parentType = parentType; - this.name = assertName(name); + this.name = name; + nameCheck(this.name); this.description = config.description; this.type = config.type; this.defaultValue = config.defaultValue; diff --git a/src/type/directives.ts b/src/type/directives.ts index 96f5b6b65a..aec968cadb 100644 --- a/src/type/directives.ts +++ b/src/type/directives.ts @@ -10,6 +10,8 @@ import { toObjMapWithSymbols } from '../jsutils/toObjMap.js'; import type { DirectiveDefinitionNode } from '../language/ast.js'; import { DirectiveLocation } from '../language/directiveLocation.js'; +import type { Env } from '../setEnv.js'; + import { assertName } from './assertName.js'; import type { GraphQLArgumentConfig, @@ -19,6 +21,16 @@ import type { import { GraphQLArgument, GraphQLNonNull } from './definition.js'; import { GraphQLBoolean, GraphQLInt, GraphQLString } from './scalars.js'; +const noOp = (_name: string): void => { + /* no-op */ +}; + +let nameCheck = noOp; + +export function setDirectiveNameCheckForEnv(newEnv: Env): void { + nameCheck = newEnv === 'development' ? assertName : noOp; +} + /** * Test if the given value is a GraphQL directive. */ @@ -62,7 +74,8 @@ export class GraphQLDirective implements GraphQLSchemaElement { astNode: Maybe; constructor(config: Readonly) { - this.name = assertName(config.name); + this.name = config.name; + nameCheck(this.name); this.description = config.description; this.locations = config.locations; this.isRepeatable = config.isRepeatable ?? false; diff --git a/src/type/index.ts b/src/type/index.ts index dd9d103868..bbb75a9ef8 100644 --- a/src/type/index.ts +++ b/src/type/index.ts @@ -202,4 +202,5 @@ export { export { validateSchema, assertValidSchema } from './validate.js'; // Upholds the spec rules about naming. +export { assertHasValidName } from './assertHasValidName.js'; export { assertName, assertEnumValueName } from './assertName.js'; diff --git a/src/type/validate.ts b/src/type/validate.ts index ede99c8054..20ff83984f 100644 --- a/src/type/validate.ts +++ b/src/type/validate.ts @@ -34,6 +34,7 @@ import { validateInputValue, } from '../utilities/validateInputValue.js'; +import { assertHasValidName } from './assertHasValidName.js'; import type { GraphQLArgument, GraphQLEnumType, @@ -42,6 +43,7 @@ import type { GraphQLInputType, GraphQLInterfaceType, GraphQLObjectType, + GraphQLSchemaElement, GraphQLUnionType, } from './definition.js'; import { @@ -196,7 +198,7 @@ function validateDirectives(context: SchemaValidationContext): void { } // Ensure they are named correctly. - validateName(context, directive); + validateName(context, directive, false); if (directive.locations.length === 0) { context.reportError( @@ -208,7 +210,7 @@ function validateDirectives(context: SchemaValidationContext): void { // Ensure the arguments are valid. for (const arg of directive.args) { // Ensure they are named correctly. - validateName(context, arg); + validateName(context, arg, false); // Ensure the type is an input type. if (!isInputType(arg.type)) { @@ -348,14 +350,16 @@ function uncoerceDefaultValue(value: unknown, type: GraphQLInputType): unknown { function validateName( context: SchemaValidationContext, - node: { readonly name: string; readonly astNode: Maybe }, + schemaElement: GraphQLSchemaElement & { + readonly name: string; + readonly astNode: Maybe; + }, + allowReservedNames: boolean, ): void { - // Ensure names are valid, however introspection types opt out. - if (node.name.startsWith('__')) { - context.reportError( - `Name "${node.name}" must not begin with "__", which is reserved by GraphQL introspection.`, - node.astNode, - ); + try { + assertHasValidName(schemaElement, allowReservedNames); + } catch (error: unknown) { + context.reportError((error as Error).message, schemaElement.astNode); } } @@ -376,20 +380,18 @@ function validateTypes(context: SchemaValidationContext): void { continue; } - // Ensure it is named correctly (excluding introspection types). - if (!isIntrospectionType(type)) { - validateName(context, type); - } + const allowReservedNames = isIntrospectionType(type); + validateName(context, type, allowReservedNames); if (isObjectType(type)) { // Ensure fields are valid - validateFields(context, type); + validateFields(context, type, allowReservedNames); // Ensure objects implement the interfaces they claim to. validateInterfaces(context, type); } else if (isInterfaceType(type)) { // Ensure fields are valid. - validateFields(context, type); + validateFields(context, type, allowReservedNames); // Ensure interfaces implement the interfaces they claim to. validateInterfaces(context, type); @@ -398,10 +400,10 @@ function validateTypes(context: SchemaValidationContext): void { validateUnionMembers(context, type); } else if (isEnumType(type)) { // Ensure Enums have valid values. - validateEnumValues(context, type); + validateEnumValues(context, type, allowReservedNames); } else if (isInputObjectType(type)) { // Ensure Input Object fields are valid. - validateInputFields(context, type); + validateInputFields(context, type, allowReservedNames); // Ensure Input Objects do not contain invalid field circular references. // Ensure Input Objects do not contain non-nullable circular references. @@ -416,6 +418,7 @@ function validateTypes(context: SchemaValidationContext): void { function validateFields( context: SchemaValidationContext, type: GraphQLObjectType | GraphQLInterfaceType, + allowReservedNames: boolean, ): void { const fields = Object.values(type.getFields()); @@ -429,7 +432,7 @@ function validateFields( for (const field of fields) { // Ensure they are named correctly. - validateName(context, field); + validateName(context, field, allowReservedNames); // Ensure the type is an output type if (!isOutputType(field.type)) { @@ -443,7 +446,7 @@ function validateFields( // Ensure the arguments are valid for (const arg of field.args) { // Ensure they are named correctly. - validateName(context, arg); + validateName(context, arg, allowReservedNames); // Ensure the type is an input type if (!isInputType(arg.type)) { @@ -648,6 +651,7 @@ function validateUnionMembers( function validateEnumValues( context: SchemaValidationContext, enumType: GraphQLEnumType, + allowReservedNames: boolean, ): void { const enumValues = enumType.getValues(); @@ -660,13 +664,14 @@ function validateEnumValues( for (const enumValue of enumValues) { // Ensure valid name. - validateName(context, enumValue); + validateName(context, enumValue, allowReservedNames); } } function validateInputFields( context: SchemaValidationContext, inputObj: GraphQLInputObjectType, + allowReservedNames: boolean, ): void { const fields = Object.values(inputObj.getFields()); @@ -680,7 +685,7 @@ function validateInputFields( // Ensure the input fields are valid for (const field of fields) { // Ensure they are named correctly. - validateName(context, field); + validateName(context, field, allowReservedNames); // Ensure the type is an input type if (!isInputType(field.type)) { diff --git a/website/pages/upgrade-guides/v16-v17.mdx b/website/pages/upgrade-guides/v16-v17.mdx index 00b8a27343..00ab2b2662 100644 --- a/website/pages/upgrade-guides/v16-v17.mdx +++ b/website/pages/upgrade-guides/v16-v17.mdx @@ -145,7 +145,7 @@ Use the `validateInputValue` helper to retrieve the actual errors. - Removed deprecated `getOperationType` function, use `getRootType` on the `GraphQLSchema` instance instead - Removed deprecated `getVisitFn` function, use `getEnterLeaveForKind` instead - Removed deprecated `printError` and `formatError` utilities, you can use `toString` or `toJSON` on the error as a replacement -- Removed deprecated `assertValidName` and `isValidNameError` utilities, use `assertName` instead +- Removed deprecated `assertValidName` and `isValidNameError` utilities, use `assertHasValidName` instead - Removed deprecated `assertValidExecutionArguments` function, use `assertValidSchema` instead - Removed deprecated `getFieldDefFn` from `TypeInfo` - Removed deprecated `TypeInfo` from `validate` https://github.com/graphql/graphql-js/pull/4187