Add `@requiresOptIn` directive

martinbonnin commented 2 years ago

You can read the rendered RFC at https://github.com/graphql/graphql-wg/blob/main/rfcs/OptInFeatures.md

Symmetrically to @deprecated, was it ever considered to add an @requiresOptIn directive?

"""
Indicates that the given field, argument, input field or enum value requires
giving explicit consent before being used.
"""
directive @requiresOptIn(feature: String!) repeatable
on FIELD_DEFINITION
    | ARGUMENT_DEFINITION
    | INPUT_FIELD_DEFINITION
    | ENUM_VALUE

@requiresOptIn directives could be used to indicate that a given field hasn't reached stable status yet. It's usable for debug/development purpose but is also subject to breaking changes such as renaming and/or type change.

Documentation and tooling could then use that information so that the client developer can make more informed decisions. In Kotlin, for an example, such fields could be annotated with an Opt-in annotation so that the caller has to explicitly enable them.

BoD commented 2 years ago

That would be useful!

such as renaming and/or type change.

Or even deleting I suppose.

benjie commented 2 years ago

I like this. I wonder if, given it's purely informational, it would be better for this to be provided via #300 rather than needing its own dedicated behaviour like @deprecated has. Note that @deprecated has a concrete effect on introspection (deprecated fields are omitted from introspection by default, but can be included if the includeDeprecated argument is passed) - I don't think similar would be true for @experimental.

martinbonnin commented 2 years ago

Good question. I'm not really sure about introspection. I guess #300 would avoid adding any specific introspection handling there.

In all cases, there's still the need to add it to the spec and reserve the name so that all tooling can agree on the semantics. That would allow all tooling working with SDL to support it. Tooling using introspection like GraphiQL would have to wait until #300 is added.

benjie commented 2 years ago

I don't think we'd add it to the spec without it being introspectable, so it needs to be added to the introspection schema one way or another before it can be merged, in my opinion. I'm not keen for us to add an isExperimental / experimentalReason to be analogous to the isDeprecated / deprecatedReason introspection fields when #300 might solve this in a broader context.

martinbonnin commented 2 years ago

experimentalReason

That was another question I had actually. I don't really see a use case for experimentalReason. All my use cases will most likely say something like "this field is experimental and might be changed in a backward incompatible manner". We can add experimentalReason for symmetry/future-proofness but there's a risk it'll rarely be used.

I'm not keen for us to add an isExperimental / experimentalReason to be analogous to the isDeprecated / deprecatedReason introspection fields when https://github.com/graphql/graphql-spec/issues/300 might solve this in a broader context.

Agreed 👍

jturkel commented 2 years ago

Note that @deprecated has a concrete effect on introspection (deprecated fields are omitted from introspection by default, but can be included if the includeDeprecated argument is passed) - I don't think similar would be true for @experimental.

