Skip to content

Commit

Permalink
feat: simple collections and massive perf gains on small queries (#766)
Browse files Browse the repository at this point in the history 
* Allow disabling the query log

* Don't parse URL multiple times

* More efficient

* Marginal boost

* Fence debug statements

* Make body-parser middleware more optimisable

* Precompute

* Cache query validation

* Minor optimisation

* Simple collections CLI options

* Minor perf tweaks

* More efficient localSettings

* Only create DB transactions if necessary

* Skip transactions where possible for greater performance

* Fix tests and JSON handling in JWTs

* No need to provision pgClient if an error occurs

* Upgrade postgraphile-core
  • Loading branch information
@benjie
benjie authored May 27, 2018
1 parent 602bc95 commit d8d131e
Show file tree
Hide file tree
Showing 8 changed files with 321 additions and 153 deletions.
5 changes: 3 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "postgraphile",
"version": "4.0.0-beta.9",
"version": "4.0.0-beta.9.3",
"description": "A GraphQL schema created by reflection over a PostgreSQL schema 🐘 (previously known as PostGraphQL)",
"author": "Benjie Gillam <[email protected]> (https://twitter.com/benjie)",
"license": "MIT",
Expand Down Expand Up @@ -43,11 +43,12 @@
"http-errors": "^1.5.1",
"jsonwebtoken": "^8.0.0",
"lodash": ">=3.5 <5",
"lru-cache": "4.1.3",
"parseurl": "^1.3.1",
"pg": ">=6.1.0 <8",
"pg-connection-string": "^0.1.3",
"pg-sql2": "2.0.0",
"postgraphile-core": "4.0.0-beta.7",
"postgraphile-core": "4.0.0-beta.7.2",
"send": "^0.16.1",
"tslib": "^1.5.0"
},
Expand Down
70 changes: 40 additions & 30 deletions src/postgraphile/__tests__/withPostGraphileContext-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ test('will throw an error if there was a `jwtToken`, but no `jwtSecret`', async
403,
'Not allowed to provide a JWT token.',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will throw an error for a malformed `jwtToken`', async () => {
Expand All @@ -91,7 +92,8 @@ test('will throw an error for a malformed `jwtToken`', async () => {
403,
'jwt malformed',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will throw an error if the JWT token was signed with the wrong signature', async () => {
Expand All @@ -111,7 +113,8 @@ test('will throw an error if the JWT token was signed with the wrong signature',
403,
'invalid signature',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will throw an error if the JWT token does not have an audience', async () => {
Expand All @@ -131,7 +134,8 @@ test('will throw an error if the JWT token does not have an audience', async ()
403,
'jwt audience invalid. expected: postgraphile',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will throw an error if the JWT token does not have an appropriate audience', async () => {
Expand All @@ -151,7 +155,8 @@ test('will throw an error if the JWT token does not have an appropriate audience
403,
'jwt audience invalid. expected: postgraphile',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will succeed with all the correct things', async () => {
Expand Down Expand Up @@ -202,11 +207,11 @@ test('will add extra claims as available', async () => {
'jwt.claims.aud',
'postgraphile',
'jwt.claims.a',
1,
'1',
'jwt.claims.b',
2,
'2',
'jwt.claims.c',
3,
'3',
],
},
],
Expand Down Expand Up @@ -420,11 +425,11 @@ test('will set the default role if no other role was provided in the JWT', async
'jwt.claims.aud',
'postgraphile',
'jwt.claims.a',
1,
'1',
'jwt.claims.b',
2,
'2',
'jwt.claims.c',
3,
'3',
],
},
],
Expand Down Expand Up @@ -459,11 +464,11 @@ test('will set a role provided in the JWT', async () => {
'jwt.claims.aud',
'postgraphile',
'jwt.claims.a',
1,
'1',
'jwt.claims.b',
2,
'2',
'jwt.claims.c',
3,
'3',
'jwt.claims.role',
'test_jwt_role',
],
Expand Down Expand Up @@ -501,11 +506,11 @@ test('will set a role provided in the JWT superceding the default role', async (
'jwt.claims.aud',
'postgraphile',
'jwt.claims.a',
1,
'1',
'jwt.claims.b',
2,
'2',
'jwt.claims.c',
3,
'3',
'jwt.claims.role',
'test_jwt_role',
],
Expand Down Expand Up @@ -549,13 +554,13 @@ test('will set a role provided in the JWT', async () => {
'jwt.claims.aud',
'postgraphile',
'jwt.claims.a',
1,
'1',
'jwt.claims.b',
2,
'2',
'jwt.claims.c',
3,
'3',
'jwt.claims.some',
{ other: { path: 'test_deep_role' } },
JSON.stringify({ other: { path: 'test_deep_role' } }),
],
},
],
Expand Down Expand Up @@ -598,13 +603,13 @@ test('will set a role provided in the JWT superceding the default role', async (
'jwt.claims.aud',
'postgraphile',
'jwt.claims.a',
1,
'1',
'jwt.claims.b',
2,
'2',
'jwt.claims.c',
3,
'3',
'jwt.claims.some',
{ other: { path: 'test_deep_role' } },
JSON.stringify({ other: { path: 'test_deep_role' } }),
],
},
],
Expand Down Expand Up @@ -635,7 +640,8 @@ describe('jwtVerifyOptions', () => {
403,
'Provide either \'jwtAudiences\' or \'jwtVerifyOptions.audience\' but not both',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will succeed with both jwtAudiences and jwtVerifyOptions if jwtVerifyOptions does not have an audience field', async () => {
Expand Down Expand Up @@ -683,7 +689,8 @@ describe('jwtVerifyOptions', () => {
403,
'jwt audience invalid. expected: another-audience',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will throw an error from a mismatched subject', async () => {
Expand All @@ -701,7 +708,8 @@ describe('jwtVerifyOptions', () => {
403,
'jwt subject invalid. expected: orangutan',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will throw an error from an issuer array that does not match iss', async () => {
Expand All @@ -721,7 +729,8 @@ describe('jwtVerifyOptions', () => {
403,
'jwt issuer invalid. expected: alpha:aliens,alpha:ufo',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// Never set up the transaction due to error
expect(pgClient.query.mock.calls).toEqual([])
})

test('will default to an audience of [\'postgraphile\'] if no audience params are provided', async () => {
Expand All @@ -737,6 +746,7 @@ describe('jwtVerifyOptions', () => {
403,
'jwt audience invalid. expected: postgraphile',
)
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
// No need for transaction since there's no settings
expect(pgClient.query.mock.calls).toEqual([])
})
})
7 changes: 6 additions & 1 deletion src/postgraphile/cli.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ program
.option('-N, --no-setof-functions-contain-nulls', 'if none of your `RETURNS SETOF compound_type` functions mix NULLs with the results then you may enable this to reduce the nullables in the GraphQL schema')
.option('-a, --classic-ids', 'use classic global id field name. required to support Relay 1')
.option('-M, --disable-default-mutations', 'disable default mutations, mutation will only be possible through Postgres functions')
.option('--simple-collections [omit|both|only]', '"omit" (default) - relay connections only, "only" - simple collections only (no Relay connections), "both" - both')

pluginHook('cli:flags:add:schema', addFlag)

Expand Down Expand Up @@ -127,6 +128,7 @@ program
.option('-l, --body-size-limit <string>', 'set the maximum size of JSON bodies that can be parsed (default 100kB) The size can be given as a human-readable string, such as \'200kB\' or \'5MB\' (case insensitive).')
.option('--cluster-workers <count>', '[experimental] spawn <count> workers to increase throughput', parseFloat)
.option('--enable-query-batching', '[experimental] enable the server to process multiple GraphQL queries in one request')
.option('--disable-query-log', 'disable logging queries to console')

pluginHook('cli:flags:add:webserver', addFlag)

Expand Down Expand Up @@ -232,6 +234,8 @@ const {
enableQueryBatching,
setofFunctionsContainNulls = true,
legacyJsonUuid,
disableQueryLog,
simpleCollections,
// tslint:disable-next-line no-any
} = Object.assign({}, config['options'], program) as any

Expand Down Expand Up @@ -340,7 +344,7 @@ const postgraphileOptions = pluginHook('cli:library:options', Object.assign({},
watchPg,
showErrorStack,
extendedErrors,
disableQueryLog: false,
disableQueryLog,
enableCors,
exportJsonSchemaPath,
exportGqlSchemaPath,
Expand All @@ -354,6 +358,7 @@ const postgraphileOptions = pluginHook('cli:library:options', Object.assign({},
legacyJsonUuid,
enableQueryBatching,
pluginHook,
simpleCollections,
}), { config, cliOptions: program })

if (noServer) {
Expand Down
32 changes: 30 additions & 2 deletions src/postgraphile/http/__tests__/createPostGraphileHttpRequestHandler-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ for (const [name, createServerFromHandler] of Array.from(serverCreators)) {
})

test('will connect and release a Postgres client from the pool on every request', async () => {
// Note: no BEGIN/END because we don't need it here
pgPool.connect.mockClear()
pgClient.query.mockClear()
pgClient.release.mockClear()
Expand All @@ -285,15 +286,33 @@ for (const [name, createServerFromHandler] of Array.from(serverCreators)) {
.expect('Content-Type', /json/)
.expect({ data: { hello: 'world' } })
expect(pgPool.connect.mock.calls).toEqual([[]])
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
expect(pgClient.query.mock.calls).toEqual([])
expect(pgClient.release.mock.calls).toEqual([[]])
})

test('will setup a transaction for requests that use the Postgres client', async () => {
test('will connect and release a Postgres client with a transaction from the pool on every mutation request', async () => {
pgPool.connect.mockClear()
pgClient.query.mockClear()
pgClient.release.mockClear()
const server = createServer()
await request(server)
.post('/graphql')
.send({ query: 'mutation{hello}' })
.expect(200)
.expect('Content-Type', /json/)
.expect({ data: { hello: 'world' } })
expect(pgPool.connect.mock.calls).toEqual([[]])
expect(pgClient.query.mock.calls).toEqual([['begin'], ['commit']])
expect(pgClient.release.mock.calls).toEqual([[]])
})

test('will setup a transaction for requests that use the Postgres client and have config', async () => {
pgPool.connect.mockClear()
pgClient.query.mockClear()
pgClient.release.mockClear()
const server = createServer({
pgDefaultRole: 'bob',
})
await request(server)
.post('/graphql')
.send({ query: '{query}' })
Expand All @@ -303,6 +322,15 @@ for (const [name, createServerFromHandler] of Array.from(serverCreators)) {
expect(pgPool.connect.mock.calls).toEqual([[]])
expect(pgClient.query.mock.calls).toEqual([
['begin'],
[
{
'text': 'select set_config($1, $2, true)',
'values': [
'role',
'bob',
],
},
],
['EXECUTE'],
['commit'],
])
Expand Down
Loading

0 comments on commit d8d131e

Please sign in to comment.