graphql / composite-schemas-spec

MIT License
26 stars 9 forks source link

More GraphQL native expression for FieldSelection? #21

Open JohnStarich opened 7 months ago

JohnStarich commented 7 months ago

I'd like to explore a way to express the FieldSelection or Schemacoordinate value with something less surprising. It's surprising to me to see strings representing selection sets (even though they're used in Federation) or index paths in a GraphQL schema.

Examples of how it's written with those in the current proposal:

directive @is(
  field: FieldSelection
  coordinate: Schemacoordinate
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION

extend type Query {
  personById(id: ID! @is(field: "id")): Person @entityResolver
}

extend type Review {
  productSKU: ID! @is(coordinate: "Product.sku") @internal
  product: Product @resolve
}

(My understanding is the FieldSelection one also behaves like the one in Federation, where "id foo" is also valid for field foo.)

  1. Can these be expressed with an existing construct, like referencing a type and its fields?
  2. Can new keywords be introduced which allow this kind of selection?
JohnStarich commented 7 months ago

A (terrible) first idea for 1:

extend type Query {
  personById(id: ID! @is(fragment: PersonID)): Person @entityResolver
}

fragment PersonID on Person {
    id
}

This attempts to borrow existing syntax and concepts. Admittedly the meaning of fragment isn't a perfect match here, so it may warrant a different name.

benjie commented 7 months ago

I think I broadly agree with your sentiment, but not with some of the specifics.

Schema coordinates are essentially canon at this point (currently RFC2, but as far as I know there's no resistance to making it RFC3, it just hasn't been pushed - I vaguely recall it needs a parser to be added to GraphQL.js or something?): https://github.com/graphql/graphql-spec/pull/794 so I think leveraging them is fine (and a push to get them included into the spec).

I definitely agree that passing selections through strings labelled field is surprising, especially given they aren't valid GraphQL documents as it stands. Perhaps making it more explicit as selectionSet: "{ id foo }" would make sense.

Your fragment proposal is an interesting one, but that's actually more surprising to me (since parsing GraphQL from strings is covered by the spec already). The parser would see fragment: PersonID as if PersonID were an enum value; which is really surprising. I'd also be concerned about naming conflicts from fragments across different source schemas, and wonder whether the fragment would evaporate or would be used (as named) in the resulting query.

I'd definitely be interested in seeing alternative solutions to this; thanks for raising it @JohnStarich :+1:

smyrick commented 7 months ago

One thing to keep in mind is that schema coordinates are distinct and can refer to the exact location of a field, type, or argument, but composition libraries, like Apollo Federation, also use the FieldSelection to reflect the potential operation path you can

For example the @requires directive or provides directive does not use coordinates, because you can only reference fields on this type definition in this subgraph.

With schema coordinates, while it is more expressive, would it be more likely that someone would try referencing fields they can't? We of course validate selection sets in the implementation regardless but I think some might do this a lot more which is invalid:

# Apollo Fed schema
type Product @key(fields: "Product.id") {
  id: ID!

  # To resolve they type we need more info
  shippingEstimate: String @requires(fields: "Product.price")

    # Can I start referring to any type? To resolve this brand new entity we need even more info
  discount: String @requires(fields: "User.accountType") 
}

type Query {
  # Name is provided here, even though it is not defined above?
  outOfStockProducts: [Product!]! @provides(fields: "Product.name")

  # But must be fetched by entity reference here
  discontinuedProducts: [Product!]! 
}

One thing I do agree on though is that validating the schema coordinates would allow us to use spec libraries easier to in the implementations to know it is a valid string, and then we would just have to worry about the composition part being valid

smyrick commented 7 months ago

Also, today Federation allows you to use the wrapping brackets, they are just optional

type Location @key(fields: "{ id }") {
  id: ID!
}
benjie commented 7 months ago

(Note that we do have an RFC open for operation expressions, inspired by schema coordinates: https://github.com/graphql/graphql-wg/blob/main/rfcs/OperationExpressions.md )

smyrick commented 7 months ago

Another callout is that selection sets today support multiple and nested keys too. You could do this with array of schema coordinates but that starts to get a little more verbose I think

type Product @key(fields: "details { id } sku country") {
  details: ProductDetails
  sku: ID
  country: String
}

type ProductDetails {
   id: ID
   name: String
}

vs

type Product @key(coordinates: ["Product.details", "ProductDetails.id", "Product.sku", "Product.country"]) {
  details: ProductDetails
  sku: ID
  country: String
}

type ProductDetails {
   id: ID
   name: String
}
JohnStarich commented 7 months ago

From our discussion today:

benjie commented 7 months ago

Are fragments defined inside schemas valid in today's GraphQL spec?

A TypeSystemDocument (i.e. a document that defines a GraphQL schema, what we typically call "GraphQL SDL" or "IDL") cannot currently contain fragments:

https://spec.graphql.org/draft/#TypeSystemDocument

SimonSapin commented 7 months ago

In GraphQL syntax all argument values for directive applications must match the Value grammar:

Value[Const]
    [if not Const] Variable
    IntValue
    FloatValue
    StringValue
    BooleanValue
    NullValue
    EnumValue
    ListValue[?Const]
    ObjectValue[?Const]

As mentioned, an unquoted name would be an EnumValue. Validation would require it to be one of values of a specific enum specified in the directive definition.

But let’s say we use a quoted string to refer to the name of a fragment definitions. Fragments are typically not found in schemas today. In terms of grammar, the spec defines:

Document
    Definition[list]

ExecutableDocument
    ExecutableDefinition[list]

TypeSystemDocument
    TypeSystemDefinition[list]

TypeSystemExtensionDocument
    TypeSystemDefinitionOrExtension[list]

A fragment definition would be valid in a Document, but so would an operation. And Document is not what’s typically implemented. For example graphql-js’s GraphQLSchema class has data structures for type definitions and directive definitions, but not fragments. So a fragment would either cause an error or be silently be ignored when parsing into GraphQLSchema.