Add support for fragment aliases

[tgriesser]

Saw this idea referenced in a Relay ticket: facebook/relay#309 (comment)

Per offline discussion with @leebyron and @dschafer we'll probably have to add support for fragment aliases in the language to make this seamless. This would use your earlier proposal, @fson:

# Relay products
fragment on User {
  foo: ${Child.getFragment('foo')},
}
# Raw GraphQL
fragment on User {
  foo: ...sub_0 # name generated by `relay-babel-plugin`
}

Thought it might be useful to open a ticket here as a reminder or for more discussion.

[leebyron]

Thanks for keeping this open!

I think we may come back to this as we work out our 2016 roadmap. We considered this feature for the initial version of the spec, but there are some pretty serious concerns to weigh.

The primary concern is response confusion. This feature can be powerful, but it can also lead to harder to understand queries. Right now one JSON object always corresponds to one GraphQL object, but this feature allows for deeply nested

The second concern is it can become easy to accidentally produce field over-fetching. For example consider:

fragment Example on User {
  foo: ...ChildFrag1
  bar: ...ChildFrag2
}

fragment ChildFrag1 on User {
  name
  age
  birthday
  more: ...ChildFrag2
}

fragment ChildFrag2 on User {
  name
  birthday
  profilePicture { url }
}

The result of this will be duplicate querying and returning of the name and birthday fields on the first level, and fetches everything in ChildFrag2 twice. Without the fragment aliases, there would be no over-fetching at all.

Considering that one major motivation for this is more safely separating fragments from each other so they are unlikely to overlap, this motivation is exactly what would cause this duplicate over-fetching to occur.

We should keep the issue open though as a place to collect discussion, and perhaps reconsider should we come up with new response formats that makes such a thing possible.

[leebyron]

Closing this aging issue. I think this could be re-proposed should a champion be compelled.

[dillonkearns]

Hello @leebyron! I would like to re-propose and champion this issue. What would you recommend as the first step for that?

Background

Here is the reason I am interested in championing this feature. I'm the author of a GraphQL client for Elm. The library abstracts away aliases for you and deserializes the GraphQL responses into Elm data structures based on your Elm request code. Since the data is turned into Elm data structures (not accessed directly as raw JSON as in JS clients), aliases are treated as an implementation detail of the Elm GraphQL client library. So the Elm client is responsible for creating aliases that result in a valid request. For example, it will automatically use an alias for fields based its arguments to ensure there are no collisions (I discuss it in detail in this post if you're curious!). And it's worth noting that the auto-generated aliases need to be independent of context (for reasons which I go into in the aforementioned post).

This works great, but I recently got a bug report regarding Counter Example 112:

# Counter Example No. 112
fragment conflictingDifferingResponses on Pet {
  ... on Dog {
    someValue: nickname
  }
  ... on Cat {
    someValue: meowVolume
  }
}

The problem is that if a type condition returns two different types under the same response key, there is an error from the GraphQL server (as the spec says it should). Here's the full issue: dillonkearns/elm-graphql#95. If you want more detail about how my Elm GraphQL client automatically generates unique aliases, you can see dillonkearns/elm-graphql#64 (comment).

The best solution I can think of to this problem for my library would be introducing Fragment Aliases. With this feature available, I would simply use a Fragment Alias that included the type of the fragment in the alias name. This would avoid the merge collision error here. I'm open to other ideas about how to solve this problem as well!

Again, please let me know what the best way to proceed is here. And thank you for such a wonderful and well-maintained project!

[dillonkearns]

@leebyron another way I could solve the problem I described above would be to use an alias with it the typename appended for each scalar field (such as nameString: name or avatarUri: avatar).

This is totally doable, and I'm open to taking that approach. And perhaps Fragment Type Aliases wouldn't get at the root problem.

I think it might help me if I understood the intention behind Counter Example No. 112 a little better. It makes perfect sense that fields can't be merged if they have different arguments, or if they alias to the same name but are different fields. But in the case of type conditions like counter example 112, there is no conflict because the data isn't actually being merged. I was surprised initially to find that this caused an error because merging seems like a "merge this and that together" sort of thing, whereas with type conditions it's more like "return back this, or return back that (but never both)". That is, it doesn't ever combine anything because the responses are mutually exclusive, so they could have different shapes.

