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

GraphQLInterfaceType Fields conflict because they return conflicting types #522

Closed
TomMcQuarrieOTH opened this issue Oct 19, 2016 · 6 comments

Comments

@TomMcQuarrieOTH
Copy link

I have a scenario where multiple entities implement a common interface with the shared field "type", but each entity also has an attribute "recurOn", where MonthlyRecurrence.recurOn is a different set of enum values to FortnightlyRecurrence.recurOn. Currently if I try to query on this attribute I get the error Fields "recurOn" conflict because they return conflicting types.

{
  recurrence {
    type,
    ... on MonthlyRecurrence {
      specificRecurrenceDate,
      recurOn
    }
    ... on FortnightlyRecurrence {
      recurOn
    }
  }
}

I read through the thread here: #53 which is more specifically regarding types with differing arguments, and to me my scenario falls into the category of "safe divergence" which should be allowed for. Since the field is defined within each child entity I would think there's no opportunity for ambiguity. Do you think it would be safe to allow this scenario?

@stubailo
Copy link

I don't have an opinion on the underlying issue, but you can work around this limitation by using an alias in the query.

@TomMcQuarrieOTH
Copy link
Author

TomMcQuarrieOTH commented Oct 20, 2016

Thanks @stubailo. Aliases proved to be a valid workaround, although they did add some complexities to our tests. For those playing at home, the alias is added to the query as follows:

        recurrence {
          type,
          ... on MonthlyRecurrence {
            specificRecurrenceDate
            monthlyRecurOn: recurOn
          }
          ... on FortnightlyRecurrence {
            fortnightlyRecurOn: recurOn
          }
        }

@leebyron
Copy link
Contributor

The safe divergence case was fixed long ago, so unless you're using a very old version of graphql-js, then that case is being accounted for!

Looking at this example, I would not expect this to be a safe divergence though. Perhaps it doesn't matter for your particular environment if you're using dynamically typed languages, but if you were using a statically typed language, then you would have encountered a problem when trying to correctly type the result of recurrence.recurOn. If one of two different Enums could be used, then your client couldn't be sure of what values would appear.

In this case you have two options:

  1. Do what @stubailo suggested, and alias one or both fields to remove the type conflict.
  2. have both recurOn return the same Enum on the server (combine the possible values). That might make it less clear what the possible values are for the specific MonthlyRecurrence case vs all possible cases, however this query usage makes it seem like differentiating the cases might not matter since the client wishes to use them in a combined way.

@brabeji
Copy link

brabeji commented Feb 5, 2018

@leebyron There seems to be slight inconsistency. I believe these test cases are up to the spec:
brabeji@da34fd8

But I can't wrap my head around how the argument about strongly typed languages doesn't apply to object types. How is that different from enums? Obviously, I would like the case with enums to also be safe. 😇

I know I'm very late to the party but thank you very much for any explanation.

@IvanGoncharov
Copy link
Member

But I can't wrap my head around how the argument about strongly typed languages doesn't apply to object types.

@brabeji Query results are strongly typed based on possible field values:
http://facebook.github.io/graphql/draft/#sec-Field-Selection-Merging

TL;DR; Only field values are conflicting with each other. In your test case:

             ... on Character {
               name
             }
             ... on Human {
               rankObject { __typename name }
             }
             ... on Droid {
               rankObject { __typename name }
             }

You can think as it stong-typed with following unnamed type:

{
   name: String
   rankObject: {
      name: String
      __typename: String
   }
}

In you enum test case you can think as it stong-typed with:

{
  rank: HumanRank
  rank: DroidRank #Conflict here
}

@brabeji
Copy link

brabeji commented Feb 19, 2018

Btw I've just found that a very nice explanation is given in the reference code itself:

// Two types conflict if both types could not apply to a value simultaneously.

rankObject being a composite type

Composite types are ignored as their individual field types will be compared later recursively.

and

rank being a leaf type.

if (isLeafType(type1) || isLeafType(type2)) {

thank you @IvanGoncharov !

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

No branches or pull requests

5 participants