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.

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

Allow interface type extensions #1222

Merged
merged 5 commits into from
Feb 8, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
188 changes: 188 additions & 0 deletions src/type/__tests__/validation-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,194 @@ describe('Type System: Objects can only implement unique interfaces', () => {
});
});

describe('Type System: Interface extensions should be valid', () => {
it('rejects an Object implementing the extended interface due to missing field', () => {
const schema = buildSchema(`
type Query {
test: AnotherObject
}

interface AnotherInterface {
field: String
}

type AnotherObject implements AnotherInterface {
field: String
}
`);
const extendedSchema = extendSchema(
schema,
parse(`
extend interface AnotherInterface {
newField: String
}
`),
);
expect(validateSchema(extendedSchema)).to.containSubset([
{
message:
'Interface field AnotherInterface.newField expected but AnotherObject does not provide it.',
locations: [{ line: 3, column: 11 }, { line: 10, column: 7 }],
},
]);
});

it('accepts an Object implementing the extended interface with existing field', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's more appropriate to have this test as part of extendSchema test suite.

const schema = buildSchema(`
type Query {
test: AnotherObject
}

interface AnotherInterface {
field: String
}

type AnotherObject implements AnotherInterface {
field: String
newField: String
}
`);
const extendedSchema = extendSchema(
schema,
parse(`
extend interface AnotherInterface {
newField: String
}
`),
);
expect(validateSchema(extendedSchema)).to.deep.equal([]);
});

it('rejects an Object implementing the extended interface due to missing field args', () => {
const schema = buildSchema(`
type Query {
test: AnotherObject
}

interface AnotherInterface {
field: String
}

type AnotherObject implements AnotherInterface {
field: String
}
`);
const extendedSchema = extendSchema(
schema,
parse(`
extend interface AnotherInterface {
newField(test: Boolean): String
}

extend type AnotherObject {
newField: String
}
`),
);
expect(validateSchema(extendedSchema)).to.containSubset([
{
message:
'Interface field argument AnotherInterface.newField(test:) expected but AnotherObject.newField does not provide it.',
locations: [{ line: 3, column: 20 }, { line: 7, column: 11 }],
path: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

You can remove path here since it's compared with containSubset

},
]);
});

it('rejects Objects implementing the extended interface due to mismatching interface type', () => {
const schema = buildSchema(`
type Query {
test: AnotherObject
}

interface AnotherInterface {
field: String
}

type AnotherObject implements AnotherInterface {
field: String
}
`);
const extendedSchema = extendSchema(
schema,
parse(`
extend interface AnotherInterface {
newInterfaceField: NewInterface
}

interface NewInterface {
newField: String
}

interface MismatchingInterface {
newField: String
}

extend type AnotherObject {
newInterfaceField: MismatchingInterface
}
`),
);
expect(validateSchema(extendedSchema)).to.containSubset([
{
message:
'Interface field AnotherInterface.newInterfaceField expects type NewInterface but AnotherObject.newInterfaceField is type MismatchingInterface.',
locations: [{ line: 3, column: 30 }, { line: 15, column: 30 }],
},
]);
});

it('accepts Objects implementing the extended interface by extending Objects with fields', () => {
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to test failing cases because error messages should have correct locations (extend AST nodes) inside error objects.
But I'm not sure if it worth to test passing test here since they already covered in extendSchema.

const schema = buildSchema(`
type Query {
testObject: AnObject
testOtherObject: AnotherObject
}

interface AnotherInterface {
field: String
}

type AnObject implements AnotherInterface {
field: String
}

type AnotherObject implements AnotherInterface {
field: String
}
`);
const extendedSchema = extendSchema(
schema,
parse(`
extend interface AnotherInterface {
newInterfaceField: NewInterface
}

interface NewInterface {
newField: String
}

type NewObject implements NewInterface {
newField: String
}

type AnotherNewObject implements NewInterface {
newField: String
}

extend type AnObject {
newInterfaceField: NewObject
}

extend type AnotherObject {
newInterfaceField: AnotherNewObject
}
`),
);
expect(validateSchema(extendedSchema)).to.deep.equal([]);
});
});

describe('Type System: Interface fields must have output types', () => {
function schemaWithInterfaceFieldOfType(fieldType) {
const BadInterfaceType = new GraphQLInterfaceType({
Expand Down
101 changes: 101 additions & 0 deletions src/utilities/__tests__/extendSchema-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,107 @@ describe('extendSchema', () => {
`);
});

it('extends interfaces by adding new fields', () => {
const ast = parse(`
extend interface SomeInterface {
newObject: NewObject
newInterface: NewInterface
newUnion: NewUnion
newScalar: NewScalar
newTree: [Foo]!
newField(arg1: String, arg2: NewEnum!): String
}

type NewObject implements NewInterface {
baz: String
}

type NewOtherObject {
fizz: Int
}

interface NewInterface {
baz: String
}

union NewUnion = NewObject | NewOtherObject

scalar NewScalar

enum NewEnum {
OPTION_A
OPTION_B
}
`);
const originalPrint = printSchema(testSchema);
const extendedSchema = extendSchema(testSchema, ast);
expect(extendedSchema).to.not.equal(testSchema);
expect(printSchema(testSchema)).to.equal(originalPrint);
expect(printSchema(extendedSchema)).to.equal(dedent`
type Bar implements SomeInterface {
name: String
some: SomeInterface
foo: Foo
Copy link
Member

Choose a reason for hiding this comment

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

Probably bug here, should fail because both Foo and Bar are missing additional fields:

 newObject: NewObject
 newInterface: NewInterface
 newUnion: NewUnion
 newScalar: NewScalar
 newTree: [Foo]!
 newField(arg1: String, arg2: NewEnum!): String

Copy link
Contributor

Choose a reason for hiding this comment

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

Well printSchema should not fail here: we should be able to parse, print and extend invalid schemas. Only validateSchema(extendedSchema) should fail.

But if that's what you're testing, you should be explicit "can parse and print schema extension with missing Object fields", then prove that it's invalid by testing validateSchema fails (don't test the actual error message though, as we don't wan to force people to update this test when they update that error message).

Copy link
Member

@IvanGoncharov IvanGoncharov Feb 7, 2018

Choose a reason for hiding this comment

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

@mjmahone You are right, I completely forget about that.
But I still think this particular test should contain valid SDL examples to not confuse readers.

But if that's what you're testing, you should be explicit "can parse and print schema extension with missing Object fields", then prove that it's invalid by testing validateSchema fails (don't test the actual error message though, as we don't wan to force people to update this test when they update that error message).

Totally make sense to add one such test for buildShema and extendSchema functions 👍

}

type Biz {
fizz: String
}

type Foo implements SomeInterface {
name: String
some: SomeInterface
tree: [Foo]!
}

enum NewEnum {
OPTION_A
OPTION_B
}

interface NewInterface {
baz: String
}

type NewObject implements NewInterface {
baz: String
}

type NewOtherObject {
fizz: Int
}

scalar NewScalar

union NewUnion = NewObject | NewOtherObject

type Query {
foo: Foo
someUnion: SomeUnion
someEnum: SomeEnum
someInterface(id: ID!): SomeInterface
}

enum SomeEnum {
ONE
TWO
}

interface SomeInterface {
name: String
some: SomeInterface
newObject: NewObject
newInterface: NewInterface
newUnion: NewUnion
newScalar: NewScalar
newTree: [Foo]!
newField(arg1: String, arg2: NewEnum!): String
}

union SomeUnion = Foo | Biz
`);
});

it('may extend mutations and subscriptions', () => {
const mutationSchema = new GraphQLSchema({
query: new GraphQLObjectType({
Expand Down
Loading