Proposed fix: lock down aliases that may break queries

[gmac]

The Problem

Field aliases are extremely difficult in federation design because they permit clients to break their own requests by manipulating reserved selection hints. Case of point, here's a busted query:

query {
  shop1 {
    products {
      boo: id
      name
    }
  }
}

Here's another...

query {
  shop1 {
    products {
      id: name
    }
  }
}

And another...

query {
  shop1 {
    products {
      id
      _id: name
    }
  }
}

As demonstrated, there are lots of creative ways to disrupt key selections. This same situation applies when __typename is hijacked as a custom alias on abstract types.

Proposed solution

To lock this down, validation errors are necessary to reserve key aliases for implementation details. I would propose that __id and __typename are standardized and reserved as system aliases on boundary types (current _id becomes __id, and the double-underscore becomes a convention of reserved system fields).

With that, errors are inserted into the query planner to invalidate requests that conflict with reserved aliases:

// while planning field selections...
if selection.Alias == "__id" && selection.Name != "id" {
  gqlerror.Errorf("invalid alias: \"__id\" is a reserved alias on type %s", parentType)
}
if selection.Alias == "__typename" && selection.Name != "__typename" {
  gqlerror.Errorf("invalid alias: \"__typename\" is a reserved alias on type %s", parentType)
}

Lastly, __id is always added to boundary types, and __typename is always added to abstract types (there's a PR in the queue for the later change).

@nmaquet and @lucianjon – feelings on this proposal? If we're aligned on the approach, I'll put a PR together after #89 goes out.

[yaacovCR]

All fields defined within an Object type must not have a name which begins with "__" (two underscores), as this is used exclusively by GraphQL’s introspection system.

https://spec.graphql.org/draft/#sec-Objects

[gmac]

Hmm. Fair. @yaacovCR how does Tools stitching handle this situation of user-defined selections conflicting with selection sets? IE: if there’s a user selection of “id: name”, and a selectionSet of “{ id }”, then how is that reconciled?

[nmaquet]

To be fair, the above spec snippet is referencing object type names and field names, not aliases. Still, it may be preferable to stay away from __ to avoid any confusion? @gmac How about _bramble_id, instead? It's a bit ugly but doesn't clash with the spec (or other tools) and makes it clear where it comes from. Thoughts?

[gmac]

Okay by me. Also introductions are in order… @yaacovCR is the primary author of GraphQL Tools Schema Stitching; I’ve worked with him on that project as backup vocals. If he’s started trolling Bramble, then we have made a powerful ally.

[nmaquet]

Excellent! Nice to meet you @yaacovCR. Your trolls are most welcome.

[yaacovCR]

I hope this is more lurking than trolling. Agree that above narrative is only with regard to field names.

@freiksenet (as far as I know) originally created graphql tools schema stitching functionality.

i added the type merging capabilities and then formed a part of a set of very constructive @gmac feedback/issue/pr cycles (that seem to be now happening here!)

All the best, Yaacov

ps I think tools does not do any query validation in this regard and just lets things blow up if key fields are aliased to something else in query.

[nmaquet]

Thanks for the feedback @yaacovCR.