Allow interface type extensions by lillyfwang · Pull Request #1222 · graphql/graphql-js

lillyfwang · 2018-02-02T05:53:25Z

The GraphQL SDL was recently updated to allow interface type extensions (along with other GraphQL type extensions), but extendSchema() hasn't been updated to actually support these type extensions (as noted in #1095). This PR adds support for extending interface types by extending every GraphQLObject type that implement the interface with the same new fields and directives. It also extends the interface type itself by updating the extensionASTNodes property for the GraphQLInterface Type.

Edit: Just saw #1199 - are there major conflicts with those changes/future work? It didn't seem like it at first glance.

IvanGoncharov · 2018-02-05T14:57:57Z

Edit: Just saw #1199 - are there major conflicts with those changes/future work? It didn't seem like it at first glance.

@lillyfwang Don't worry about it. My PR has a more long-term goal to allow simplify Schema transformation in general. Your PR is very helpful since it adds more test-cases and challenges transformSchema to handle more use cases 👍

IvanGoncharov · 2018-02-05T15:16:05Z

+            [def],
+          );
+        }
+        // Extend all of the object types that implement this interface


@lillyfwang You shouldn't do that, if you want to extend interface you shouldn't also extend all objects that implement this interface with missing fields:

extend interface SomeInterface { newInterface: NewInterface } extend type Foo { newInterface: NewObject } extend type Bar { newInterface: NewOtherObject }

Note: similar to how you normally define a type that implements an interface you can use compatible type. That's why you can't auto-implement interfaces 😞

@IvanGoncharov I'm not sure if I'm understanding your comment properly, but is your concern here that "NewOtherObject" could possibly not implement "NewInterface"? Is a solution here to validate the schema after extending it to make sure the new extended schema is valid?

@lillyfwang No I mean you can't do:

Extend all of the object types that implement this interface

You shouldn't add fields to GraphQLObjectType since extend interface should affect only interface not object types implementing this interface.

For example, if you use bellow schema:

interface SomeInterface { foo: String } type SomeObject { foo: String }

Incorrect extension (correct in your PR):

extend interface SomeInterface { bar: String }

Correct extension:

extend interface SomeInterface { bar: String } extend type SomeObject { bar: String }

Oh okay I understand - thanks for explaining!

I agree with @IvanGoncharov here - we want to preserve the use case for extending interface fields when types already have the fields defined, and we also want to ensure we can extend objects with a more specific variant of that field, example:

extend interface SomeInterface { newField: SomeInterface } extend type SomeObject { # Assuming it already implements SomeInterface newField: SomeObject # This is okay now, since SomeObject is more specific than SomeInterface }

leebyron

This is looking great, @lillyfwang - thanks for taking this on.

I think with a couple more tests and addressing the feedback this should be ready to merge

leebyron · 2018-02-06T18:37:58Z

+            [def],
+          );
+        }
+        // Extend all of the object types that implement this interface


I agree with @IvanGoncharov here - we want to preserve the use case for extending interface fields when types already have the fields defined, and we also want to ensure we can extend objects with a more specific variant of that field, example:

extend interface SomeInterface { newField: SomeInterface } extend type SomeObject { # Assuming it already implements SomeInterface newField: SomeObject # This is okay now, since SomeObject is more specific than SomeInterface }

leebyron · 2018-02-06T18:39:05Z

    astNode: schema.astNode,
  });

+  function appendExtensionToTypeExtensions(


lillyfwang · 2018-02-06T21:34:49Z

Just saw @leebyron's comment...updating with more tests now

IvanGoncharov · 2018-02-06T22:18:25Z

        break;
-      case Kind.SCALAR_TYPE_EXTENSION:
      case Kind.INTERFACE_TYPE_EXTENSION:
+        const extendedInterfaceTypeName = def.name.value;


nit: can you please move this case before Kind.DIRECTIVE_DEFINITION

IvanGoncharov · 2018-02-06T22:40:02Z

+            `Cannot extend interface "${extendedInterfaceTypeName}" because ` +
+              'it does not exist in the existing schema.',
+            [def],
+          );


