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

Add OptInFeatures.md #1006

Merged
merged 7 commits into from
Jun 6, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
204 changes: 204 additions & 0 deletions rfcs/OptInFeatures.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
# RFC: Opt-in features

**Proposed by:** [Martin Bonnin](https://twitter.com/martinbonnin)

This document is a work in progress. A lot of the questions about introspection are closely related
to [#300](https://github.com/graphql/graphql-spec/issues/300) (`Expose user-defined meta-information via introspection API in form of directives`)
and will therefore need to be revisited based on the progress there.

## 📜 Problem Statement

GraphQL has a [built-in mechanism for deprecation](https://spec.graphql.org/draft/#sec--deprecated) allowing to
gracefully remove features from the schema. The lifecycle of a feature can typically be represented as `stable`
-> `deprecated` -> `removed`.

In a lot of cases though, a feature lifecycle includes an experimental phase where it has just been added and can be
changed without warning. In this state, the feature is usable and feedback is encouraged but isn't considered stable
enough to be put in production. The feature lifecycle becomes `experimental` -> `stable` -> `deprecated` -> `removed` .

### Goals

The goal of this proposal is to support the `experimental` state and, moving forward, any state that requires the client
developer to make an explicit decision before using a given feature. In that sense, it's about "opting in" to using the
feature, which includes supporting `experimental` states.

To give a few examples:

* a field is experimental and might be changed or removed without prior notice (the above example).

* a field is expensive to compute and should be used with caution.

* a field has specific security requirements and requires a specific header or other form of authentication.

### Non-goals

This proposal is not about security and/or hiding parts of a schema. Its goal is to make it easier to communicate opt-in
features to client developer and therefore needs to expose that information.

## 👀 Prior work

* GitHub uses [schema previews](https://docs.github.com/en/graphql/overview/schema-previews) to opt-in new features.
* Kotlin has [OptIn requirements](https://kotlinlang.org/docs/opt-in-requirements.html) that started out
as `@Experimental`
before [being changed to `@OptIn`](https://youtrack.jetbrains.com/issue/KT-26216/Generalize-Experimental-API)
* Atlassian
has [a `@beta` directive](https://developer.atlassian.com/platform/atlassian-graphql-api/graphql/#schema-changes) that
is enforced during execution. A client must provide a `X-ExperimentalApi: $Feature` HTTP header or the request will
fail.

## 🧑‍💻 Proposed solution

### The `@requiresOptIn` directive

It is proposed to add an `@requiresOptIn` directive to the specification:

```graphql
"""
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could @optIn also apply to type definitions since you might want to expose entirely new types in an experimental feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly? I copy/pasted this list from @deprecated because I started with @experimental being the counterpoint of @deprecated but as we're moving away from this I think it makes a lot of sense.

I guess it has implications in terms of validation though. If a type is @optIn("foo"), should all the usages be optIn("foo") too? That can make a lot of directives?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about type definitions.

You can query over a union or interface, which could mean you get one of many types. Specifying what a server would have to do if you hadn't opt-ed in based on the runtime value makes this much more complex.
I think having a decidable result based only on schema & operation (and regardless of the value) is desirable.

For the same reason, I'm also not sure about ENUM_VALUE

ARGUMENT_DEFINITION and INPUT_FIELD_DEFINITION make sense from a static validation perspective, but I'm unsure if there's real-world need for them.

At the field level it's often things like "this it's hitting a t1.nano and not ready for 1 million users". Experimental input fields and arguments are more likely about business logic and validation correctness. If they are truely doing something newly experimental they could create a new field instead. I'd love to see some examples of situations where this would be a useful addition

```

The `optIn` directive can then be used in the schema. For an example, to signal an experimental field:

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

### Introspection

> This section is a proposal based on the current introspection mechanism. A more global mechanism (
> see [#300](https://github.com/graphql/graphql-spec/issues/300)) would make it obsolete

`@requiresOptIn` features should be hidden from introspection by default and include if `includeRequiresOptIn` contains the
given feature:

```graphql
type __Type {
kind: __TypeKind!
name: String

# [...] other fields omitted for clarity

# includeRequiresOptIn is a list of features to include
fields(includeDeprecated: Boolean = false, includeRequiresOptIn: [String!]): [__Field!]
}
```

Tools can get a list of `@requiresOptIn` features required to use a field (or input field, argument, enum value)
using `requiresOptIn`:

```graphql
type __Field {
name: String!
isDeprecated: Boolean!

# [...] other fields omitted for clarity

# list of @requiresOptIn features required to use this field
requiresOptIn: [String!]
args(includeDeprecated: Boolean = false, includeRequiresOptIn: [String!]): [__InputValue!]!
}
```

A given field is included in introspection results if all the conditions are satisfied. In pseudo code, if the following
condition is true:

```
includeRequiresOptIn.containsAll(field.requiresOptIn) && (includeDeprecated || !field.isDeprecated)
```

### Validation

Similarly to [deprecation](https://spec.graphql.org/draft/#sel-FAHnBZNCAACCwDqvK), the `@requiresOptIn` directive must
not appear on required (non-null without a default) arguments or input object field definitions.

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

## 🗳️ Alternate solutions
Copy link

@tomgasson tomgasson Jun 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative proposal: @requiresOptIn and @OptIn

Similar to the Proposed solution, except the schema uses @requiresOptIn instead

directive @requiresOptIn(feature: String!) on FIELD_DEFINITION 
type Session {
  id: ID!
  title: String!
  startInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
  endInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
}

Operations use @optIn

directive @optIn(feature: String!) on FIELD 
query {
  session {
    id
    title
    startInstant @optIn(feature: "experimentalInstantApi")
    endInstant @optIn(feature: "experimentalInstantApi")
  }
}

Adding @requiresOptIn to an existing field would need to be detected as a schema breaking change.
Removing @requiresOptIn is safe, and an operation using @optIn on a field without @requiresOptIn would continue to work.
Tools can use introspection values to provide code actions to add @optIn where necessary
Matching the feature prevents misuse, for example:

query {
  session @optIn {
    id @optIn
    title @optIn
    startInstant @optIn
    endInstant @optIn
  }
}

For introspection, unlike the Proposed solution the schema is telling you about the opt-in features, rather than you having to provide them

 type __Type {
   # [...] other fields omitted for clarity
   fields(
     includeDeprecated: Boolean = false,
+    includeRequiresOptIn: Boolean = false
   ): [__Field!]
 }
 
 type __Field {
   name: String!
   # [...] other fields omitted for clarity
   isDeprecated: Boolean!
   deprecationReason: String
+  requiresOptIn: Boolean!
+  optInFeature: String
 }

Pros:

  • Self-documenting. There's no set of feature names defined somewhere else
  • Usage is colocated with consumption
  • No grammar changes
  • No need to enumerate all opt in features

Cons:

  • Only uses a string, no capability for providing more type information
  • Goes further than just being documentation to somewhat imply behaviour (although that could still be for server to decide)
  • No place to document a feature itself (i.e one feature might apply to many fields, and want a high-level description of the feature itself)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative proposal: @requiresOptIn

Woops, sorry I missed this earlier today, I actually just pushed @requiresOptIn, I like this a lot, it plays well with @optIn and is more explicit

Operations use @optIn

Do we need to put that in the spec? This has execution implication and different teams might want to implement that differently.

I initially envisioned the proposal as a way to "communicate" opt-in requirements (like @deprecated communicates the deprecated status). "Enforcing" it seems another steps with a wide range of implications.

For introspection, unlike the Proposed solution the schema is telling you about the opt-in features, rather than you having to provide them

Not sure I get this one. The current proposed solution already exposes the opt-in features:

type __Field {
  name: String!
  isDeprecated: Boolean!

  # [...] 

  # list of @requiresOptIn features required to use this field
  requiresOptIn: [String!]

We can use an additional Boolean but it feels like duplication because we can tell if there's the need for duplication if the list is not empty.

On a separate (but related) note, I wish isDeprecated would be replaced by checking for deprecationReason != null.


### `@experimental` directive

```graphql
# Indicates that the given field or enum value is still experimental and might be changed
# in a backward incompatible manner
directive @experimental(
reason: String! = "Experimental"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE
```

Pros:

* simple
* symmetrical with `@deprecated`

Cons:

* doesn't account for opt-in requirements that are not experimental
* makes it harder to group by features. `reason` could be used for this but it is less explicit than `feature`

### marker directives

The spec defines a directive named @requiresOptIn (and in doing so introduces the need to be able to apply directives to
directive definitions)

```graphql
directive @requiresOptIn on DIRECTIVE_DEFINITION
```

Services create a directive for each distinct opt-in feature they want in their schema:

```graphql
# optIn usage defines @experimentalDeploymentApi as an opt-in marker
directive @experimentalDeploymentApi on FIELD_DEFINITION @requiresOptIn

type Query {
deployment: Deployment @experimentalDeploymentApi
}

enum WorkspaceKind {
CROSS_PROJECT
CROSS_COMPANY
}
directive @workspaces(kind: WorkspaceKind) on FIELD_DEFINITION @requiresOptIn

type Deployment {
workspaces: [Workspace] @workspaces(kind: CROSS_COMPANY)
}
```

Pros:

* gives more control to the user about the directive used
* has more type information

Cons:

* more complex
* requires a grammar change

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble understanding this mechanism. Here's what I gather:

The spec defines a directive named @optIn (and in doing so introduces the need to be able to apply directives to directives)

directive @optIn on DIRECTIVE_DEFINITION

Services create a directive for each distinct opt-in feature they want in their schema

directive @experimentalDeploymentApi on FIELD_DEFINITION @optin

type Query {
 deployment: Deployment @experimentalDeploymentApi
}


enum WorkspaceKind {
 CROSS_PROJECT
 CROSS_COMPANY
}
directive @workspaces(kind: WorkspaceKind) on FIELD_DEFINITION @optin

type Deployment {
 workspaces: [Workspace] @workspaces(kind: CROSS_COMPANY)
}

Is my understanding correct?

  • Do clients do something to consume this? Or is it just documenting?

  • It's not clear what extra information would be useful to capture. Some examples might help clarify

  • This appears to hinder the ability to do out-of-band feature opt-ins (HTTP headers) because you don't have an identifier to use. E.g.

headers: {
  "X-ExperimentalApi": "experimentalDeploymentApi"
}

Copy link
Contributor Author

@martinbonnin martinbonnin Jun 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is my understanding correct?

100% correct. I took the liberty of reusing your wording as it's much clearer 👌 and pushed it to the proposal, I hope you don't mind?

Do clients do something to consume this? Or is it just documenting?

Simply documenting. See also my other comment.

It's not clear what extra information would be useful to capture. Some examples might help clarify

To me, the main advantage is that tools can get the list of opt-in features by scanning the directive definitions. They don't have to scan each field and infer unique feature names. So it's a bit easier on the tooling side.

As a side effect, it also allows users to add arguments to their directives (your WorkspaceKind above) although at this point, I'm not sure at all there are use cases for this.

This appears to hinder the ability to do out-of-band feature opt-ins (HTTP headers) because you don't have an identifier to use. E.g.

You can always use directive name. Your example would actually work

headers: {
  "X-ExperimentalApi": "experimentalDeploymentApi"
}


## 🪵 Decision Log

This proposal started out with a very simple premise and implementation, and has gotten more complex as
the community has explored edge cases and facets about how GraphQL is actually used in practice.

This decision log was written with newcomers in mind to avoid rediscussing issues that have already been hashed out,
and to make it easier to understand why certain decisions have been made. At the time of writing,
the decisions here aren't set in stone, so any future discussions can use this log as a starting point.

### directive name

Initially, the directive name was `@experimental` then `@optIn` to account for other use cases than just experimental
status before [settling on `@requiresOptIn`](https://github.com/graphql/graphql-wg/pull/1006#discussion_r889467023)
because it is both more explicit and leaves room for clients to use an `@optIn` directive.