-
-
Notifications
You must be signed in to change notification settings - Fork 34
feature(automatic-transactions): Creates a decorator to make transactions easier for the developer #76
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
feature(automatic-transactions): Creates a decorator to make transactions easier for the developer #76
Changes from 3 commits
3de300e
90febc6
95623b9
3d5c5ca
b9a417f
91fc85d
9513db6
3fea7fe
f4b730d
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 |
---|---|---|
@@ -0,0 +1,12 @@ | ||
module.exports = (existingHandler, newHandler) => { | ||
jsumners marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (Array.isArray(existingHandler)) { | ||
return [ | ||
...existingHandler, | ||
newHandler | ||
] | ||
} else if (typeof existingHandler === 'function') { | ||
return [existingHandler, newHandler] | ||
} else { | ||
return [newHandler] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,10 @@ | |
const defaultPg = require('pg') | ||
const fp = require('fastify-plugin') | ||
|
||
const addHandler = require('./add-handler.js') | ||
|
||
const transactionFailedSymbol = Symbol('transactionFailed') | ||
|
||
function transactionUtil (pool, fn, cb) { | ||
pool.connect((err, client, done) => { | ||
if (err) return cb(err) | ||
|
@@ -52,6 +56,10 @@ function transact (fn, cb) { | |
}) | ||
} | ||
|
||
function extractRequestClient (req, connectionId) { | ||
return connectionId.length ? req.pg[connectionId] : req.pg | ||
} | ||
|
||
function fastifyPostgres (fastify, options, next) { | ||
let pg = defaultPg | ||
|
||
|
@@ -102,6 +110,59 @@ function fastifyPostgres (fastify, options, next) { | |
} | ||
} | ||
|
||
if (!fastify.hasRequestDecorator('pg')) { | ||
fastify.decorateRequest('pg', null) | ||
} | ||
|
||
fastify.addHook('onRoute', routeOptions => { | ||
const transactionConnection = routeOptions.pg && routeOptions.pg.transact | ||
|
||
if (transactionConnection) { | ||
const preHandler = async (req, reply) => { | ||
const client = await pool.connect() | ||
|
||
if (name) { | ||
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. are you sure about the namespacing logic? you must handle all combinations of:
to make an example of something that doesn't work correctly here, if you register with name 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. Good spot. An error might be good in this case too, but I should also make a test that covers it. Currently working through the test for duplicate namespaces that might have exposed something else, so I will work on this next. 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 think this can be handled just by checking the Either way, I think we should do this, so I will add an error and test for this |
||
req.pg = {} | ||
mcollina marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (client[name]) { | ||
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 can't apply the same logic that was applied before, it doesn't make much sense here 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. We resolved this in Slack. It makes sense to be defensive and avoid a namespace that matches a property on the |
||
throw new Error(`pg client '${name}' is a reserved keyword`) | ||
} else if (req.pg[name]) { | ||
throw new Error(`request client '${name}' has already been registered`) | ||
} | ||
|
||
req.pg[name] = client | ||
} else { | ||
if (req.pg) { | ||
throw new Error('request client has already been registered') | ||
} else { | ||
req.pg = client | ||
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. Must 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. In fact, this is caught by the previous line |
||
} | ||
} | ||
|
||
extractRequestClient(req, transactionConnection).query('BEGIN') | ||
} | ||
|
||
const onError = (req, reply, error, done) => { | ||
req[transactionFailedSymbol] = true | ||
extractRequestClient(req, transactionConnection).query('ROLLBACK', done) | ||
} | ||
|
||
const onSend = async (req) => { | ||
const requestClient = extractRequestClient(req, transactionConnection) | ||
try { | ||
if (!req[transactionFailedSymbol]) { | ||
await requestClient.query('COMMIT') | ||
} | ||
} finally { | ||
requestClient.release() | ||
} | ||
} | ||
|
||
routeOptions.preHandler = addHandler(routeOptions.preHandler, preHandler) | ||
routeOptions.onError = addHandler(routeOptions.onError, onError) | ||
routeOptions.onSend = addHandler(routeOptions.onSend, onSend) | ||
} | ||
}) | ||
|
||
next() | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
"main": "index.js", | ||
"types": "index.d.ts", | ||
"scripts": { | ||
"testonly": "tap -J test/*.test.js && npm run test:typescript", | ||
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. why this? Can you remove? 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 found this useful because when I was developing I frequently wanted to see if I'd broken tests. When running that I didn't want something like indentation linting to stop the tests running. Happy to remove, but that was my reasoning. 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. ok |
||
"test": "standard && tap -J test/*.test.js && npm run test:typescript", | ||
"test:typescript": "tsd", | ||
"test:report": "standard && tap -J --coverage-report=html test/*.test.js", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
'use strict' | ||
|
||
const t = require('tap') | ||
const test = t.test | ||
const addHandler = require('../add-handler') | ||
|
||
test('addHandler - ', t => { | ||
test('when existing handler is not defined', t => { | ||
t.plan(1) | ||
|
||
const handlers = addHandler( | ||
undefined, | ||
'test' | ||
) | ||
|
||
t.same(handlers, ['test']) | ||
}) | ||
test('when existing handler is a array', t => { | ||
t.plan(1) | ||
|
||
const handlers = addHandler( | ||
['test'], | ||
'again' | ||
) | ||
|
||
t.same(handlers, ['test', 'again']) | ||
}) | ||
test('when existing handler is a function', t => { | ||
t.plan(2) | ||
|
||
const stub = () => 'test' | ||
|
||
const handlers = addHandler( | ||
stub, | ||
'again' | ||
) | ||
|
||
t.same(handlers[0](), 'test') | ||
t.same(handlers[1], 'again') | ||
}) | ||
|
||
t.end() | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
'use strict' | ||
|
||
const t = require('tap') | ||
const test = t.test | ||
const Fastify = require('fastify') | ||
const fastifyPostgres = require('../index') | ||
const { connectionString } = require('./helpers') | ||
|
||
const extractUserCount = response => parseInt(JSON.parse(response.payload).rows[0].userCount) | ||
|
||
test('fastify postgress useTransaction route option - ', t => { | ||
test('queries that succeed provided', async t => { | ||
const fastify = Fastify() | ||
t.teardown(() => fastify.close()) | ||
|
||
await fastify.register(fastifyPostgres, { | ||
connectionString | ||
}) | ||
|
||
await fastify.pg.query('TRUNCATE users') | ||
|
||
await fastify.get('/count-users', async (req, reply) => { | ||
const result = await fastify.pg.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'pass-opt-in\'') | ||
|
||
reply.send(result) | ||
}) | ||
|
||
await fastify.get('/pass', { pg: { transact: true } }, async (req, reply) => { | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-in']) | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-in']) | ||
reply.send('complete') | ||
}) | ||
|
||
await fastify.inject({ | ||
method: 'GET', | ||
url: '/pass' | ||
}) | ||
|
||
const response = await fastify.inject({ | ||
method: 'GET', | ||
url: '/count-users' | ||
}) | ||
|
||
t.is(extractUserCount(response), 2) | ||
}) | ||
test('queries that succeed provided to a namespace', async t => { | ||
const fastify = Fastify() | ||
t.teardown(() => fastify.close()) | ||
|
||
await fastify.register(fastifyPostgres, { | ||
connectionString, | ||
name: 'test' | ||
}) | ||
|
||
await fastify.pg.test.query('TRUNCATE users') | ||
|
||
await fastify.get('/count-users', async (req, reply) => { | ||
const result = await fastify.pg.test.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'pass-opt-in\'') | ||
|
||
reply.send(result) | ||
}) | ||
|
||
await fastify.get('/pass', { pg: { transact: 'test' } }, async (req, reply) => { | ||
await req.pg.test.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-in']) | ||
await req.pg.test.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-in']) | ||
|
||
reply.send('complete') | ||
}) | ||
|
||
await fastify.inject({ | ||
method: 'GET', | ||
url: '/pass' | ||
}) | ||
|
||
const response = await fastify.inject({ | ||
method: 'GET', | ||
url: '/count-users' | ||
}) | ||
|
||
t.is(extractUserCount(response), 2) | ||
}) | ||
test('queries that fail provided', async t => { | ||
const fastify = Fastify() | ||
t.teardown(() => fastify.close()) | ||
|
||
await fastify.register(fastifyPostgres, { | ||
connectionString | ||
}) | ||
|
||
await fastify.pg.query('TRUNCATE users') | ||
|
||
await fastify.get('/count-users', async (req, reply) => { | ||
const result = await fastify.pg.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'fail-opt-in\'') | ||
|
||
reply.send(result) | ||
}) | ||
|
||
await fastify.get('/fail', { pg: { transact: true } }, async (req, reply) => { | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-in']) | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-in']) | ||
await req.pg.query('INSERT INTO nope(username) VALUES($1) RETURNING id', ['fail-opt-in']) | ||
reply.send('complete') | ||
}) | ||
|
||
await fastify.inject({ | ||
method: 'GET', | ||
url: '/fail' | ||
}) | ||
|
||
const response = await fastify.inject({ | ||
method: 'GET', | ||
url: '/count-users' | ||
}) | ||
|
||
t.is(extractUserCount(response), 0) | ||
}) | ||
|
||
t.end() | ||
}) |
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.
use strict is missing.
Maybe it's better to move this file inside
lib/