Including experimental fields in introspection by default could result in clients accidentally using unstable experimental fields. Perhaps clients should be required to opt-in if they want to use experimental parts of the API to avoid such mistakes (although this makes #300 alone insufficient for solving this problem)?

The GitHub API has pretty nice schema previews infrastructure for exposing experimental parts of their API that don't have the same stability guarantees as the rest of the API. It requires clients to opt into a preview by specifying an Accept HTTP header. The GitHub model also supports multiple named previews for exposing different parts of the experimental API e.g. deployments is a separate preview than merge info. This could be a nice use case for experimentalReason or some general mechanism for additional metadata.

benjie commented 2 years ago

It almost feels like "deprecated" isn't really "deprecated" and more "don't include this by default because [...]" (deprecated/unstable/etc)

benjie commented 2 years ago

@martinbonnin Would you like to present this to the GraphQL WG? Add yourself and a 20 minute agenda item to https://github.com/graphql/graphql-wg/blob/main/agendas/2022/2022-06-02.md (there's comments in that file instructing how to do so) or another convenient date. If you cannot make it to a GraphQL WG I'd be happy to raise it to the WG on your behalf, you can watch the discussion afterwards on YouTube.

martinbonnin commented 2 years ago

@benjie Sure thing! Pull request is there https://github.com/graphql/graphql-wg/pull/964.

fotoetienne commented 2 years ago

It almost feels like "deprecated" isn't really "deprecated" and more "don't include this by default because [...]" (deprecated/unstable/etc)

We've used deprecated in this way, but as kind of a hack leveraging the fact that isDeprecated is exposed in introspection and already recognized by GraphiQL. For context, we introduced the @experimental directive at Netflix in 2019, with an implementation in our gateway that automatically transforms it into @deprecated(reason: "Experimental"). First class support for @experimental would allow @deprecated to remain specific to deprecation which is more ideal.

Regarding isExperimental vs a more general solution like #300: either could be fine as long as it's possible for tools like GraphiQL to have a "include experimental" check box. IIRC GraphiQL always sets includeDeprecated: true and just filters at runtime, so #300 would probably be sufficient.

leebyron commented 2 years ago

Thoughts:

Since Kotlin is the precedent for this behavior, consider they deprecated "experimental" in favor of optin

Perhaps:

directive @optIn(feature: String)

Where elements with this directive are not exposed in introspection by default unless expressly asked for it. So adding:

includeDeprecated: Boolean
includeOptIn: [String]

Where the array of strings provided would need to match feature for those elements to appear?

Open Qs from me:

How would it interact with deprecated? Can you deprecate an experimental/opt-in?
Does validation of schema get more complex? The interaction of validation with deprecation is already non-trivial

martinbonnin commented 2 years ago

If we're going the full Kotlin-like route, we could even think of adding directive usages to directive definitions:

Specified directive:

directive @optIn

User schema for something like Github Deployment Previews:

# optIn usage defines @experimentalDeploymentApi as an opt-in marker
directive @experimentalDeploymentApi @optIn

type Query {
  deployment: Deployment @experimentalDeploymentApi
}

This would require a change to the grammar:

description directive @Name ArgumentsDefinition directives repeatable on DirectiveLocations

TypeSystemDirectiveLocation =
SCHEMA
OBJECT
[...]
DIRECTIVE_DEFINITION

To include/exclude introspection, we could pass a list of marker names:

includeDeprecated: Boolean
includeOptIn: [String] #array of marker directive names

That might take us a bit far but it'd allow more customization on the user side as the directive aren't limited to a single feature argument anymore

martinbonnin commented 2 years ago

I've added a RFC at https://github.com/martinbonnin/graphql-wg/blob/opt-in-features/rfcs/OptInFeatures.md. This is still pretty much a work in progress but it'll be easier to read for someone that just bumps into this thread or wants the latest status.

@leebyron to answer your specific questions:

How would it interact with deprecated? Can you deprecate an experimental/opt-in?

I would think so? Especially if we say @optIn is not only about experimental status but also about potential other use cases?

A use case that comes to mind is a field that is super expensive to compute:

type Stats {
  aggregateAccrossTheWholeDb: Float @optIn(feature: "aggregates") @deprecated(reason: "use memoizedValue instead")
  memoizedValue: Float
}

Does validation of schema get more complex? The interaction of validation with deprecation is already non-trivial

I think so... For deprecation, the edge case is arguments and input fields, right? I think it'd make sense to add the same kind of language for @optIn features:

The `@optIn` directive must not appear on required (non-null without a default) arguments or 
input object field definitions.

In other words, `@optIn`  arguments or input fields, must be either nullable or have a default value.

Are there other cases?

tomgasson commented 2 years ago

At Atlassian we mark fields in the schema with @beta for this

directive @beta(name: String!) on FIELD_DEFINITION

type Project {
    """
    The team which owns the project
    """
    team: Team @beta(name: "teams")
}

That info is re-output in schema documentation (for GraphiQL and LSPs) by serialising it into the description of the field

The team which owns the project

# Beta Field
This field is currently in a beta phase and may change without notice.

To use this field you must set a X-ExperimentalApi: AppTags HTTP header.

Use of this header indicates that they are opting into the experimental preview of this field.

If you do not set this header then request will be rejected outright.

Once the field moves out of the beta phase, then the header will no longer be required or checked.

Similar to GitHub's Accept HTTP Header, we use a X-ExperimentalApi HTTP header where you can provide one or more features.

However, we have a problem with our current situation.

In Relay, fields are used in component fragments without knowing the query they're consumed by, and queries are run via loadQuery / usePreloadedQuery with no capability to tell relay to "also send along these headers".

We've ended up with a large list of features enabled for all queries, because we can't easily distinguish which query needs which experimental feature enabled.

headers: {
    'Content-Type': 'application/json',
    'X-ExperimentalApi': [... all the features ...].join(","),
}

Essentially invalidating the original desire for the @beta annotation to make it clear that you shouldn't rely on a field being mature.

We've had problems with teams unknowingly consuming exploratory fields because of this.

Usage annotation

We're now looking to improve on this with a usage annotation

directive @experimental(name: String!) on FIELD

fragment Project_data on Project {
    name
    team @experimental(name: "teams")
}

Which allows us to ensure you can't accidentally use a @beta field

We have 2 possible ways to implement that

Treat this as a client-only directive, and add a transform to Relay's compiler to collect these experimental values for each query, and make them available in it's query metadata for us to send as a header
- No change from our current server situation
- Requires Relay compiler support
- Only solves this problem for relay, but it's universal
The server can check for an @experimental directive matching the feature name when evaluating (or when validating a query for) @beta fields
- Slight overhead in the query, when there's many requests for the same field
- Assists attribution back to the call-site, backends devs can see where in a query it's being used, and map that back to client code

Tooling / Introspection

Not having to abuse the description for this would be nice, so I like the idea of it being exposed via introspection in some way.

We already use GraphiQL's "Request Headers" input to manually add the "X-ExperimentalApi" header, but buttons or checkboxes would be nicer

Bikeshedding

We've determined we want a different directives for the schema annotation than the query annotation, to keep them forward-safe if we determined we needed more than just a name. I used what we have (@beta in the schema) and what we've been discussing (@experimental in the query) above. @optIn is nice. Maybe @requiresOptIn (schema) / @optIn (query) makes sense?

When using a header, the name was necessary to align the schema field with the usage. But when you annotate both sides, you no longer have the same need for a name. Although it may does continue to prevent people from slapping '@experimental' on every field in their query out of laziness

Behaviour

We have a bit of an open question of what to do when someone doesn't opt-in on the query side. Right now, we fail the whole query (doesn't validate). But throwing an error or returning null for just that field would be a better way to minimise the blast-radius. One small part of the query being able to break the whole thing is fairly counter to the ideals of colocating and being resilient to partial failures.

If we failed per-field, we might consider injecting errors (chaos-monkey style) to experimental fields to actively encourage them to mature, rather than allowing them to stagnate in the experimental state.

bbakerman commented 2 years ago

Tangential to this particular experimental directive but the fact that the graphql spec does not allow "directives" to be listed on their usage sites (fields / types etc...) means that all the directive behavior has to be specified and built a priori into graphql engines.

For example @deprecated is NOT know about in a tool like graphiql because a field has the directive BUT rather the graphql engine had to know a priori to mark the field in memory as isDeprecated.

Without directives being available on fields / enums etc, I think it forces to much "implementation behavior" to HAVE to be in the graphql engine and not say in how the outside tooling might decide to treat it.

In graphql-java we went beyond the spec and allowed introspection to show "applied directives" on the places they are used. (its opt in right now but we may make it default behavior because its super useful)

So for example the documentation that @tomgasson mentioned could be "auto generated" by an external documentation tool because @beta was present in the introspection data or in the future @experimental / @requireOptIn et al.. say.

rivantsov commented 2 years ago

@bbakerman - 100% agree. This was one of the points of my presentation on GraphQL issues at the end of June meeting - to make introspection equivalent to SDL file, both have the same info; intro for tools, SDL for humans. You are right, all these headaches of accommodating new directives would be gone if we had this.
I will keep pushing for this, will publish more detailed issues/discussions, pls keep an eye on this - and support it

kevincianfarini commented 9 months ago

Hi. I want to voice my support for this proposal. I currently work at a company which maintains an API that is not only consumed by internal clients, but also by third party clients which we license our server software to.

I am trying to convince our back end engineers to adopt a habit of deprecating APIs before removing them and a general hygiene of avoiding abrupt breaking changes. One of the arguments against this that I for see will be our inability to iterate quickly on APIs while enforcing a deprecation cycle. Until now we've benefitted a lot by the agility of only consuming our APIs internally, but only recently has the need for stable APIs arisen.

