Skip to content
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

Jump to bottom

Allows to add schema definition missing in the original schema #1441

Merged
merged 2 commits into from
Aug 1, 2018
Merged

Allows to add schema definition missing in the original schema #1441

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40efdac
Allows to add schema definition missing in the original schema
IvanGoncharov Jul 31, 2018
f365242
Address review comments
IvanGoncharov Jul 31, 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
27 changes: 26 additions & 1 deletion src/utilities/__tests__/extendSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,7 +1271,7 @@ describe('extendSchema', () => {
expect(schema.getMutationType()).to.equal(null);
});

it('does not allow new schema within an extension', () => {
it('does not allow overriding schema within an extension', () => {
const sdl = `
schema {
mutation: Mutation
Expand All @@ -1286,6 +1286,31 @@ describe('extendSchema', () => {
);
});

it('adds schema definition missing in the original schema', () => {
let schema = new GraphQLSchema({
directives: [
new GraphQLDirective({
name: 'onSchema',
locations: ['SCHEMA'],
}),
],
});
expect(schema.getQueryType()).to.equal(undefined);

const ast = parse(`
schema @onSchema {
query: Foo
}

type Foo {
bar: String
}
`);
schema = extendSchema(schema, ast);
const queryType = schema.getQueryType();
expect(queryType).to.have.property('name', 'Foo');
});

it('adds new root types via schema extension', () => {
const schema = extendTestSchema(`
extend schema {
Expand Down
27 changes: 19 additions & 8 deletions src/utilities/extendSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import type {
DocumentNode,
DirectiveDefinitionNode,
SchemaExtensionNode,
SchemaDefinitionNode,
} from '../language/ast';

type Options = {|
Expand Down Expand Up @@ -106,18 +107,28 @@ export function extendSchema(
// have the same name. For example, a type named "skip".
const directiveDefinitions: Array<DirectiveDefinitionNode> = [];

let schemaDef: ?SchemaDefinitionNode;
// Schema extensions are collected which may add additional operation types.
const schemaExtensions: Array<SchemaExtensionNode> = [];

for (let i = 0; i < documentAST.definitions.length; i++) {
const def = documentAST.definitions[i];
switch (def.kind) {
case Kind.SCHEMA_DEFINITION:
// Sanity check that a schema extension is not defining a new schema
throw new GraphQLError(
'Cannot define a new schema within a schema extension.',
[def],
);
// 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:
schemaExtensions.push(def);
break;
Expand Down Expand Up @@ -216,9 +227,9 @@ export function extendSchema(
subscription: extendMaybeNamedType(schema.getSubscriptionType()),
};

// Then, incorporate all schema extensions.
for (const schemaExtension of schemaExtensions) {
if (schemaExtension.operationTypes) {
// Then, incorporate schema definition and all schema extensions.
for (const schemaExtension of [schemaDef, ...schemaExtensions]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wait just saw this: can we do instead:

if (schemaDef) {
  schemaExtensions.push(schemaDef);
}
for (const schemaExtension of schemaExtensions) {

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 You are right, the current code is confusing 😞
Sadly, schemaExtensions used bellow explicitly for extension nodes.
Moreover schemaDef doesn't need schemaDef.operationTypes because it always initialized.
So I decided to just duplicate code since #1435 will reduce this code and we can simplify things in subsequent refactorings.

if (schemaExtension && schemaExtension.operationTypes) {
for (const operationType of schemaExtension.operationTypes) {
const operation = operationType.operation;
if (operationTypes[operation]) {
Expand Down