So what I'm trying to understand better is what is the reason for disallowing these cases with different shapes within type conditions? Was the intention of this design to avoid user error by telling them that types are different? Or something else?

I appreciate any clarifications you can give!

[IvanGoncharov]

So what I'm trying to understand better is what is the reason for disallowing these cases with different shapes within type conditions? Was the intention of this design to avoid user error by telling them that types are different? Or something else?

@dillonkearns You can find answer inside commit that added it d481d17:

Might produce the following illegal Java artifacts:

interface A {
  int getField()
}

interface B implements A {
  string getField()
}

So basically without this rule, it's not possible to generate simple native data structures for strongly typed languages which is very important for mobile clients.

[dillonkearns]

@IvanGoncharov thank you so much for the clarification! That's really helpful 👍

So basically without this rule, it's not possible to generate simple native data structures for strongly typed languages

That makes sense. Elm is strongly typed, but my client, https://github.com/dillonkearns/elm-graphql, doesn't have this same issue because it generates code for you to build up your own queries. I think the type of client you're describing would take the approach of generating code based on a specific raw GraphQL String.

Another way to solve that problem could have been to let the clients give errors in such cases and not output any code unless it was disambiguated. But since some clients are depending on this behavior now, it would be a major breaking change so it's not worth considering at this point.

It's not ideal, but it sounds like the best approach for me is to use the typename in aliases for scalars as I described above. I wish there was a way where I didn't have to add more noise to the generated queries, but it will definitely get the job done.

Thank you again for clarifying the rationale for that part of the spec!

[IvanGoncharov]

Another way to solve that problem could have been to let the clients give errors in such cases and not output any code unless it was disambiguated.

@danielkwinsor This is an exact purpose for validation. Idea is that you validate your queries inside client source code and not on the server and AFAIK Facebook doing exactly that using persisted queries.

https://facebook.github.io/graphql/June2018/#sec-Validation

Any client‐side or development‐time tool should report validation errors and not allow the formulation or execution of requests known to be invalid at that given point in time.

[dillonkearns]

Hello @IvanGoncharov, thank you for taking the time to write these detailed responses, I really appreciate it!

I'm not sure if I'm understanding this correctly, but it looks like these requests still will be rejected by the server framework, right? Except in some cases where the server framework can choose to skip a validation for the purpose of backwards-compatibility?

An invalid request is still technically executable, and will always produce a stable result as defined by the algorithms in the Execution section, however that result may be ambiguous, surprising, or unexpected relative to a request containing validation errors, so execution should only occur for valid requests.

(The bold is mine). Does the bold section mean that the server should give an error, or is it only the clients that should provide errors (sort of like an optional linter warning). In other words, should these be errors or warnings on the server?

I was able to confirm that Elixir/Absinthe returns a 200 when making a request that fails this validation (you can run the request yourself here). Is this an exception, or should every implementation give a 200 status for this request?

[IvanGoncharov]

Does the bold section mean that the server should give an error, or is it only the clients that should provide errors (sort of like an optional linter warning). In other words, should these be errors or warnings on the server?

@dillonkearns Execution and Validation are fully separate operations. Validation should be done at some point but:

• it could be done multiple times (e.g., during development and on a server)
• if you in control of a client you can validate everything on a client and skip validation on server saving CPU and memory (e.g. persistent queries).

It is possible since validation is fully independent of execution and doesn't require anything except schema (that you can get using introspection) so you can fully validate query without sending it to the server. A good example is ESLint https://github.com/apollographql/eslint-plugin-graphql

So returning to the original question:

Another way to solve that problem could have been to let the clients give errors in such cases and not output any code unless it was disambiguated.

Instead of defining separate validation rules for every tool/server GraphQL defines a common set of rules for the entire ecosystem. That means GraphQL => Java can support all valid GraphQL queries and use GraphQL validation to validate its input.

[dillonkearns]

Hey @IvanGoncharov, yes that makes sense that GraphQL syntax limits what is valid in order to make any valid GraphQL query work for codegen tools. Thank you so much for helping me understand the rationale here, that's really helpful in my design process for my own GraphQL codegen tool! I've gone ahead and implemented a solution to the problem I described earlier using the typename in field aliases. It's working well (just a bit noisy in the demos)!

Thanks again for all your replies here Ivan!