Introducing an opt-in mechanism will allow my organization to iterate quickly on unstable APIs by dog fooding them with internal clients, while cautioning that third parties will be accepting risk by using them. Essentially the highly cohesive environment between client and server teams within our organization really benefits from a mechanism like opt-in, while still preserving API stability for clients who are more risk averse.

I hope this perspective is valuable and encourages the adoption of this proposal.

ccbrown commented 8 months ago

How does this address experimental types? For example, consider the example currently in the RFC:

type Session {
    id: ID!
    title: String!
    # [...]
    startInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
    endInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
}

If Instant is only used by fields that require opt-in, should it be included in introspection results?

More importantly, if a type itself is experimental, how should an API mark it as such to prevent others from accidentally using it from existing, stable fields? Consider an API with a node field:

type Query {
    # [...]
    node(id: ID!): Node
}

Node is an interface implemented by globally identifiable objects. If one of the experimental APIs adds such an object, how do we prevent users from using it via Node or other interfaces that it implements?

{
  node(id: "foo") {
    # Foo is experimental! We may want to rename or delete it before it 
    # stabilizes. Without opting in, users shouldn't be able to write a 
    # query that would be broken by those changes.
    ... on Foo {
      id
    }
  }
}

It seems to me like we might want to consider:

Also supporting requiresOptIn on types.
Removing the includeRequiresOptIn argument from the introspection schema. Features to be included can be specified in the same way as for other requests and the returned types can be filtered accordingly. This ensures that the returned schema information always represents a valid, coherent schema.

As an unrelated point, I don't see how the current proposal of the requiresOptIn field on __Field in the introspection API is useful. Unless I'm missing something, if a field requires opt-in, it's not going to be possible to access that requiresOptIn field unless you already know the required features. For feature discovery via the introspection API to be possible, it seems like there needs to be a way to enumerate all of the features defined by the schema:

type __Schema {
    # [...] other fields omitted for clarity

    optInFeatures: [String!]!
}

Edit: I see some of these concerns were brought up in https://github.com/graphql/graphql-wg/pull/1006, but it doesn't seem like they were really addressed.

martinbonnin commented 8 months ago

@ccbrown lots of excellent points!

Re: @requiresOptIn on objects, this reminds me of the @deprecated discussion (see https://github.com/graphql/graphql-spec/pull/997) and how GraphQL started with deprecated fields only to then include input values and now (potentially) objects and (potentially too) scalars, input objects and enums.

The reason I did not include them initially was that I copy/pasted the @deprecated directive definition to keep the symmetry but if @deprecated is going to be allowed on types as well, we might as well do it for @requiresOptIn.

Would you include all types?

directive @requiresOptIn(feature: String!) repeatable
on FIELD_DEFINITION
    | ARGUMENT_DEFINITION
    | INPUT_FIELD_DEFINITION
    | ENUM_VALUE
    # new values to support experimental types
    | SCALAR
    | OBJECT
    | INTERFACE
    | UNION
    | ENUM
    | INPUT_OBJECT

The @deprecated PR mentions only OBJECT but feels like we might want to allow all of them?

ccbrown commented 8 months ago

I would include all types. Theoretically, a server could be clever and infer the required features for all types other than object, but I think asking SDL authors to explicitly add the directive to all experimental types is better. It's clearer and more future-proof in the event that spec changes allowing interfaces and unions to implement interfaces are made.

Once types are included, there should be a list of rules established for schemas, which can all be validated statically:

Fields on interface implementations must require a subset of the features required by the corresponding interface field.
Objects and interfaces must always contain at least one field whose feature requirements are a subset of their own.
Unions must always contain at least one member whose feature requirements are a subset of their own.
Enums must always contain at least one value whose feature requirements are a subset of their own.
Field types must never require features that aren't required by the field itself.
Field argument types must never require features that aren't required by the argument itself.

These rules would ensure that no matter what combination of features is used, the schema is always perfectly conformant.

I'm in the process of building out a server implementation in https://github.com/ccbrown/api-fu/pull/69 and this is the direction I'm currently taking. So far it seems to work well.

graphql / graphql-spec