-
Notifications
You must be signed in to change notification settings - Fork 536
[RFC] Persisted Document support #109
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,15 @@ The `graphqlHTTP` function accepts the following options: | |
* **`validationRules`**: Optional additional validation rules queries must | ||
satisfy in addition to those defined by the GraphQL spec. | ||
|
||
* **`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`. | ||
|
||
* **`persistValidatedDocument`**: A function that takes a validated Document and | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear to me when you would want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
## HTTP Usage | ||
|
||
|
@@ -140,6 +149,21 @@ new GraphQLObjectType({ | |
}); | ||
``` | ||
|
||
## Persisted Documents | ||
|
||
Putting control of the query into clients' hands is one of the primary benefits | ||
of GraphQL. Though when this is deployed to production naively, it is possible to | ||
end up uploading and revalidating the same document text across all clients of a | ||
particular deployment. It can even be problematic within a single client, sending | ||
the exact same query repeatedly. Uploading and validating a large GraphQL document | ||
can be very costly, especially over a poor network connection from a mobile device | ||
with something large and complex such as Facebook's News Feed GraphQL query. | ||
|
||
One solution to this problem that was developed early in the use of GraphQL at | ||
Facebook was persisted documents. At build time, we take the GraphQL document and | ||
persist it on the server getting back an ID. Then at runtime, we only upload the | ||
ID of the document and the variables. When the server receives this request, it | ||
loads the query and executes it, knowing it was already validated. | ||
|
||
## Debugging Tips | ||
|
||
|
@@ -154,7 +178,6 @@ formatError: error => ({ | |
}) | ||
``` | ||
|
||
|
||
[`GraphQL.js`]: https://github.com/graphql/graphql-js | ||
[`formatError`]: https://github.com/graphql/graphql-js/blob/master/src/error/formatError.js | ||
[GraphiQL]: https://github.com/graphql/graphiql | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,8 @@ import { | |
GraphQLNonNull, | ||
GraphQLString, | ||
GraphQLError, | ||
BREAK | ||
BREAK, | ||
parse | ||
} from 'graphql'; | ||
import graphqlHTTP from '../'; | ||
|
||
|
@@ -82,6 +83,14 @@ function urlString(urlParams?: ?Object) { | |
return string; | ||
} | ||
|
||
function persistString(urlParams?: ?Object) { | ||
let string = '/graphql/persist'; | ||
if (urlParams) { | ||
string += ('?' + stringify(urlParams)); | ||
} | ||
return string; | ||
} | ||
|
||
function catchError(p: any): Promise<any> { | ||
return p.then( | ||
() => { throw new Error('Expected to catch error.'); }, | ||
|
@@ -212,6 +221,72 @@ describe('test harness', () => { | |
}); | ||
}); | ||
|
||
it('allows GET with documentID param', async () => { | ||
const app = server(); | ||
|
||
app.use(urlString(), graphqlHTTP({ | ||
schema: TestSchema, | ||
loadPersistedDocument: async () => parse('{test}') | ||
})); | ||
|
||
const response = await request(app) | ||
.get(urlString({ | ||
documentID: '1' | ||
})); | ||
|
||
expect(response.text).to.equal( | ||
'{"data":{"test":"Hello World"}}' | ||
); | ||
}); | ||
|
||
it('allows GET with documentID and variable values', async () => { | ||
const app = server(); | ||
|
||
app.use(urlString(), graphqlHTTP({ | ||
schema: TestSchema, | ||
loadPersistedDocument: async() => parse('query helloWho($who: String){ test(who: $who) }') | ||
})); | ||
|
||
const response = await request(app) | ||
.get(urlString({ | ||
documentID: '1', | ||
variables: JSON.stringify({ who: 'Dolly' }) | ||
})); | ||
|
||
expect(response.text).to.equal( | ||
'{"data":{"test":"Hello Dolly"}}' | ||
); | ||
}); | ||
|
||
it('allows GET with documentID and operation name', async () => { | ||
const app = server(); | ||
|
||
app.use(urlString(), graphqlHTTP(() => ({ | ||
schema: TestSchema, | ||
loadPersistedDocument: async() => parse(` | ||
query helloYou { test(who: "You"), ...shared } | ||
query helloWorld { test(who: "World"), ...shared } | ||
query helloDolly { test(who: "Dolly"), ...shared } | ||
fragment shared on QueryRoot { | ||
shared: test(who: "Everyone") | ||
} | ||
`) | ||
}))); | ||
|
||
const response = await request(app) | ||
.get(urlString({ | ||
documentID: '1', | ||
operationName: 'helloWorld' | ||
})); | ||
|
||
expect(JSON.parse(response.text)).to.deep.equal({ | ||
data: { | ||
test: 'Hello World', | ||
shared: 'Hello Everyone', | ||
} | ||
}); | ||
}); | ||
|
||
it('Reports validation errors', async () => { | ||
const app = server(); | ||
|
||
|
@@ -328,6 +403,31 @@ describe('test harness', () => { | |
}); | ||
}); | ||
|
||
it('Errors when sending a mutation via GET by documentID', async () => { | ||
const app = server(); | ||
|
||
app.use(urlString(), graphqlHTTP({ | ||
schema: TestSchema, | ||
loadPersistedDocument: async() => parse(` | ||
mutation TestMutation { writeTest { test } } | ||
`) | ||
})); | ||
|
||
const error = await catchError( | ||
request(app) | ||
.get(urlString({ | ||
documentID: '1' | ||
})) | ||
); | ||
|
||
expect(error.response.status).to.equal(405); | ||
expect(JSON.parse(error.response.text)).to.deep.equal({ | ||
errors: [ | ||
{ message: 'Can only perform a mutation operation from a POST request.' } | ||
] | ||
}); | ||
}); | ||
|
||
it('Allows passing in a context', async () => { | ||
const app = server(); | ||
|
||
|
@@ -809,7 +909,7 @@ describe('test harness', () => { | |
|
||
expect(error.response.status).to.equal(400); | ||
expect(JSON.parse(error.response.text)).to.deep.equal({ | ||
errors: [ { message: 'Must provide query string.' } ] | ||
errors: [ { message: 'Must provide query string or document ID.' } ] | ||
}); | ||
}); | ||
|
||
|
@@ -827,7 +927,7 @@ describe('test harness', () => { | |
|
||
expect(error.response.status).to.equal(400); | ||
expect(JSON.parse(error.response.text)).to.deep.equal({ | ||
errors: [ { message: 'Must provide query string.' } ] | ||
errors: [ { message: 'Must provide query string or document ID.' } ] | ||
}); | ||
}); | ||
}); | ||
|
@@ -1036,7 +1136,7 @@ describe('test harness', () => { | |
|
||
expect(error.response.status).to.equal(400); | ||
expect(JSON.parse(error.response.text)).to.deep.equal({ | ||
errors: [ { message: 'Must provide query string.' } ] | ||
errors: [ { message: 'Must provide query string or document ID.' } ] | ||
}); | ||
}); | ||
|
||
|
@@ -1098,7 +1198,7 @@ describe('test harness', () => { | |
|
||
expect(error.response.status).to.equal(400); | ||
expect(JSON.parse(error.response.text)).to.deep.equal({ | ||
errors: [ { message: 'Must provide query string.' } ] | ||
errors: [ { message: 'Must provide query string or document ID.' } ] | ||
}); | ||
}); | ||
|
||
|
@@ -1498,5 +1598,47 @@ describe('test harness', () => { | |
|
||
}); | ||
}); | ||
|
||
describe('Persisted document support', () => { | ||
|
||
it('works when persistValidatedDocument and loadPersistedDocument are provided', async() => { | ||
const app = server(); | ||
|
||
const documents = {}; | ||
let docID = 0; | ||
|
||
const persist = async document => { | ||
const key = 'key_' + docID++; | ||
documents[key] = document; | ||
return key; | ||
}; | ||
|
||
const load = async key => { | ||
return documents[key]; | ||
}; | ||
|
||
app.use(urlString(), graphqlHTTP({ | ||
schema: TestSchema, | ||
persistValidatedDocument: persist, | ||
loadPersistedDocument: load | ||
})); | ||
|
||
let response = await request(app) | ||
.post(persistString({ | ||
document: '{test}' | ||
})); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need a test that posting data works instead of just formatting a query string |
||
|
||
const persistID = JSON.parse(response.text).documentID; | ||
|
||
response = await request(app) | ||
.get(urlString({ | ||
documentID: persistID | ||
})); | ||
|
||
expect(response.text).to.equal( | ||
'{"data":{"test":"Hello World"}}' | ||
); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofHomePageQuery
.