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 a concept of annotations to GraphQL #265

Closed
wants to merge 1 commit into from

Conversation

igorcanadi
Copy link
Contributor

This pull requests adds a new feature to the language -- annotations for fields in type definitions and extensions. For example, now you're able to do this:

type Hello {
  @mock(value: "hello")
   world: String
}

We need annotations to enable the mock functionality we're building -- we'd like product developers to quickly specify some mock data on top of which they can build UI. That way we decouple building UI and server endpoints.

@leebyron
Copy link
Contributor

This is cool, the idea has come up before - see the discussion at #114

There are two major issues with this particular approach that I can see:

The first is just naming. Directives imply runtime behavioral instructions, and I'm not sure what "concrete argument" means in this context. In other systems that introduce a concept like this, it's usually called "metadata" or "annotation" or something along those lines.

The other is syntactic clarity. While directives are useful for small bits of data inline within the query, they can be problematic when describing metadata that can be quite long. Some other proposals have been to embed metadata in comments, or to investigate other syntactic additions which are more amenable to the kinds of things we could see in metadata. Another issue for syntax clarity is the position relative to the fields or types or other elements they describe. For directives positioning directly after the token on the same line was easy to read, but for most metadata this probably isn't the ideal position. Instead most metadata is longer form and probably belongs directly above a field or type on it's own line.

@igorcanadi
Copy link
Contributor Author

The first is just naming. Directives imply runtime behavioral instructions, and I'm not sure what "concrete argument" means in this context. In other systems that introduce a concept like this, it's usually called "metadata" or "annotation" or something along those lines.

This makes sense. I can do s/directives/annotations in the diff.

The other is syntactic clarity. While directives are useful for small bits of data inline within the query, they can be problematic when describing metadata that can be quite long. Some other proposals have been to embed metadata in comments, or to investigate other syntactic additions which are more amenable to the kinds of things we could see in metadata. Another issue for syntax clarity is the position relative to the fields or types or other elements they describe. For directives positioning directly after the token on the same line was easy to read, but for most metadata this probably isn't the ideal position. Instead most metadata is longer form and probably belongs directly above a field or type on it's own line.

I like the idea of adding annotations before the tokens (as in #180). This is also consistent with Java's and Python's annotations. I see you had some concerns about that syntax here: #180 (comment). One way to address your concern is to disallow adding post-fix annotations in the same line (i.e. require \n before the annotation). What do you think about that?

I'm not sure how I feel about annotations in comments. If you want to add annotations like @deprecated or @doc, then sure. But if you have annotations that actually change the behavior (such as @mock), then it's a bit weird to me.

Let me know what you think. Also, cc @fson.

@clintwood
Copy link

Very keen to see annotations in GraphQL!

Wouldn't something like @@ or @! be a better prefix for annotations (seeing that @ is already taken - @ should really have been left for annotations ;), something like @! would have been better for a directive in my humblest opinion). Enforcing a \n before the annotation doesn't seem right. Also agree that having annotations in comments is weird.

@igorcanadi
Copy link
Contributor Author

Wouldn't something like @@ or @! be a better prefix for annotations (seeing that @ is already taken - @ should really have been left for annotations ;), something like @! would have been better for a directive in my humblest opinion)

Yup I agree that it'd be better to have a different prefix for annotations. I don't care which prefix in particular we end up using.

@igorcanadi igorcanadi changed the title Implement field directives Add a concept of annotations to GraphQL Dec 16, 2015
@igorcanadi
Copy link
Contributor Author

I pushed the new commit, with the following changes:

  1. s/directives/annotations. Now annotations are a completely separate concept from directives.
  2. annotations are specified before the field definition.

Currently the prefix is still @, but I will change it once we get consensus.

@freiksenet
Copy link
Contributor

How will multi-line strings be handled? Eg. dostrings?

@igorcanadi
Copy link
Contributor Author

How will multi-line strings be handled? Eg. dostrings?

They are currently not supported, but if we allow any string constant to be multi-line, annotations would get multi-line arguments for free. See the example: #114 (comment)

If we go that route then the two features are orthogonal.

@freiksenet
Copy link
Contributor

@igorcanadi yes, makes sense. There has been also discussion about using comments for stuff like docstrings anyway.

Looks great, can't wait to have this feature!

@fson
Copy link
Contributor

fson commented Dec 16, 2015

As seen in #114, I used to think annotations should also be used for descriptions (with docstrings), but now I believe that a separate syntax based on comments would be better suited for that purpose. Descriptions can be long, multiline, markdown and include code examples and more. Although using the comment syntax has some complexities from implementation point of view (parser can't just ignore all comments anymore and it needs to distinguish between comments and descriptions), it can be more developer friendly than multiline strings:

# This field returns the *world*.
#
# _Example:_
# ```
# code example
# ```
world: String

# vs

@description(text: """
  This field returns the *world*.

  _Example:_
  ``
  code example
  ``""");
world: String

So I agree the multiline strings are orthogonal to the annotation feature.

That said, I think for things like @igorcanadi's mocks or the metadata we need for Reindex schema (to express relationships between types), annotations make a lot of sense.

I think treating annotations as a first class feature instead of embedding them inside comments does have some benefits. If we use comments for descriptions, we should be able to parse them with a markdown parser. But if we would put annotations inside description comments, they would show up in the description and also could interfere with markdown in unexpected ways, consider this:

# This is the *description*.
# mock: "__foo__"
description: String

which would be rendered in documentation browser as:

This is the description.
mock: "foo"

Dedicated annotation syntax also allows the arguments to use all the input types provided by GraphQL, such as String, Int and even input objects or lists. Annotations inside comments are just dumb strings (unless we design a whole new set of syntax rules for parsing them).

@clintwood
Copy link

Ugh, I need these field annotations so badly :(

Currently I'm defining metadata in a config file based on type and field names and grabbing it during resolve.

Any movement on this @igorcanadi?

@igorcanadi
Copy link
Contributor Author

Hey @leebyron, any chance you have the time to review this PR? I need it to continue developing the mocking functionality. I'm also happy to discuss offline if you have any concerns.

@leebyron
Copy link
Contributor

leebyron commented Jan 4, 2016

This is at the top of my list now getting back to things in 2016!

@almilo
Copy link

almilo commented Jan 28, 2016

Hi @leebyron any progress on this PR?
Many thanks for making GraphQL OS! It is just awesome and with metadata annotations it can become even more :)

@josephsavona
Copy link
Contributor

This could be incredibly helpful for Relay or any other client that needs to understand how to interpret a query response. Examples include knowing which fields are connections, specifying the semantic meaning of arguments (i.e. specifying that "first" does mean first-n-things), or perhaps configuring a custom resolver for some field (so that an app can perform sorting/filtering client-side without a round trip).

@clintwood
Copy link

@josephsavona, (@igorcanadi, @leebyron), incredibly useful - the specific use case you mention regarding "specifying the semantic meaning [through annotations]" is what I'm looking for - as mentioned earlier I'm having to maintain a hacky custom metadata mechanism to effectively provide annotations - it's becoming a maintenance issue - placing this metadata directly where it's needed would be a huge win!

@almilo
Copy link

almilo commented Feb 28, 2016

In the mean time, I have built a home-brewed regex-based annotation extractor to demonstrate some usages of annotations like making a client schema (text file) executable by adding run-time behaviour (implementation).

Running example:

It took me almost no time to write the annotated schema to turn the REST api into a GraphQL endpoint and that is just a usage of many that I could imagine. @leebyron any feedback on progress?

@josephsavona
Copy link
Contributor

@almilo that's another great use-case - I'd love to see a tool that took .graphql files with REST annotations as input and generated a JS schema as output that used the annotations to fulfill fields from the REST backend. This could be super useful for trying out Relay without having to run a GraphQL schema on the server - the generated schema could be run client-side and talk to existing REST endpoints, helping developers to see the value of the combined GraphQL & Relay stack with low investment.

@almilo
Copy link

almilo commented Feb 28, 2016

@josephsavona so you mean you want to generate a '.js' file and not only the runnable schema as done here: https://github.com/almilo/annotated-graphql/blob/master/src/annotated-graphql-schema-factory.js#L13 ?
I took this approach from another project (graph.ql) and I will work on simplifying it by using the graphql-js utilities as much as possible. So I could maybe give a try at your approach and implement such a tool.

@clintwood
Copy link

EDIT: Sorry wrong person [@josephsavona]

@igorcanadi, is this PR still up to date? I tried merging it in my own custom build some weeks ago but gave up for issues I was having (the detail of which I now forget). I have use cases not too different from what's being discussed above. I'd be happy to have my own fork of GraphQL and merge this feature right now but I'm not sure of the state of this PR!

@clintwood
Copy link

@leebyron, @igorcanadi, Because I need these annotations now, I have created an annotations fork of the GraphQL repo to include annotation similar to what is discussed here and to bring everything up to the latest commits in the repo. I have borrowed a lot from this PR, however, the differences in my implementation include:

  • Used a double @@ as the token for Annotations (TokenKind.ATAT)
  • Annotations are supported on OperationDefinitions, FragmentDefinitions and FieldDefinitions
  • The parser also parses annotations on selection fields (not sure if that was an intentional omission or if I've possibly misunderstood the intent here)
  • A few more tests to cover additional annotation features introduced (probably need more though).

@igorcanadi, please feel totally free to use whatever from the annotations fork to update this PR - thanks for your initial effort!

leebyron added a commit that referenced this pull request Mar 17, 2016
This proposes a change to how we represent the ability to validate the locations of directives via introspection.

Specifically, this deprecates `onField`, `onFragment`, and `onOperation` in favor of `locations` which is a list of `__DirectiveLocation`.

**Rationale:**

This allows for a more fine-grained validation of directive placement, now you can assert that a directive is allowed on queries but not mutations, or allowed on fragment definitions but not on fragment spreads.

Also, this makes expanding the locations a directive is allowed to be placed easier to do, as future expansions will not affect the introspection API. This should be considered a prereq to #265.

Finally, this is a prereq to a forthcoming RFC to add directives to the type schema language, one of the last missing pieces to represent a full schema using this language. Currently considering something like:

```
directive @Skip(if: Boolean) on FIELD, FRAGMENT_SPREAD, INLINE_FRAGMENT
```

**Drawbacks:**

Any change to the introspection API is a challenge. Especially so for graphql-js which is used as both client tools via Graph*i*QL and as a node.js server.

To account for this, I've left the existing fields as deprecated, and continued to support these deprecated fields in `buildClientSchema`, which is used by Graph*i*QL. While graphql-js will likely continue to expose these deprecated fields for some time to come, the spec itself should not include these fields if this change is reflected.
@leebyron
Copy link
Contributor

@clintwood I'd love to better understand your goals. I'm working through the dependencies to get this done now and looking at what we might borrow from your work but am a little confused as it looks like what you've built is orthogonal to what @igorcanadi is proposing - your annotations are being applied to fields in a requested query (where we already supply directives for this purpose) and @igorcanadi is proposing annotations to the schema itself.

Could you help me understand the goal you are trying to achieve and why directives didn't solve them?

@leebyron
Copy link
Contributor

The more I've thought about this proposal, the more I think that it should be an extension of directives rather than a new concept. It's so similar both in syntax and in purpose that having two concepts that are subtly different is probably not a good idea. It's maybe a little too late to consider renaming "directive" to "annotation" since we've baked it into the introspection system and many rely on that, I'm open to suggestions on that front.

To back up and list out the goals we seek to solve with this proposal and directives in general:

1) The annotation of elements within a GraphQL operation (query) as indications that it a GraphQL service should alter its execution behavior.

This is the original motivation for directives, both for the directives that are defined by the spec (@skip and @include) as well as future proposals (@defer, @stream, @live).

2) The GraphQL schema language is used (at Facebook) to define client-side extensions to the schema for the purposes of code-generation or tighter integration with client-side native capabilities.

