graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.12k forks source link

[RFC] Custom Scalar Specification URIs #635

Open eapache opened 5 years ago

eapache commented 5 years ago

(This RFC grew out of the discussion around https://github.com/graphql/graphql-spec/issues/579 and is championed by @eapache and @andimarek.)

This RFC proposes attaching an optional URI to custom scalar definitions pointing to a document holding data-format, serialization, and coercion rules for the scalar.

Problem

The set of scalars provided within the main GraphQL specification is limited to primitive types (Int, String, etc) plus ID. In the wild, most schemas end up implementing several additional custom scalars for types like Date and URL; in fact the GraphQL specification currently recommends those two as examples of motivating use cases for custom scalars, and there have been proposals to add them to the main GraphQL specification (e.g. https://github.com/graphql/graphql-spec/pull/315, https://github.com/graphql/graphql-spec/issues/579). There are several closely-related problems with the world that has grown around the usage of these custom scalars:

Solution

Allow attaching a URI to custom scalar definitions, in both the SDL and via introspection. If present, the URI is expected to point to a definition of the data-format, serialization, and coercion rules for the scalar. This provides value because:

Out of Scope

The following problems are closely related, but considered out of scope for this RFC. They could be tackled in later RFCs if there is interest:

Alternatives and Variations

Cost

The primary costs of this RFC are:

Illustrative example

This example specifies a new “DateTime” scalar based on RFC3339. It should be published and hosted somewhere on graphql.org distinct from the main GraphQL spec.

The upstream RFC can be found here: https://www.ietf.org/rfc/rfc3339.txt. And the errata here: https://www.rfc-editor.org/errata/rfc3339.

RFC3339 has some ambiguities or unclear sections. This spec aims to clear up these ambiguities without introducing new semantics or changing any of the semantics established in RFC3339.

The format for input and output for this scalar is the same.

This DateTime scalar represents a “date-time” as specified in section 5.6. While RFC3339 allows also other formats (see Errata of the RFC) only “date-time” is accepted in this scalar.

The allowed separation character between “full-date” and “full-time” is “T”. In RFC3330 “time-secfrac” is optional, and not limited to a specific amount of digits. This scalar requires it to be present with exactly three digits representing milliseconds.

This scalar also doesn’t allow “-00:00” as offset to describe an unknown offset (See 4.3. Unknown Local Offset Convention in the RFC)

Example of valid DateTime scalars:

2011-08-30T13:22:53.108Z
2011-08-30T13:22:53.108-03
2011-08-30T13:22:53.108+03:30

Example of invalid DateTime scalars:

2011-08-30T13:22:53.108912Z  -> too many secfrac digits 
2011-08-30T13:22:53.108 -> no offset
2011-08-30 -> no time
2011-08-30T13:22:53.108-00:00 -> negative offset representing unknown offset not allowed

Spec edits

https://github.com/graphql/graphql-spec/pull/649

Concerns, challenges, and drawbacks

leebyron commented 5 years ago

Here's an alternate spec grammar and introspection idea for you:

scalar DateTime @specified(by: "https://tools.ietf.org/html/rfc3339")
Fields
* `kind` must return `__TypeKind.SCALAR`.
* `name` must return a String.
* `specifiedBy` may return an RFC3986-compliant URI or {null}.
* `description` may return a String or {null}.
* All other fields must return {null}.

This makes the SDL grammar change non-breaking, just defines a well-known directive (similar to @deprecated)

michaelstaib commented 5 years ago

@leebyron I think that feels great and aligns good with @deprecated

fotoetienne commented 5 years ago

An idea: A specified format for scalar specification documents.

Example: uri points to a json document that contains specified fields such as name, description etc. This would allow for:

eapache commented 4 years ago

@leebyron adding a new directive seems just as breaking as changing the grammar? What happens to existing schemas that have already defined a custom directive with that name? Should we call it @__specified or something?

eapache commented 4 years ago

Other than the grammar/format, I have updated the main RFC and draft specification to address all of the comments from the working group meeting.

leebyron commented 4 years ago

Technically almost any change to GraphQL can be determined to be breaking since ultimately someone somewhere can theoretically be relying on the behavior we're about to change. In practice we need to determine the sort of breakage and how likely it is to effect real consumers.

For a grammar change, this will affect all current GraphQL SDL parsers. Upon seeing grammar they do not yet understand (eg. a consumer has not yet upgraded to a version of their tool which supports the new grammar) the tool will present a syntax error and no value can be had from this point. This would be likely to affect a large number of people who use the SDL and encounter services which start to support this feature.

Directives were added for exactly this reason. We can extend without creating syntactically breaking changes (tools which don't understand the directive can ignore it). Of course your point is totally correct that any newly specified directive has some probability of colliding with someone's existing custom directive. There's no way to get this probability to zero, only to make it smaller and smaller by picking directive names less and less likely to already be used. The tradeoff is that names that are less likely to be used are probably also less understandable and useable by consumers. So we enter a balancing act. We should seek to pick the best name possible while breaking the fewest (ideally none) existing customers.

Since custom directives are not something casual consumers of GraphQL are using (really only a small number of sophisticated tools), this seems like something we could do a quick check or poll for.

eapache commented 4 years ago

Makes sense. I'm not sure how to poll for something like that other than to ask it at the next working group; if there's a better venue I'm open to suggestions.

Longer-term it might make sense to pre-reserve a directive namespace for future spec usage the same way we reserve types and fields that start with __. I'm not sure what that would look like though, and if custom directives are as unused as you suggest then it may not matter.

leebyron commented 4 years ago

Technically this limitation is already in place https://graphql.github.io/graphql-spec/draft/#sec-Type-System.Directives.Validation as a consistency with type and field naming limitation, but no directives actually make use of it.

I think the __ made sense as being intentionally somewhat odd for optionally tapping into less often used meta information as part of introspection. It's a good way to say "these things are less important than your schema, so they're getting out of the way with an odd namespace" but maybe a less good way to express common or important concepts like this one, or @deprecated which don't expect to be competing with your schema.

leebyron commented 4 years ago

I filed https://github.com/graphql/graphql-spec/issues/646 since I think this is worth elaborating on a bit in the spec itself.

eapache commented 4 years ago

I've finished the switch to a directive, and filed https://github.com/graphql/graphql-spec/pull/649 with what should be a complete draft of the specification edits needed for this RFC. At the next working group meeting we can poll for any last concerns (particularly around the compatibility of adding a directive) but barring any objections I believe this is ready to move to Draft (RFC 2).

eapache commented 4 years ago

We polled for concerns at the working group meeting, and found a few minor ones which I've addressed with tweaks to the spec PR. @m14t has been kind enough to create a draft implementation for graphql-js (https://github.com/graphql/graphql-js/pull/2276) so I believe this is actually ready for Draft/stage2 now.