You need to test it similar to how it's down for object type:

graphql-js/src/utilities/__tests__/extendSchema-test.js

Line 985 in b333b30

it('does not allow extending an unknown type', () => {

IvanGoncharov · 2018-02-06T22:40:55Z

+          throw new GraphQLError(
+            `Cannot extend non-interface type "${extendedInterfaceTypeName}".`,
+            [def],
+          );


Same here:

graphql-js/src/utilities/__tests__/extendSchema-test.js

Line 997 in b333b30

describe('does not allow extending a non-object type', () => {

IvanGoncharov · 2018-02-06T22:44:36Z

+        ] = appendExtensionToTypeExtensions(
+          def,
+          typeExtensionsMap[extendedInterfaceTypeName],
+        );


You need to test multiple extension of the same interface:

graphql-js/src/utilities/__tests__/extendSchema-test.js

Line 732 in b333b30

it('extends objects multiple times', () => {

IvanGoncharov · 2018-02-06T22:51:47Z

+    ]);
+  });
+
+  it('accepts Objects implementing the extended interface by extending Objects with fields', () => {


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.

IvanGoncharov · 2018-02-06T22:57:54Z

+    ]);
+  });
+
+  it('accepts an Object implementing the extended interface with existing field', () => {


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

IvanGoncharov · 2018-02-06T22:59:41Z

+        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,


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

IvanGoncharov · 2018-02-06T23:22:00Z

+      type Bar implements SomeInterface {
+        name: String
+        some: SomeInterface
+        foo: Foo


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

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).

@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 👍

IvanGoncharov · 2018-02-08T17:01:16Z

@lillyfwang Looks great now 👍
One issue is that you PR conflicting with master because of #1226
Can you please rebase your PR?

leebyron · 2018-02-08T19:33:18Z

Excellent work, @lillyfwang!

Allow interface type extensions

d35badd

facebook-github-bot added the CLA Signed label Feb 2, 2018

IvanGoncharov requested changes Feb 5, 2018

View reviewed changes

leebyron reviewed Feb 6, 2018

View reviewed changes

Do not extend object types that implement the interface

5fb61bc

Added more tests in schema validation after interface extensions

6f1afca

IvanGoncharov reviewed Feb 6, 2018

View reviewed changes

Added more test cases to extendSchema, removed some tests from validate

f6d7eeb

IvanGoncharov approved these changes Feb 8, 2018

View reviewed changes

Merge branch 'master' into extend-interface-types

0b58284

leebyron merged commit 2986c2d into graphql:master Feb 8, 2018

OlegIlyenko mentioned this pull request Feb 18, 2018

Verify: some minor changes to the ref. impl. sangria-graphql/sangria#311

Closed

13 tasks

mattkrick mentioned this pull request Mar 17, 2018

support extending interfaces JetBrains/js-graphql-intellij-plugin#137

Closed

dependabot-preview Bot mentioned this pull request Mar 19, 2018

build: bump graphql from 0.10.5 to 0.13.2 hongymagic/k2#160

Open

ckolderup mentioned this pull request Mar 28, 2018

Add support for interface extension ardatan/graphql-tools#699

Closed

snyk-bot mentioned this pull request Mar 3, 2020

[Snyk] Upgrade graphql from 0.13.1 to 0.13.2 SalesforceCommerceCloud/sfcc-sample-apps#132

Closed

snyk-bot mentioned this pull request Jun 1, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.4.1 turkdevops/graphql-js#196

Closed

Uh oh!

Conversation

lillyfwang commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IvanGoncharov commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IvanGoncharov Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leebyron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lillyfwang commented Feb 6, 2018

Uh oh!

IvanGoncharov Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leebyron commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lillyfwang commented Feb 2, 2018 •

edited

Loading

IvanGoncharov commented Feb 5, 2018 •

edited

Loading

IvanGoncharov Feb 5, 2018 •

edited

Loading

IvanGoncharov Feb 6, 2018 •

edited

Loading

IvanGoncharov Feb 7, 2018 •

edited

Loading

IvanGoncharov commented Feb 8, 2018 •

edited

Loading