-
-
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 1 commit
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 |
---|---|---|
|
@@ -52,6 +52,19 @@ function transact (fn, cb) { | |
}) | ||
} | ||
|
||
// Re-usable code adds the handlers nicely | ||
const addHandler = (existingHandler, newHandler) => { | ||
if (Array.isArray(existingHandler)) { | ||
existingHandler.push(newHandler) | ||
} else if (typeof existingHandler === 'function') { | ||
existingHandler = [existingHandler, newHandler] | ||
} else { | ||
existingHandler = [newHandler] | ||
} | ||
|
||
return existingHandler | ||
} | ||
|
||
function fastifyPostgres (fastify, options, next) { | ||
let pg = defaultPg | ||
|
||
|
@@ -102,6 +115,54 @@ function fastifyPostgres (fastify, options, next) { | |
} | ||
} | ||
|
||
fastify.addHook('onRoute', routeOptions => { | ||
const useTransaction = routeOptions.useTransaction || (routeOptions.options && routeOptions.options.useTransaction) | ||
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 we allowing two different ways to require a transaction? why? 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 is a misunderstanding on my part about how Fastify works. When I was using the following syntax I was using an options key
But I think I could put the key directly into that object in which would negate the need for the extra conditions I will change |
||
|
||
if (useTransaction) { | ||
// This will rollback the transaction if the handler fails at some point | ||
const onError = async (req, reply, error) => { | ||
req.transactionFailed = true | ||
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. prefer to use a symbol so this plugin is the only one who has access to it |
||
|
||
try { | ||
await req.pg.query('ROLLBACK') | ||
} catch (err) { | ||
await req.pg.query('ROLLBACK') | ||
} | ||
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. what does this do? 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. This is the best I could come up with in the response to the question 'what happens if rollback fails' 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 rollback fails and you try it again, it will most likely fail again. so retrying a rollback is not useful. rather, we should make sure that failing a rollback due to an error happened in the handler returns an error to the client which reflects the error that was thrown in the handler, whatever it was 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. Does Fastify already do that? ie. https://www.fastify.io/docs/latest/Hooks/#onerror The only option would seem to be throwing a 'rollback failed' Wondering what happens to unfinished transactions who's clients are released? |
||
} | ||
|
||
routeOptions.onError = addHandler(routeOptions.onError, onError) | ||
} | ||
|
||
const preHandler = async (req, reply) => { | ||
const client = await pool.connect() | ||
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. why are we grabbing a client from the pool before knowing if we need a transaction? or actually, don't we already know, if we're executing this code, that we need a transaction so the next conditional is unnecessary? 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, so we can run all of this conditionally? I was thinking we would still need to get the client in any case, but now I see that was a mistaken thought. This also relates to #76 (comment) where I wanted to ensutre that I always released the client It looks like the whole lot can go in to the conditional so I'll make that change 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 the route has no option to require the use of the transaction, all the plugin code should be a no-op basically 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 order to make this work I had to re-instate these lines from the earlier attempt which ensure a client is available on the
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. doesn't look like this code is in any way related to what you're trying to do here. if you need a client, you get it from the pool, but only if you need it 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 the original issue description |
||
|
||
if (useTransaction) { | ||
await req.pg.query('BEGIN') | ||
} | ||
} | ||
|
||
// This will commit the transaction (or rollback if that fails) and also always | ||
// release the client, regardless of error state or useTransaction value | ||
const onSend = async (req, reply, payload) => { | ||
try { | ||
if (!req.transactionFailed && useTransaction) { | ||
await req.pg.query('COMMIT') | ||
} | ||
} catch (err) { | ||
if (useTransaction) { | ||
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. do we need this conditional? aren't we executing this code only if we already knew that we needed a transaction? |
||
await req.pg.query('ROLLBACK') | ||
} | ||
} finally { | ||
req.pg.release() | ||
} | ||
} | ||
|
||
// Add these handlers | ||
routeOptions.preHandler = addHandler(routeOptions.preHandler, preHandler) | ||
routeOptions.onSend = addHandler(routeOptions.onSend, onSend) | ||
}) | ||
|
||
next() | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,8 @@ | |
"main": "index.js", | ||
"types": "index.d.ts", | ||
"scripts": { | ||
"test": "standard && tap -J test/*.test.js && npm run test:typescript", | ||
"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 --fix && tap -J test/*.test.js && npm run test:typescript", | ||
climba03003 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"test:typescript": "tsd", | ||
"test:report": "standard && tap -J --coverage-report=html test/*.test.js", | ||
"test:verbose": "standard && tap -J test/*.test.js -Rspec", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,202 @@ | ||
'use strict' | ||
|
||
const t = require('tap') | ||
const test = t.test | ||
const Fastify = require('fastify') | ||
const fastifyPostgres = require('../index') | ||
const { connectionString } = require('./helpers') | ||
|
||
// // A failed set of queries with transactions on, on test, NONE of these entries should be visible in the DB | ||
// fastify.get('/fail', { useTransaction: true }, async (req, reply) => { | ||
// console.log('in fail registration') | ||
|
||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-in-q1']) | ||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-in-q2']) | ||
// await req.pg.query('INSERT INTO nope(username) VALUES($1) RETURNING id', ['fail-opt-in-q3']) | ||
|
||
// reply.send('Fail example') | ||
// }) | ||
|
||
// // A passing set of queries with transactions on, on test, ALL of these entries should be visible in the DB | ||
// fastify.get('/pass', { useTransaction: true }, async (req, reply) => { | ||
// console.log('in pass registration') | ||
|
||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-in-q1']) | ||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-in-q2']) | ||
|
||
// reply.send('Pass example') | ||
// }) | ||
|
||
// // A failed set of queries with transactions off, on test, THE FIRST TWO of these entries should be visible in the DB | ||
// fastify.get('/fail-opt-out', { useTransaction: false }, async (req, reply) => { | ||
// console.log('in fail registration') | ||
|
||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-out-q1']) | ||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-out-q2']) | ||
// await req.pg.query('INSERT INTO nope(username) VALUES($1) RETURNING id', ['fail-opt-out-q3']) | ||
|
||
// reply.send('Fail example') | ||
// }) | ||
|
||
// // A passing set of queries with transactions off, on test, ALL of these entries should be visible in the DB | ||
// fastify.get('/pass-opt-out', { useTransaction: false }, async (req, reply) => { | ||
// console.log('in pass registration') | ||
|
||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-out-q1']) | ||
// await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-out-q2']) | ||
|
||
// reply.send('Pass example') | ||
// }) | ||
|
||
const extractUserCount = response => parseInt(JSON.parse(response.payload).rows[0].userCount) | ||
|
||
test('fastify postgress useTransaction route option - ', t => { | ||
test('set to true - ', t => { | ||
test('passing queries 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 req.pg.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'pass-opt-in\'') | ||
|
||
reply.send(result) | ||
}) | ||
|
||
await fastify.get('/pass', { useTransaction: 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('failing queries 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 req.pg.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'fail-opt-in\'') | ||
|
||
reply.send(result) | ||
}) | ||
|
||
await fastify.get('/fail', { useTransaction: 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() | ||
}) | ||
test('set to false - ', t => { | ||
test('passing queries 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 req.pg.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'pass-opt-out\'') | ||
|
||
reply.send(result) | ||
}) | ||
|
||
await fastify.get('/pass-opt-out', { useTransaction: false }, async (req, reply) => { | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-out']) | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['pass-opt-out']) | ||
reply.send('complete') | ||
}) | ||
|
||
await fastify.inject({ | ||
method: 'GET', | ||
url: '/pass-opt-out' | ||
}) | ||
|
||
const response = await fastify.inject({ | ||
method: 'GET', | ||
url: '/count-users' | ||
}) | ||
|
||
t.is(extractUserCount(response), 2) | ||
}) | ||
test('failing queries 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 req.pg.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'fail-opt-out\'') | ||
|
||
reply.send(result) | ||
}) | ||
|
||
await fastify.get('/fail-opt-out', { useTransaction: false }, async (req, reply) => { | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-out']) | ||
await req.pg.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-out']) | ||
await req.pg.query('INSERT INTO nope(username) VALUES($1) RETURNING id', ['fail-opt-out']) | ||
reply.send('complete') | ||
}) | ||
|
||
await fastify.inject({ | ||
method: 'GET', | ||
url: '/fail-opt-out' | ||
}) | ||
|
||
const response = await fastify.inject({ | ||
method: 'GET', | ||
url: '/count-users' | ||
}) | ||
|
||
t.is(extractUserCount(response), 2) | ||
}) | ||
|
||
t.end() | ||
}) | ||
|
||
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.
this can be written in a nicer way without reassigning variables and returning early.
in general, choose between two implementations:
in this case, because there's one case where you're forced to reassign the existing handler, only the first option is viable. stick to that