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.

Sign up for GitHub

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

The deduplicator now accepts a query param #312

Closed
wants to merge 7 commits into from
Closed

The deduplicator now accepts a query param #312

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
07a1be2
the deduplicator now accepts a query param and code prettified
lfades May 11, 2018
878bce8
Now the docs use the query param for the deduplicate option
lfades May 11, 2018
554b5bc
Merge branch 'master' of https://github.com/graphcool/graphql-yoga in…
lfades May 11, 2018
02f865c
Improved docs for the deduplicator option
lfades May 11, 2018
e07b8f2
Now only some values are taken as valid for ?deduplicate
lfades May 11, 2018
016cd0e
Some changes to formatResponse
lfades May 18, 2018
6c60de9
Merged with master
lfades May 18, 2018
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ The `options` object has the following fields:
| `uploads` | [UploadOptions](/src/types.ts#L39-L43) or `false` or `undefined` | `null` | Provides information about upload limits; the object can have any combination of the following three keys: `maxFieldSize`, `maxFileSize`, `maxFiles`; each of these have values of type Number; setting to `false` disables file uploading |
| `https` | [HttpsOptions](/src/types.ts#L62-L65) or `undefined` | `undefined` | Enables HTTPS support with a key/cert
| `getEndpoint` | String or Boolean | `false` | Adds a graphql HTTP GET endpoint to your server (defaults to `endpoint` if `true`). Used for leveraging CDN level caching. |
| `deduplicator` | Boolean | `true` | Enables [graphql-deduplicator](https://github.com/gajus/graphql-deduplicator). Once enabled sending the header `X-GraphQL-Deduplicate` will deduplicate the data. |
| `deduplicator` | Boolean | `true` | Enables [graphql-deduplicator](https://github.com/gajus/graphql-deduplicator). Once enabled, adding `?deduplicate` to the endpoint URL will deduplicate the data. |
| `bodyParserOptions` | BodyParserJSONOptions | [BodyParserJSONOptions Defaults](https://github.com/expressjs/body-parser#bodyparserjsonoptions) | Allows pass through of [body-parser options](https://github.com/expressjs/body-parser#bodyparserjsonoptions) |

Additionally, the `options` object exposes these `apollo-server` options:
Expand Down
67 changes: 40 additions & 27 deletions src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ import { GraphQLServer, Options } from './index'
import { promisify } from 'util'
import * as request from 'request-promise-native'

async function startServer(
t: TestContext & Context<any>,
options?: Options
) {
const randomId = () => Math.random().toString(36).substr(2, 5)
async function startServer(t: TestContext & Context<any>, options?: Options) {
const randomId = () =>
Math.random()
.toString(36)
.substr(2, 5)

const typeDefs = `
type Author {
Expand All @@ -33,18 +33,18 @@ async function startServer(
__typename: 'Author',
id: randomId(),
name: 'Jhon',
lastName: 'Doe'
lastName: 'Doe',
}
const book = {
__typename: 'Book',
id: randomId(),
name: 'Awesome',
author
author,
}
const resolvers = {
Query: {
hello: (_, { name }) => `Hello ${name || 'World'}`,
books: () => Array(5).fill(book)
books: () => Array(5).fill(book),
},
}

Expand All @@ -70,7 +70,7 @@ test.afterEach.always('stop graphql servers', async t => {

if (httpServers && httpServers.length) {
await Promise.all(
httpServers.map(server => promisify(server.close).call(server))
httpServers.map(server => promisify(server.close).call(server)),
)
}
})
Expand Down Expand Up @@ -99,7 +99,10 @@ test('works with simple hello world server', async t => {
})

test('Response data can be deduplicated with graphql-deduplicator', async t => {
const { uri, data: { book } } = await startServer(t)
const {
uri,
data: { book },
} = await startServer(t)

const query = `
query {
Expand All @@ -125,19 +128,27 @@ test('Response data can be deduplicated with graphql-deduplicator', async t => {
}).promise()

const deduplicated = await request({
uri: uri + '?deduplicate',
method: 'POST',
json: true,
body: { query },
}).promise()

// The use of a header is deprecated but should work
const deduplicatedWithHeader = await request({
Copy link
Contributor

@kachkaev kachkaev May 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It might be useful to state why the test is here. Others might be surprised seeing it despite that the docs don't mention X-GraphQL-Deduplicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, a comment can make it more clear

uri,
method: 'POST',
json: true,
body: { query },
headers: {
'X-GraphQL-Deduplicate': true
}
'X-GraphQL-Deduplicate': true,
},
}).promise()

t.deepEqual(body, {
data: {
books: Array(5).fill(book)
}
books: Array(5).fill(book),
},
})

t.deepEqual(deduplicated, {
Expand All @@ -146,18 +157,23 @@ test('Response data can be deduplicated with graphql-deduplicator', async t => {
book,
...Array(4).fill({
__typename: book.__typename,
id: book.id
})
]
}
id: book.id,
}),
],
},
})

t.deepEqual(deduplicated, deduplicatedWithHeader)

t.deepEqual(body.data, inflate(deduplicated.data))
Copy link
Contributor

Choose a reason for hiding this comment

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

If neither of deduplications worked, will the test pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that one is just to check that both are the same, to check that data is properly deduplicated there's another check above

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sure! I somehow missed t.deepEqual above 😅

})

test('graphql-deduplicated can be completely disabled', async t => {
const { uri, data: { book } } = await startServer(t, {
deduplicator: false
test('graphql-deduplicator can be completely disabled', async t => {
const {
uri,
data: { book },
} = await startServer(t, {
deduplicator: false,
})

const query = `
Expand All @@ -177,18 +193,15 @@ test('graphql-deduplicated can be completely disabled', async t => {
`

const body = await request({
uri,
uri: uri + '?deduplicate',
method: 'POST',
json: true,
body: { query },
headers: {
'X-GraphQL-Deduplicate': true
}
}).promise()

t.deepEqual(body, {
data: {
books: Array(5).fill(book)
}
books: Array(5).fill(book),
},
})
})
13 changes: 9 additions & 4 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,10 @@ export class GraphQLServer {
if (mocks) {
addMockFunctionsToSchema({
schema: this.executableSchema,
mocks: typeof mocks === "object" ? mocks : undefined,
mocks: typeof mocks === 'object' ? mocks : undefined,
preserveResolvers: false,
})
}

}

if (props.middlewares) {
Expand Down Expand Up @@ -184,8 +183,11 @@ export class GraphQLServer {
return this.options.formatResponse
}
return (response, ...args) => {
const truthyValues = ['', '1', 'true']
Copy link
Contributor

@kachkaev kachkaev May 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Could this array be initialised once rather than at every request? It'd be probably useful to name it a bit more explicitly if moving elsewhere, say to trulyValuesForQueryParameter. I'm also wondering if '' would be the value for ?deduplicate just like in the case of ?deduplicate=. Can't check this now, but could the first case produce null? I'm also not sure that ?deduplicate= should be treated as true, to me this is like ?deduplicate=false.

It'd be nice to add a test with a non-truly value (e.g. ?deduplicate=false) to make sure we're not accidentally deduplicating at all times. If we turn a check into a function, it may be easier to unit-test and reuse with ?crunch in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?deduplicate and ?deduplicate= are the same (''), I think we can treat both as true or only allow ?deduplicate=true and ?deduplicate=1, otherwise we'll need to do another check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically we can do something like this:

function isTrulyQueryParam(value: string) {
  return value === '1' || value === 'true'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go for treating ?deduplicate, ?deduplicate=, ?deduplicate=true and ?deduplicate=1 as true then. I guess people would want to use http://example.com/graphql?deduplicate – it looks pretty clean.

Copy link
Contributor

@kachkaev kachkaev May 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

/**
 * Normalises GET param values into a boolean.
 * Returns true in the following cases:
 * ?param ?param= ?param=1 ?param=true
 */
function isTrulyQueryParam(value: string) {
  return value === '' || value === '1' || value === 'true'
}


if (
req.get('X-GraphQL-Deduplicate') &&
(truthyValues.includes(req.query.deduplicate) ||
req.get('X-GraphQL-Deduplicate')) &&
response.data &&
!response.data.__schema
) {
Expand All @@ -205,7 +207,10 @@ export class GraphQLServer {
app.use(cors())
}

app.post(this.options.endpoint, bodyParser.graphql(this.options.bodyParserOptions))
app.post(
this.options.endpoint,
bodyParser.graphql(this.options.bodyParserOptions),
)

if (this.options.uploads) {
app.post(this.options.endpoint, apolloUploadExpress(this.options.uploads))
Expand Down
12 changes: 6 additions & 6 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,10 @@ export interface LambdaOptions extends ApolloServerOptions {
}

export interface BodyParserJSONOptions {
limit?: number | string,
inflate?: boolean,
reviver?: any,
strict?: boolean,
type?: string,
verify?: any,
limit?: number | string
inflate?: boolean
reviver?: any
strict?: boolean
type?: string
verify?: any
}