From e11b86d00b9f54d3ff969d9a089398a5d79f23df Mon Sep 17 00:00:00 2001 From: Christoph Zwerschke Date: Mon, 29 Apr 2019 16:13:12 +0200 Subject: [PATCH] Make isSpecifiedDirective and isSpecifiedScalar stricter These can now both be used as standalone tests. Also added some unit tests for these predicates. --- src/type/__tests__/predicate-test.js | 115 ++++++++++++++++++++------- src/type/directives.js | 13 +-- src/type/scalars.js | 4 +- 3 files changed, 98 insertions(+), 34 deletions(-) diff --git a/src/type/__tests__/predicate-test.js b/src/type/__tests__/predicate-test.js index 29c233c9e6..4c86241419 100644 --- a/src/type/__tests__/predicate-test.js +++ b/src/type/__tests__/predicate-test.js @@ -12,6 +12,11 @@ import { expect } from 'chai'; import { GraphQLScalarType, + GraphQLBoolean, + GraphQLID, + GraphQLInt, + GraphQLFloat, + GraphQLString, GraphQLEnumType, GraphQLInputObjectType, GraphQLInterfaceType, @@ -19,13 +24,13 @@ import { GraphQLUnionType, GraphQLList, GraphQLNonNull, - GraphQLString, GraphQLDirective, GraphQLIncludeDirective, GraphQLSkipDirective, GraphQLDeprecatedDirective, isType, isScalarType, + isSpecifiedScalarType, isObjectType, isInterfaceType, isUnionType, @@ -82,6 +87,10 @@ const ScalarType = new GraphQLScalarType({ name: 'Scalar', serialize() {}, }); +const Directive = new GraphQLDirective({ + name: 'Directive', + locations: ['QUERY'], +}); describe('Type predicates', () => { describe('isType', () => { @@ -119,6 +128,11 @@ describe('Type predicates', () => { expect(() => assertScalarType(ScalarType)).not.to.throw(); }); + it('returns false for scalar class (rather than instance)', () => { + expect(isScalarType(GraphQLScalarType)).to.equal(false); + expect(() => assertScalarType(GraphQLScalarType)).to.throw(); + }); + it('returns false for wrapped scalar', () => { expect(isScalarType(GraphQLList(ScalarType))).to.equal(false); expect(() => assertScalarType(GraphQLList(ScalarType))).to.throw(); @@ -127,6 +141,56 @@ describe('Type predicates', () => { it('returns false for non-scalar', () => { expect(isScalarType(EnumType)).to.equal(false); expect(() => assertScalarType(EnumType)).to.throw(); + expect(isScalarType(Directive)).to.equal(false); + expect(() => assertScalarType(Directive)).to.throw(); + }); + + it('returns false for random garbage', () => { + expect(isScalarType({ what: 'is this' })).to.equal(false); + expect(() => assertScalarType({ what: 'is this' })).to.throw(); + }); + }); + + describe('isSpecifiedScalarType', () => { + it('returns true for specified scalars', () => { + expect(isSpecifiedScalarType(GraphQLString)).to.equal(true); + expect(isSpecifiedScalarType(GraphQLInt)).to.equal(true); + expect(isSpecifiedScalarType(GraphQLFloat)).to.equal(true); + expect(isSpecifiedScalarType(GraphQLBoolean)).to.equal(true); + expect(isSpecifiedScalarType(GraphQLID)).to.equal(true); + }); + + it('returns false for custom scalar', () => { + expect(isSpecifiedScalarType(ScalarType)).to.equal(false); + }); + + it('returns false for scalar class (rather than specified instance)', () => { + expect(isSpecifiedScalarType(GraphQLScalarType)).to.equal(false); + }); + + it('returns false for wrapped specified scalar', () => { + expect(isSpecifiedScalarType(GraphQLList(GraphQLString))).to.equal(false); + }); + + it('returns false for non-scalar', () => { + expect(isSpecifiedScalarType(EnumType)).to.equal(false); + expect(isSpecifiedScalarType(Directive)).to.equal(false); + }); + + it('returns false for spec defined directive', () => { + expect(isSpecifiedScalarType(GraphQLSkipDirective)).to.equal(false); + }); + + it('returns false for object type named like specified scalar', () => { + const ObjectNamedLikeScalar = new GraphQLObjectType({ + name: 'String', + fields: { serialize: { type: GraphQLString } }, + }); + expect(isSpecifiedScalarType(ObjectNamedLikeScalar)).to.equal(false); + }); + + it('returns false for random garbage', () => { + expect(isSpecifiedScalarType({ what: 'is this' })).to.equal(false); }); }); @@ -593,30 +657,26 @@ describe('Type predicates', () => { describe('Directive predicates', () => { describe('isDirective', () => { - it('returns true for directives', () => { - const directive = new GraphQLDirective({ - name: 'Foo', - locations: ['QUERY'], - }); - expect(isDirective(directive)).to.equal(true); - expect(() => assertDirective(directive)).not.to.throw(); + it('returns true for spec defined directive', () => { expect(isDirective(GraphQLSkipDirective)).to.equal(true); expect(() => assertDirective(GraphQLSkipDirective)).not.to.throw(); }); + it('returns true for custom directive', () => { + expect(isDirective(Directive)).to.equal(true); + expect(() => assertDirective(Directive)).not.to.throw(); + }); + it('returns false for directive class (rather than instance)', () => { expect(isDirective(GraphQLDirective)).to.equal(false); expect(() => assertDirective(GraphQLDirective)).to.throw(); }); - it('returns false for object type', () => { - expect(isDirective(ObjectType)).to.equal(false); - expect(() => assertDirective(ObjectType)).to.throw(); - }); - - it('returns false for scalar type', () => { - expect(isDirective(GraphQLString)).to.equal(false); - expect(() => assertDirective(GraphQLString)).to.throw(); + it('returns false for non-directive', () => { + expect(isDirective(EnumType)).to.equal(false); + expect(() => assertDirective(EnumType)).to.throw(); + expect(isDirective(ScalarType)).to.equal(false); + expect(() => assertDirective(ScalarType)).to.throw(); }); it('returns false for random garbage', () => { @@ -632,30 +692,31 @@ describe('Directive predicates', () => { }); it('returns false for custom directive', () => { - const directive = new GraphQLDirective({ - name: 'Foo', - locations: ['QUERY'], - }); - expect(isSpecifiedDirective(directive)).to.equal(false); + expect(isSpecifiedDirective(Directive)).to.equal(false); }); it('returns false for directive class (rather than specified instance)', () => { - // $DisableFlowOnNegativeTest expect(isSpecifiedDirective(GraphQLDirective)).to.equal(false); }); - it('returns false for object type', () => { - // $DisableFlowOnNegativeTest - expect(isSpecifiedDirective(ObjectType)).to.equal(false); + it('returns false for non-directive', () => { + expect(isSpecifiedDirective(EnumType)).to.equal(false); + expect(isSpecifiedDirective(ScalarType)).to.equal(false); }); it('returns false for spec defined scalar type', () => { - // $DisableFlowOnNegativeTest expect(isSpecifiedDirective(GraphQLString)).to.equal(false); }); + it('returns false for scalar type named like specified directive', () => { + const ScalarNamedLikeDirective = new GraphQLScalarType({ + name: 'deprecated', + serialize: () => null, + }); + expect(isSpecifiedDirective(ScalarNamedLikeDirective)).to.equal(false); + }); + it('returns false for random garbage', () => { - // $DisableFlowOnNegativeTest expect(isSpecifiedDirective({ what: 'is this' })).to.equal(false); }); }); diff --git a/src/type/directives.js b/src/type/directives.js index c3ef545557..48194b70e3 100644 --- a/src/type/directives.js +++ b/src/type/directives.js @@ -187,10 +187,13 @@ export const specifiedDirectives: $ReadOnlyArray<*> = [ GraphQLDeprecatedDirective, ]; -export function isSpecifiedDirective( - directive: GraphQLDirective, -): boolean %checks { - return specifiedDirectives.some( - specifiedDirective => specifiedDirective.name === directive.name, +export function isSpecifiedDirective(directive: mixed): boolean %checks { + return ( + isDirective(directive) && + // Would prefer to use specifiedDirectives.some(), however %checks needs + // a simple expression. + (directive.name === GraphQLIncludeDirective.name || + directive.name === GraphQLSkipDirective.name || + directive.name === GraphQLDeprecatedDirective.name) ); } diff --git a/src/type/scalars.js b/src/type/scalars.js index f5293b6e5d..e6ed82f26f 100644 --- a/src/type/scalars.js +++ b/src/type/scalars.js @@ -10,7 +10,7 @@ import isFinite from '../polyfills/isFinite'; import isInteger from '../polyfills/isInteger'; import inspect from '../jsutils/inspect'; -import { GraphQLScalarType, isNamedType } from './definition'; +import { GraphQLScalarType, isScalarType } from './definition'; import { Kind } from '../language/kinds'; // As per the GraphQL Spec, Integers are only treated as valid when a valid @@ -255,7 +255,7 @@ export const specifiedScalarTypes: $ReadOnlyArray<*> = [ export function isSpecifiedScalarType(type: mixed): boolean %checks { return ( - isNamedType(type) && + isScalarType(type) && // Would prefer to use specifiedScalarTypes.some(), however %checks needs // a simple expression. (type.name === GraphQLString.name ||