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

Variant of starWars with interface subtyping failure #389

Conversation

NicolasRouquette
Copy link

@NicolasRouquette NicolasRouquette commented Jul 23, 2018

Demonstrates the problem (#388) on a StarWars2 unit test variant that fails.

@NicolasRouquette
Copy link
Author

Propose a conceptually simple strategy to fix #388:

Just like InterfaceType[Ctx, Val] has a field: manualPossibleTypes,
the idea is to add an extra field: manualPossibleInterfaces.

@NicolasRouquette
Copy link
Author

@OlegIlyenko Can you please review the logic for interface subtyping in sangria.validation.TypeComparators.isSubType?

In particular, this line:

      case (t1: InterfaceType[_, _], t2: AbstractType) ⇒ (t1.name == t2.name) || schema.isPossibleInterface(t2.name, t1)

It uses a similar criteria than what's used for the Named case:

      case (t1: Named, t2: Named) ⇒ t1.name == t2.name

My original fix had a stronger criteria:

      case (t1: InterfaceType[_, _], t2: AbstractType) ⇒ (t1 == t2) || schema.isPossibleInterface(t2.name, t1)

However, this led to a failure in the schema extension test; see:
https://travis-ci.org/sangria-graphql/sangria/jobs/407010037#L1963

Since the spec says that type references are by name (see: http://facebook.github.io/graphql/June2018/#sec-Type-References) then it should be OK to use a name equality test to confirm that two types refer to the same thing.

@NicolasRouquette
Copy link
Author

This PR is out of scope of the June 2018 Graph QL Spec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant