[RFC] Persisted Document support #109
Conversation
ghost
added
the
|
I am not the greatest JS engineer, so basic/style feedback is expected and welcome. |
| request: Request, | ||
| response: Response | ||
| ): Promise<OptionsData> { | ||
| return new Promise(resolve => { |
There was a problem hiding this comment.
No need for new Promise:
return Promise.resolve(
typeof options === 'function' ?
options(request, response) :
options
).thenThere was a problem hiding this comment.
tried that, but doing so prevents us from catching exceptions thrown from options when it is a function at [1]
|
@alangenfeld @wincent @josephsavona why Document can't have variables for fetching. According to provided variables, we may get the absolutely different responses for same document (query) id. I think better naming is |
|
Yeah, this is a tricky nomenclature issue. The parameter is I'm not sure Document is ambiguous in this context, since it refers to http://facebook.github.io/graphql/#sec-Language.Query-Document (or to the |
|
@dschafer please consider better name for flowtype parameter in graphql-js. |
| returns an id that can be used to load it later. This is used in conjunction | ||
| with `loadPersistedDocument`. Providing this function will enable persisting | ||
| documents via the `document` parameter at the `/persist` subpath. It is | ||
| recommended that this option only be enabled in development deployments. |
There was a problem hiding this comment.
It's not clear to me when you would want to use persistValidatedDocument in development. You basically have to be storing the persisted documents either in some database, or in your source code, right? And either way, hitting a persistValidatedDocument REST API on a development backend server is not going to be the easiest way to get these persisted documents into the place they need to go. In your development environment you can just write them to the db or put them in your source code directly.
There was a problem hiding this comment.
If you want to build tooling for client development, you might want to include persisting all required client queries into your build process and have an API for this, making this interface HTTP is pretty obvious.
Persisting them to a database has the nice benefit (especially for larger organizations) that you don't need a new server deployment after adding (updating) a new query. You could even imagine that Github's public GraphQL API allowed clients to persist queries.
There was a problem hiding this comment.
You would have to be running your backend server during your build process, though - isn't that somewhat unusual? Rather than writing some backend code to persist the document, and making your build process hit an HTTP API on your running backend server, your build process could just execute the code to persist the document directly. I guess overall it seems like this is something people could implement themselves just as easily as if we added a persistValidatedDocument API.
There was a problem hiding this comment.
I don't think it's unusual at all - in fact it seems the opposite to me. When building my iOS app or Android app or web app, I'd be really surprised if in that context I couldn't access my GraphQL API because it was also being built. I would expect a GraphQL API to be decoupled from the apps that are using it such that the build and deploy processes are also separate. Typically the build process for an iOS, Android or web app does not have access to a database, which is why exposing an HTTP API is beneficial.
| @@ -53,6 +53,16 @@ The `graphqlHTTP` function accepts the following options: | |||
| * **`graphiql`**: If `true`, may present [GraphiQL][] when loaded directly | |||
| from a browser (a useful tool for debugging and exploration). | |||
|
|
|||
| * **`loadPersistedDocument`**: A function that takes an input id and returns a | |||
| valid Document. If provided, this will allow your GraphQL endpoint to execute | |||
| a document specified via `documentID`. | |||
There was a problem hiding this comment.
These documents typically each have a unique name, right? Why not use the name as the identifier instead of a separate documentID? I think that is how Airbnb is setting up their graphql backend.
There was a problem hiding this comment.
While the document might have a unique name like HomePageQuery, this is not enough, as the query likely changes over time as new client versions are released. It is important that old clients still get the old version of HomePageQuery.
README.md
Outdated
| @@ -156,6 +166,22 @@ new GraphQLObjectType({ | |||
| }); | |||
| ``` | |||
|
|
|||
| ## Persisted Documents | |||
|
|
|||
| Putting control of the query in to clients hands is one of the primary benefits | |||
There was a problem hiding this comment.
in to -> into
clients -> clients'
|
@lacker the problem with using the document name as the identifier is that the document changes over time, the id serves as a hash for a rev of a particular document. |
alangenfeld
force-pushed
the
persisted-document
branch
from
370cdad
to
772a1cb
Compare
|
Apologies for the long delay, its been a hectic few months in GraphQL land at FB. Thanks everybody for the feedback so far. I think the questions that are still open are:
|
|
My suggestions + rationale for the open questions:
|
|
In apollo-server we called them "stored operations", since that encompasses queries, mutations and subscriptions. I know that technically they can also contain fragments, which makes them documents, but at least it's a less confusing name than "queries". Using "queries" may lead people to believe that mutations can't be persisted. |
|
Let's revive this and get it shored up and ready to land. Some thoughts: "Document" is absolutely the right name. We've been bad about clarity around names and in fact should consider renaming some of the fields accepted in this library to help with that. A Document is a whole GraphQL file or request. An operation is one query/mutation (a document may contain many in addition to fragments). A query is a kind of operation. Since we're typically talking about documents that have one operation that is a query, we say "GraphQL query" - but I think it's important to get the naming right for this which should allow persisting any kind of document. Perhaps we should rearrange the API such that you provide both or neither functions. Right now we could assert one or the other, but I think the API could help with that. Maybe: persistedDocuments: {
save: document => id
load: id => document
}I think we're missing an operation as well, which is getting the actual document text for a given id. Tools like GraphiQL would want to use this to populate the left pane. Maybe we should expose paths One last thought - should we be persisting the actual string text rather than the AST itself? Or perhaps allowing either with a |
I've been curious about the same thing for @github. Right now we're thinking of making our persisted IDs a SHA256 of the document text. Normalizing the document AST would let us store slightly different text documents under the same hashed ID. Seems like a good idea to me. Is there even a mechanize that would allow clients to retrieve the original document text? If they can only persist and execute operations, any comments wouldn't make a difference. |
|
I think it would be valuable to retrieve the original document text for use with tools like GraphiQL. It would be immensely helpful for debugging to be able to take one of these persisted IDs and check what the server is actually running for it. |
|
Also, if you'd prefer to have comments or formatting be irrelevant to the hash of a query, that's certainly something you could have a client decide by doing a |
|
Is there some value here to borrowing the prepared statement nomenclature from SQL? Many programmers are already familiar the concept of working with a statement in two phases, one phase to setup the statement and the second phase to execute it with parameters. The methods in this example might become prepareDocument and loadPreparedDocument? I wonder if "persist" terminology might require slightly more cognition to disambiguate. It is the communication pattern with the server that is being persisted, not necessarily the data being exchanged. |
| resolveOptionsData(options, request, response), | ||
| parseBody(request) | ||
| ]).then(initData => { | ||
| const [ optionsData, bodyData ] = initData; |
There was a problem hiding this comment.
travis.yml contains Node 0.12 with all tests currently passing. I believe destructuring isn't supported there.
|
I'll take over from here :) |
|
Thanks @leebyron! Apologies for not being able to see this one through. |
|
Is someone using approach: on dev environment support mix(persisted queries & arbitrary queries) and on prod environment support only persisted queries? |
|
What's the status on this PR? |
|
Any news? |
There was a problem hiding this comment.
Sorry for letting this sit. I'd love to have your help on this, @alloy.
From looking over the PR it seems that really this just needs a few things, repairing conflicts and cleaning things up a bit would be helpful!
I also think persistValidatedDocument should provide both the AST and the original query string, probably as (text, ast) => id so that the unedited text can be saved and a call to print be the user is unnecessary for most storage cases.
Similarly, It would be nice if loadPersistedDocument accepted a return type of text | ast and used typeof === 'string' to disambiguate
| let response = await request(app) | ||
| .post(persistString({ | ||
| document: '{test}' | ||
| })); |
There was a problem hiding this comment.
Need a test that posting data works instead of just formatting a query string
| if (showGraphiQL) { | ||
| return null; | ||
| } | ||
| throw httpError(400, 'Must provide query string.'); | ||
| throw httpError(400, 'Must provide query string or document ID.'); |
There was a problem hiding this comment.
Should probably only say document ID should be provided if loadPersistedDocument is available
| // Return 400: Bad Request if any validation errors exist. | ||
| response.statusCode = 400; | ||
| return { errors: validationErrors }; | ||
| if (query) { |
There was a problem hiding this comment.
These 3 if statements are a bit confusing in sequence, I would expect:
if (documentID) {
} else if (query) {
} else {
| @@ -278,7 +285,7 @@ function getGraphQLParams(urlData: Object, bodyData: Object): GraphQLParams { | |||
| // Name of GraphQL operation to execute. | |||
| const operationName = urlData.operationName || bodyData.operationName; | |||
|
|
|||
| return { query, variables, operationName }; | |||
| return { query, variables, operationName, documentID}; | |||
There was a problem hiding this comment.
formatting (note: we should enable prettier on this repo)
|
@leebyron Looks straightforward enough, thanks! 👍 I’ll start work on that this week. |
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status. |
|
let's make this happen in 2019? |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
I made many attempts at updating this PR, but the lack of context and it being so out of date made it hard :/ It’s definitely still on my TODO list, so yeah maybe this will be the year! |
|
this could be achieve in a middleware based way, so we don't need to make express-graphql more complex like this https://gist.github.com/robrichard/d5a6ae7ecb3c9efc7fad7b3236d255e7 solution by @robrichard using the middleware approach makes easy to work with express-graphql, koa-graphql, apollo-server and any other middleware based server |
|
@sibelius that seems like a great approach |
|
But it should be standardized, no? To guarantee interoperability between the different client and server libraries. Middleware or not is just an implementation detail, right? |
|
Yeah agreed that that could be a good approach 👍 I do think we may still need to add some change here (eg to be able to use already parsed and validated documents), but those should be easy to do from scratch. |
|
we could have another package to handle all this persisted queries validation and logic |
This adds support to express-graphql for GraphQL persisted documents. This is a feature we have discussed in ad-hoc ways since open sourcing GraphQL, and are now attempting to demonstrate more concretely. Persisted documents are foundational to how we use GraphQL at Facebook, especially for our mobile clients. Additional background context is being added to the README.
The goal in adding this support to express-graphql is to provide a clear example of how this idea can be implemented which should allow it to be translated to any other GraphQL server middleware or implementation.
Notes: