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

More GraphQL native expression for FieldSelection? #21

Open
JohnStarich opened this issue Feb 8, 2024 · 9 comments
Open

More GraphQL native expression for FieldSelection? #21

JohnStarich opened this issue Feb 8, 2024 · 9 comments

Comments

@JohnStarich
Copy link

JohnStarich commented Feb 8, 2024

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
Copy link
Author

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
Copy link
Member

benjie commented Feb 8, 2024

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?): graphql/graphql-spec#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 👍

@smyrick
Copy link

smyrick commented Feb 15, 2024

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
Copy link

smyrick commented Feb 15, 2024

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

type Location @key(fields: "{ id }") {
  id: ID!
}

@benjie
Copy link
Member

benjie commented Feb 15, 2024

(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
Copy link

smyrick commented Feb 15, 2024

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
Copy link
Author

From our discussion today:

  • Schemacoordinate: There's general consensus that schema coordinates are canon at this point, so no need to have a look at this one.
  • FieldSelection:
    • Fragments seem to suit this fairly well. They're a familiar syntax and also used to define a selection set.
    • Are fragments defined inside schemas valid in today's GraphQL spec? If not, should it still be pursued?

@benjie
Copy link
Member

benjie commented Feb 15, 2024

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
Copy link
Contributor

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.

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

4 participants