This as I understand it is @igorcanadi's motivation with @mock, allowing a prototype GraphQL environment that requires no server. In order to accomplish this, only the proposed syntax needs to be changed, not necessarily the schema API and introspection results.

I've also proposed #318 as a way for clients to define what client-side directives are possible, how to use them, and where they're allowed to be used.

3) The GraphQL schema, as exposed via introspection, could also contain additional metadata. Additional metadata could help server implementers provide more information so tools like Relay could be aware of more semantics and do smarter things.

This is what I believe @josephsavona is proposing. This necessitates a change to the schema API and introspection results, but not necessarily the schema language syntax - the syntax must change only because the schema language is intended to ultimately fully represent a schema.

I think most of my open questions are here. Should metadata on types and fields be freeform? If so then my proposals in #317 and #318 are misaligned with this goal. Should metadata be structured? If so, then where should this structure live?

Perhaps what would be most helpful is motivating examples for this third goal so we ensure that whatever we land on feels nice for those cases.

4) The schema language should capture descriptions.

I think @fson's suggestion is fitting here. Putting descriptions in doc-block style comments is gross from the parser's point of view, but clearly better for developer ergonomics and has obvious precedent in a zillion other languages.

@leebyron
Copy link
Contributor

In doing some research to find similar proposals, I think Cap'n Proto's Annotations are very close to what we're doing here. https://capnproto.org/language.html#annotations

