Skip to content

[RFC]SDL validation #1438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 5, 2018
Merged

[RFC]SDL validation #1438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce1d49d
Add SDL validation rule to validate schema uniques
IvanGoncharov Aug 2, 2018
ca82c30
Apply "UniqueDirectivesPerLocation" to SDL
IvanGoncharov Jul 30, 2018
6aff49c
Adapt "KnownDirectives" to work with SDL
IvanGoncharov Jul 30, 2018
1e24f87
Add more rules to validate SDL
IvanGoncharov Aug 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/type/__tests__/validation-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ describe('Type System: Input Objects must have fields', () => {
schema = extendSchema(
schema,
parse(`
directive @test on ENUM
directive @test on INPUT_OBJECT

extend input SomeInputObject @test
`),
Expand Down
19 changes: 19 additions & 0 deletions src/utilities/__tests__/buildASTSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,25 @@ describe('Schema Builder', () => {
const errors = validateSchema(schema);
expect(errors.length).to.equal(0);
});

it('Rejects invalid SDL', () => {
const doc = parse(`
type Query {
foo: String @unknown
}
`);
expect(() => buildASTSchema(doc)).to.throw('Unknown directive "unknown".');
});

it('Allows to disable SDL validation', () => {
const body = `
type Query {
foo: String @unknown
}
`;
buildSchema(body, { assumeValid: true });
buildSchema(body, { assumeValidSDL: true });
});
});

describe('Failures', () => {
Expand Down
17 changes: 17 additions & 0 deletions src/utilities/__tests__/extendSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,23 @@ describe('extendSchema', () => {
expect(isScalarType(arg1.type)).to.equal(true);
});

it('Rejects invalid SDL', () => {
const sdl = `
extend schema @unknown
`;
expect(() => extendTestSchema(sdl)).to.throw(
'Unknown directive "unknown".',
);
});

it('Allows to disable SDL validation', () => {
const sdl = `
extend schema @unknown
`;
extendTestSchema(sdl, { assumeValid: true });
extendTestSchema(sdl, { assumeValidSDL: true });
});

it('does not allow replacing a default directive', () => {
const sdl = `
directive @include(if: Boolean!) on FIELD | FRAGMENT_SPREAD
Expand Down
15 changes: 12 additions & 3 deletions src/utilities/buildASTSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import keyMap from '../jsutils/keyMap';
import keyValMap from '../jsutils/keyValMap';
import type { ObjMap } from '../jsutils/ObjMap';
import { valueFromAST } from './valueFromAST';
import { assertValidSDL } from '../validation/validate';
import blockStringValue from '../language/blockStringValue';
import { TokenKind } from '../language/lexer';
import { parse } from '../language/parser';
Expand Down Expand Up @@ -86,6 +87,13 @@ export type BuildSchemaOptions = {
* Default: false
*/
commentDescriptions?: boolean,

/**
* Set to true to assume the SDL is valid.
*
* Default: false
*/
assumeValidSDL?: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just piggyback on the GraphQLSchemaValidationOptions's assumeValid key.

If I set assumeValid and then get an error thrown when building the AST schema, that would be surprising. Especially as buildASTSchema's sole job is to build a schema.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjmahone Good catch 🔎 🐞
Fixed.

};

/**
Expand All @@ -112,6 +120,10 @@ export function buildASTSchema(
throw new Error('Must provide a document ast.');
}

if (!options || !(options.assumeValid || options.assumeValidSDL)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjmahone We need separate assumeValidSDL check so developers are able to change set of validation rules(add or delete).

const errors = validateSDL(ast, null, customSetOfRules);
if (errors.length !== 0) {
  // ...
}
const schema = buildASTSchema(ast, { assumeValidSDL: true });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok this makes sense. A little bit annoying to add in that second key, but not the end of the world.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait: are you thinking assumeValid is a superset of assumeValidSDL or disjoint? I would still ideally have it be a superset. We could add assumeValidExecutableAST or something that is disjoint with assumeValidSDL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjmahone My proposal:
{} => validateSDL + validateSchema
{ assumeValidSDL: true } => validateSchema
{assumeValid: true } => no checks

assertValidSDL(ast);
}

let schemaDef: ?SchemaDefinitionNode;

const typeDefs: Array<TypeDefinitionNode> = [];
Expand All @@ -121,9 +133,6 @@ export function buildASTSchema(
const d = ast.definitions[i];
switch (d.kind) {
case Kind.SCHEMA_DEFINITION:
if (schemaDef) {
throw new Error('Must provide only one schema definition.');
}
schemaDef = d;
break;
case Kind.SCALAR_TYPE_DEFINITION:
Expand Down
24 changes: 12 additions & 12 deletions src/utilities/extendSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import keyMap from '../jsutils/keyMap';
import keyValMap from '../jsutils/keyValMap';
import objectValues from '../jsutils/objectValues';
import { ASTDefinitionBuilder } from './buildASTSchema';
import { assertValidSDLExtension } from '../validation/validate';
import { GraphQLError } from '../error/GraphQLError';
import { isSchema, GraphQLSchema } from '../type/schema';
import { isIntrospectionType } from '../type/introspection';
Expand Down Expand Up @@ -67,6 +68,13 @@ type Options = {|
* Default: false
*/
commentDescriptions?: boolean,

/**
* Set to true to assume the SDL is valid.
*
* Default: false
*/
assumeValidSDL?: boolean,
|};

/**
Expand Down Expand Up @@ -99,6 +107,10 @@ export function extendSchema(
'Must provide valid Document AST',
);

if (!options || !(options.assumeValid || options.assumeValidSDL)) {
assertValidSDLExtension(documentAST, schema);
}

// Collect the type definitions and extensions found in the document.
const typeDefinitionMap = Object.create(null);
const typeExtensionsMap = Object.create(null);
Expand All @@ -115,18 +127,6 @@ export function extendSchema(
const def = documentAST.definitions[i];
switch (def.kind) {
case Kind.SCHEMA_DEFINITION:
// Sanity check that a schema extension is not overriding the schema
if (
schema.astNode ||
schema.getQueryType() ||
schema.getMutationType() ||
schema.getSubscriptionType()
) {
throw new GraphQLError(
'Cannot define a new schema within a schema extension.',
[def],
);
}
schemaDef = def;
break;
case Kind.SCHEMA_EXTENSION:
Expand Down
18 changes: 18 additions & 0 deletions src/validation/ValidationContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import type {
FragmentSpreadNode,
FragmentDefinitionNode,
} from '../language/ast';
import type { ASTVisitor } from '../language/visitor';
import type { GraphQLSchema } from '../type/schema';
import type {
GraphQLInputType,
Expand Down Expand Up @@ -64,6 +65,21 @@ export class ASTValidationContext {
}
}

export class SDLValidationContext extends ASTValidationContext {
_schema: ?GraphQLSchema;

constructor(ast: DocumentNode, schema?: ?GraphQLSchema): void {
super(ast);
this._schema = schema;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should even allow access to a GraphQLSchema here: instead, if you pass in an existing schema, we can convert that schema to AST nodes (a very rough way to do this would be to parse(printSchema(schema))).

Ideally, the current GraphQLSchema would only be a runtime thing: you need it to validate queries you're trying to persist, but you don't need it to validate or transform itself.

I like the idea that there's a split between "validation that needs a schema" and "validation that does not", and "validation that does not need a schema" is a superset of "SDL validation".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjmahone Problem is that conversion from SDL to GraphQLSchema is lossless, but not vice versa. For example, you lose all directives which prevent from enforcing the rule that forbids directive duplication in extends.

Also after parse(printSchema(schema))) you loose ability to print errors with original locations and formating of nodes.

And there are others many potential scenarious where we loose something by such converion especially if schema that we trying to extend was build from GraphQL*Types not from SDL. For example, developer can create custom class derived GraphQLObjectType with some additional data and require this additional data inside his custom SDL rules.

And it's not a technical problem, since GraphQLSchema is intended to be most complete represenation of schema.

and "validation that does not need a schema" is a superset of "SDL validation".

"validation that does not need a schema" => ASTValidationContext
And this context suffisient for all rules that scoped to particular defintion.

If rule should check conflicts/dependencies beetween definitions it should handle case when you extending existing schema and use SDLValidationContext.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fix it so it's lossless, but that would be a different PR, and we could probably target 14.1 for that.

Also after parse(printSchema(schema))) you loose ability to print errors with original locations and formating of nodes.

I think there's a way around this, where we'd do buildSchemaDocuments(schema): Array<DocumentNode>, where each DocumentNode would contain all definitions from a specific source. That's obviously more complex of a change than what we want to do here, but I think would be worth having. For GraphQL*Types we'd have to have some form of "default" or "non-Source" document. But for truly custom classes, you're right we can't make lossless conversions.

If rule should check conflicts/dependencies beetween definitions it should handle case when you extending existing schema and use SDLValidationContext.

Makes sense, so let's push this forward as-is. But I'd like to switch our mental model to being AST-validation first, and then only using the schema when it's truly necessary. You've mostly accomplished this, but I envision a bunch of people creating validation rules that silently fail if no schema is present (because they assume they always have a schema present). That would be an unfortunate future.

}

getSchema(): ?GraphQLSchema {
return this._schema;
}
}

export type SDLValidationRule = SDLValidationContext => ASTVisitor;

export class ValidationContext extends ASTValidationContext {
_schema: GraphQLSchema;
_typeInfo: TypeInfo;
Expand Down Expand Up @@ -234,3 +250,5 @@ export class ValidationContext extends ASTValidationContext {
return this._typeInfo.getArgument();
}
}

export type ValidationRule = ValidationContext => ASTVisitor;
143 changes: 92 additions & 51 deletions src/validation/__tests__/KnownDirectives-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,24 @@
*/

import { describe, it } from 'mocha';
import { expectPassesRule, expectFailsRule } from './harness';
import { buildSchema } from '../../utilities';
import {
expectPassesRule,
expectFailsRule,
expectSDLErrorsFromRule,
} from './harness';

import {
KnownDirectives,
unknownDirectiveMessage,
misplacedDirectiveMessage,
} from '../rules/KnownDirectives';

const expectSDLErrors = expectSDLErrorsFromRule.bind(
undefined,
KnownDirectives,
);

function unknownDirective(directiveName, line, column) {
return {
message: unknownDirectiveMessage(directiveName),
Expand All @@ -27,6 +38,20 @@ function misplacedDirective(directiveName, placement, line, column) {
};
}

const schemaWithSDLDirectives = buildSchema(`
directive @onSchema on SCHEMA
directive @onScalar on SCALAR
directive @onObject on OBJECT
directive @onFieldDefinition on FIELD_DEFINITION
directive @onArgumentDefinition on ARGUMENT_DEFINITION
directive @onInterface on INTERFACE
directive @onUnion on UNION
directive @onEnum on ENUM
directive @onEnumValue on ENUM_VALUE
directive @onInputObject on INPUT_OBJECT
directive @onInputFieldDefinition on INPUT_FIELD_DEFINITION
`);

describe('Validate: Known directives', () => {
it('with no directives', () => {
expectPassesRule(
Expand Down Expand Up @@ -138,10 +163,36 @@ describe('Validate: Known directives', () => {
);
});

describe('within schema language', () => {
describe('within SDL', () => {
it('with directive defined inside SDL', () => {
expectSDLErrors(`
type Query {
foo: String @test
}

directive @test on FIELD_DEFINITION
`).to.deep.equal([]);
});

it('with standard directive', () => {
expectSDLErrors(`
type Query {
foo: String @deprecated
}
`).to.deep.equal([]);
});

it('with overrided standard directive', () => {
expectSDLErrors(`
schema @deprecated {
query: Query
}
directive @deprecated on SCHEMA
`).to.deep.equal([]);
});

it('with well placed directives', () => {
expectPassesRule(
KnownDirectives,
expectSDLErrors(
`
type MyObj implements MyInterface @onObject {
myField(myArg: Int @onArgumentDefinition): String @onFieldDefinition
Expand Down Expand Up @@ -180,13 +231,13 @@ describe('Validate: Known directives', () => {
}

extend schema @onSchema
`,
);
`,
schemaWithSDLDirectives,
).to.deep.equal([]);
});

it('with misplaced directives', () => {
expectFailsRule(
KnownDirectives,
expectSDLErrors(
`
type MyObj implements MyInterface @onInterface {
myField(myArg: Int @onInputFieldDefinition): String @onInputFieldDefinition
Expand All @@ -213,49 +264,39 @@ describe('Validate: Known directives', () => {
}

extend schema @onObject
`,
[
misplacedDirective('onInterface', 'OBJECT', 2, 43),
misplacedDirective(
'onInputFieldDefinition',
'ARGUMENT_DEFINITION',
3,
30,
),
misplacedDirective(
'onInputFieldDefinition',
'FIELD_DEFINITION',
3,
63,
),
misplacedDirective('onEnum', 'SCALAR', 6, 25),
misplacedDirective('onObject', 'INTERFACE', 8, 31),
misplacedDirective(
'onInputFieldDefinition',
'ARGUMENT_DEFINITION',
9,
30,
),
misplacedDirective(
'onInputFieldDefinition',
'FIELD_DEFINITION',
9,
63,
),
misplacedDirective('onEnumValue', 'UNION', 12, 23),
misplacedDirective('onScalar', 'ENUM', 14, 21),
misplacedDirective('onUnion', 'ENUM_VALUE', 15, 20),
misplacedDirective('onEnum', 'INPUT_OBJECT', 18, 23),
misplacedDirective(
'onArgumentDefinition',
'INPUT_FIELD_DEFINITION',
19,
24,
),
misplacedDirective('onObject', 'SCHEMA', 22, 16),
misplacedDirective('onObject', 'SCHEMA', 26, 23),
],
);
`,
schemaWithSDLDirectives,
).to.deep.equal([
misplacedDirective('onInterface', 'OBJECT', 2, 43),
misplacedDirective(
'onInputFieldDefinition',
'ARGUMENT_DEFINITION',
3,
30,
),
misplacedDirective('onInputFieldDefinition', 'FIELD_DEFINITION', 3, 63),
misplacedDirective('onEnum', 'SCALAR', 6, 25),
misplacedDirective('onObject', 'INTERFACE', 8, 31),
misplacedDirective(
'onInputFieldDefinition',
'ARGUMENT_DEFINITION',
9,
30,
),
misplacedDirective('onInputFieldDefinition', 'FIELD_DEFINITION', 9, 63),
misplacedDirective('onEnumValue', 'UNION', 12, 23),
misplacedDirective('onScalar', 'ENUM', 14, 21),
misplacedDirective('onUnion', 'ENUM_VALUE', 15, 20),
misplacedDirective('onEnum', 'INPUT_OBJECT', 18, 23),
misplacedDirective(
'onArgumentDefinition',
'INPUT_FIELD_DEFINITION',
19,
24,
),
misplacedDirective('onObject', 'SCHEMA', 22, 16),
misplacedDirective('onObject', 'SCHEMA', 26, 23),
]);
});
});
});
Loading