@clintwood
Copy link

@leebyron, I'm proposing annotations against both schema and query language constructs. My main use case is dynamic query composition client side and build time preprocessing via babel transforms of requests for client side and server side.

So I see annotations as distinct from directives. I'm not convinced that annotations should be considered an extension of directives (specifically in terms of behavior). They have different use cases (as discussed below) and should be made distinct from one another.

Directives should be focused on execution behavior, should be able to alter the execution behavior of GraphQL services that could alter returned results, specifically for those directives documented (@Skip, @include and the proposed @defer, @stream, @LiVe) in the Facebook spec and optionally for user defined directives. New directives should therefore be considered carefully and implemented sparingly since service implementers will need to consider these in their implementations. Directives are currently well understood and the proposed directive location changes look great. Also the actual placement (location in the script) after name is sensible for directives but not really for annotations

Annotation should be focussed on metadata, should not alter execution behavior of GraphQL services. Annotations should be available in the GraphQL schema language as well as in GraphQL query language. This metadata can then be accessed by users via the query AST (parsed request) or schema definition for userland processing. One such use case could be dynamic composition of queries, mutations, fragments based on metadata associated with query scripts. e.g. (assuming @@ as the annotation prefix for the sake of these examples)

Example annotation in GraphQL schema definitions

`
// used to mark key and add some additional
// metadata for client side checks
interface MyID {
  @@key(maxlen: 128)
  id: String
}

Example annotations use in GraphQL query scripts:

// when used client side the user can adjust
// local  caching per annotation metadata
// likewise id is marked as the key via metadata
let someQuery =
`
@@clientCache(ttl: 3600)
query viewer {
  @@key
  id
  name
}
`;
// 
let someFragment =
`
@@fatQuery
fragment on LikeStory {
  story {
    likers {
      count,
    },
    likeSentence,
    viewerDoesLike,
  },
}
`;

I can think of many client side and server side use cases for annotations as described above including preprocessing using babel transformations that could make good use of this metadata.

To me annotation feel closer to comments whereas directives feel closer to language constructs.

Hope this makes sense!
[Edit] BTW I haven't fully implemented annotations on GraphQL schema yet and will be happy to do so and provide a separate PR if there is interest in pursuing this further...

@helfer
Copy link
Contributor

helfer commented May 4, 2016

I'm thinking of creating a PR for the following proposal for adding decorators (aka annotations) over the next couple of weeks: https://github.com/apollostack/graphql-decorators/blob/master/README.md

I think it's a reasonable way of adding arbitrary metadata to GraphQL schemas while ensuring consistency, correctness and compatibility between different implementations of the GraphQL spec.

@igorcanadi
Copy link
Contributor Author

Closing in favor of #376.

@igorcanadi igorcanadi closed this May 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants