feature(automatic-transactions): Creates a decorator to make transactions easier for the developer #76

toni-sharpe · 2021-02-09T09:11:05Z

Description

Enables handlers to be wrapped in a transaction which reduces developer boilerplate when defining routes.
To do this we initialise some request hooks in the onRoute handler.
** preHandler: gets a client from the pool and decorates the request object with that client. Note that we use a client, not the db object that is at fastify.pg.
** onError: if the handler fails we rollback in this handler (and set a Boolean so we don't try to commit in onSend.
** onSend: We commit here if the handler is successful and make sure the client is released to the pool.
We also handle namespacing, ensuring that the name provided in the plugin config is also used to decorate request with a client at that name.
We use { pg: { transact: true || 'string' } } to decide whether to use req.pg or req.pg[name] in the request hooks. Note that this was easier to handle than the proposed API in the issue.
Various checks avoid adding handlers where they're not required.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Questions

The issue with what to do when a try/catch is used in the handler is still outstanding

Resolves #75

nherment · 2021-02-11T09:53:53Z

test/req-transaction.test.js

+    fastify.route({
+      method: 'GET',
+      url: '/users',
+      handler: (req, reply) => {


add option { pg: { transact: true } }

nherment · 2021-02-11T10:20:08Z

What I think is missing is:

Ensure the new Fastify 'start' hook is triggered based on { pg: { transact: true } }
Ensure the hook that replaces req.pg with a single client, within a transaction (do a lient.query('BEGIN') as the transact() func does)
Ensure the TX is closed (commited or rolled back) by the hook when the route handler is done

You can priobably use the existign transact method defined in the plugin

simoneb

I'm posting this late but we went through this already anyway

simoneb · 2021-02-15T13:28:05Z

index.js

+
+    if (useTransaction) {
+      fastify.addHook('preHandler', async (req, reply) => {
+        await fastify.pg.transact(async (client, commit, done) => {


get rid of this one

simoneb · 2021-02-15T13:28:12Z

index.js

+        })
+      })
+      fastify.addHook('onSend', async (req, reply) => {
+        await fastify.pg.transact(async (client, commit, done) => {


get rid of this one

simoneb · 2021-02-15T13:39:09Z

index.js

+    const useTransaction = routeOptions.options && routeOptions.options.useTransaction
+
+    if (useTransaction) {
+      fastify.addHook('preHandler', async (req, reply) => {


fastify.addHook('onRoute', routeOptions => { const useTransaction = routeOptions.options && routeOptions.options.useTransaction // - add the preHandler and onSend hooks as in https://github.com/fastify/fastify-rate-limit/blob/c73dfdb78f3012f56d0ec7e02f754f205260ac1d/index.js#L112-L118 // - in preHandler you: 1) grab a client from the pool, 2) start the transaction (with client.query('BEGIN')) and 3) decorate the request with the client so it's accessible within the handler // - the handler is executed // - in onSend you decide whether to commit or rollback the transaction (see discussion in issue comments to decide which logic to apply here })

* Uses hooks to handle transactions outside the route handler code * preHandler does BEGIN * onSend does COMMIT * onError does ROLLBACK * useTransaction routeOption gives the developer opt-in for this feature - they have to explicitly ask * Tests cover the four possibilities, a passing and failing set of queries, called in both true and false states for useTransaction For DX: * Adds `--fix` to the linting to help automate indenting etc. * Adds a `testonly` script which doesn't drop out for linting (helpful when iterating quickly over tests). resolves fastify#75

simoneb · 2021-02-16T21:38:29Z

index.js

+const addHandler = (existingHandler, newHandler) => {
+  if (Array.isArray(existingHandler)) {
+    existingHandler.push(newHandler)
+  } else if (typeof existingHandler === 'function') {
+    existingHandler = [existingHandler, newHandler]
+  } else {
+    existingHandler = [newHandler]
+  }
+
+  return existingHandler
+}


this can be written in a nicer way without reassigning variables and returning early.

in general, choose between two implementations:

immutable: return a new value to the caller without modifying the inputs

mutable: mutate the input and don't return anything to the caller

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

simoneb · 2021-02-16T21:39:24Z

index.js

@@ -102,6 +115,54 @@ function fastifyPostgres (fastify, options, next) {
    }
  }

+  fastify.addHook('onRoute', routeOptions => {
+    const useTransaction = routeOptions.useTransaction || (routeOptions.options && routeOptions.options.useTransaction)


are we allowing two different ways to require a transaction? why?

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

method: 'GET', options: { useTransaction }, handler ...

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

simoneb · 2021-02-16T21:39:58Z

index.js

+    if (useTransaction) {
+      // This will rollback the transaction if the handler fails at some point
+      const onError = async (req, reply, error) => {
+        req.transactionFailed = true


prefer to use a symbol so this plugin is the only one who has access to it

simoneb · 2021-02-16T21:40:14Z

index.js

+        try {
+          await req.pg.query('ROLLBACK')
+        } catch (err) {
+          await req.pg.query('ROLLBACK')
+        }


what does this do?

This is the best I could come up with in the response to the question 'what happens if rollback fails'

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

Does Fastify already do that? ie. onError intercepts an error that was already heading to the user via 500 mechanisms?

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?

simoneb · 2021-02-16T21:41:40Z

index.js

+      const client = await pool.connect()
+      req.pg = client


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?

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

if the route has no option to require the use of the transaction, all the plugin code should be a no-op basically

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 req:

if (!fastify.hasRequestDecorator('pg')) { fastify.decorateRequest('pg', null) fastify.addHook('onRequest', async (req, reply) => { req.pg = fastify.pg }) }

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

In the original issue description pg is added to the req but if we only do that conditionally in useTransaction then we don't have that available on the req anymore. This makes the API inconsistent, ie, if I change useTransaction from true to false I then also have to change from req.pg to fastify.pg in my code. My useTransaction=false tests failed and enabled me to catch this.

simoneb · 2021-02-16T21:42:51Z

index.js

+          await req.pg.query('COMMIT')
+        }
+      } catch (err) {
+        if (useTransaction) {


do we need this conditional? aren't we executing this code only if we already knew that we needed a transaction?

package.json

simoneb · 2021-02-17T12:21:41Z

add-handler.js

+    handlers = [newHandler]
+  }
+
+  return handlers


remove handlers variable and early return

simoneb · 2021-02-17T12:23:31Z

index.js

+  fastify.addHook('onRoute', routeOptions => {
+    const useTransaction = routeOptions.useTransaction
+
+    const transactionFailedSymbol = Symbol('transactionFailed')


pull this at the module root

simoneb · 2021-02-17T12:24:51Z

index.js

@@ -102,6 +104,57 @@ function fastifyPostgres (fastify, options, next) {
    }
  }

+  fastify.addHook('onRoute', routeOptions => {
+    const useTransaction = routeOptions.useTransaction


this should be as suggested in the issue { pg: { transact: true } }

simoneb · 2021-02-17T12:25:55Z

index.js

+      const preHandler = async (req, reply) => {
+        const client = await pool.connect()
+        req.pg = client
+        await req.pg.query('BEGIN')
+      }


this is preHandler, let's define it before onError

simoneb · 2021-02-17T12:27:38Z

index.js

+
+      const preHandler = async (req, reply) => {
+        const client = await pool.connect()
+        req.pg = client


use decorateRequest

simoneb · 2021-02-17T12:34:37Z

index.js

+      const preHandler = async (req, reply) => {
+        const client = await pool.connect()
+        req.pg = client
+        await req.pg.query('BEGIN')


awaiting at the end of an async function is an anti pattern https://www.nearform.com/blog/javascript-promises-the-definitive-guide/

simoneb · 2021-02-17T12:42:30Z

index.js

+      const onError = async (req, reply, error) => {
+        req[transactionFailedSymbol] = true
+
+        try {
+          await req.pg.query('ROLLBACK')
+        } catch (err) {
+          // await req.pg.query('ROLLBACK')
+        }
+      }


please check in a unit test where you mock req.pg.query what happens if sending the ROLLBACK query fails, because according to the fastify docs, the onError hook does not support sending an error to the done callback

Suggested change

const onError = async (req, reply, error) => {

req[transactionFailedSymbol] = true

try {

await req.pg.query('ROLLBACK')

} catch (err) {

// await req.pg.query('ROLLBACK')

}

}

const onError = (req, reply, error, done) => {

req[transactionFailedSymbol] = true

req.pg.query('ROLLBACK', done)

}

simoneb · 2021-02-17T12:44:54Z

index.js

+
+      // 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) => {


nit: we may not need the reply and payload args if we're not using them

simoneb · 2021-02-17T12:47:30Z

index.js

+        } catch (err) {
+          await req.pg.query('ROLLBACK')
+        } finally {


no need to rollback if the commit failed. this keeps the same semantics as the transaction util defined above

simoneb · 2021-02-17T12:52:38Z

index.js

+  if (!fastify.hasRequestDecorator('pg')) {
+    fastify.decorateRequest('pg', null)
+    fastify.addHook('onRequest', async (req, reply) => {
+      req.pg = fastify.pg
+    })
+  }


let remove this for the time being

if I were to write this code, I would keep it like: (not 100% sure though)

Suggested change

if (!fastify.hasRequestDecorator('pg')) {

fastify.decorateRequest('pg', null)

fastify.addHook('onRequest', async (req, reply) => {

req.pg = fastify.pg

})

}

if (!fastify.hasRequestDecorator('pg')) {

fastify.decorateRequest('pg', fastify.pg)

}

Resolves fastify#75

simoneb · 2021-02-18T08:35:36Z

index.js

+            return next(new Error(`pg client '${name}' is a reserved keyword`))
+          } else if (fastify.pg[name]) {
+            return next(new Error(`request client '${name}' has already been registered`))


the error handling here should be done differently from how it's done in the plugin root. calling next with an Error was fine there, it is not here. Simply throw the error instead. Same at line 130.

simoneb · 2021-02-18T08:39:57Z

index.js

+          }
+        }
+
+        req.pg.query('BEGIN')


if the plugin is registered with a name, you have to use it everywhere. this will not work. same in all other places where you're accessing req.pg. all those accesses, in case a name is provided, should be req.pg[name]

simoneb · 2021-02-18T08:40:58Z

index.js

+        const client = await pool.connect()
+
+        if (name) {
+          if (client[name]) {


you can't apply the same logic that was applied before, it doesn't make much sense here

We resolved this in Slack. It makes sense to be defensive and avoid a namespace that matches a property on the client object from pg.

simoneb · 2021-02-18T08:41:24Z

index.js

+        if (name) {
+          if (client[name]) {
+            return next(new Error(`pg client '${name}' is a reserved keyword`))
+          } else if (fastify.pg[name]) {


this also doesn't make much sense, you must check against req.pg, not fastify.pg

As with the base fastify instance, the pg decoration will use namespaces. For each namespaced called to the main function, the namespace will be added to the req.pg object too, with similar checks The option to use a chosen namespace with the routeOptions is provided and we make sure that this namespace is used to extract the correct client from req.pg A test also covers this for queries wrapped in a route transaction Resolves fastify#75

index.js

simoneb · 2021-02-18T14:00:33Z

index.js

+      const preHandler = async (req, reply) => {
+        const client = await pool.connect()
+
+        if (name) {


are you sure about the namespacing logic? you must handle all combinations of:

non namespaced / namespaced registration

non namespaced / namespaced route transaction

to make an example of something that doesn't work correctly here, if you register with name foo and enable the transaction with name bar, this code should do nothing.

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.

if you register with name foo and enable the transaction with name bar, this code should do nothing.

I think this can be handled just by checking the req for the correct namespace and if it doesn't exist throwing an error.

Either way, I think we should do this, so I will add an error and test for this

toni-sharpe · 2021-02-19T07:33:10Z

index.js

+          if (req.pg) {
+            throw new Error('request client has already been registered')
+          } else {
+            req.pg = client


Must use Object.assign here

In fact, this is caught by the previous line

Resolves fastify#75

We needed to avoid the unnecessary addition of handlers in the case where a name and transact value mismatched (different names, or true and a name, etc.) Resolves fastify#75

simoneb · 2021-02-19T13:07:17Z

index.js

+        }
+      }
+
+      extractRequestClient(req, transact).query('BEGIN')


you don't need this here, you can simply use client

simoneb · 2021-02-19T13:23:27Z

test/req-initialization.test.js

+    fastify.inject({
+      method: 'GET',
+      url: '/'
+    })


I believe this could be simply fastify.inject('/')

simoneb · 2021-02-19T13:26:00Z

index.js

@@ -52,6 +56,18 @@ function transact (fn, cb) {
  })
 }

+function extractRequestClient (req, transact) {
+  if (transact.length) {


invert the condition here to return early, plus use a more obvious condition: typeof transact === 'string'

Resolves fastify#75

toni-sharpe · 2021-02-19T13:43:53Z

Hey everybody,

We're ready to take this out of draft and get reviews from other users.

mcollina

Good work! This is missing docs

mcollina · 2021-02-19T17:54:53Z

add-handler.js

@@ -0,0 +1,12 @@
+module.exports = (existingHandler, newHandler) => {


use strict is missing.

Maybe it's better to move this file inside lib/

mcollina · 2021-02-19T17:55:57Z

package.json

@@ -5,6 +5,7 @@
  "main": "index.js",
  "types": "index.d.ts",
  "scripts": {
+    "testonly": "tap -J test/*.test.js && npm run test:typescript",


why this? Can you remove?

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.

jsumners

Looking good. Missing documentation.

add-handler.js

Resolves fastify#75

Adds in a README section covering the transact option Resolves fastify#75

mcollina

lgtm

mcollina · 2021-02-22T09:04:09Z

cc @voxpelli

README.md

Resolves fastify#75 Co-authored-by: James Sumners <[email protected]>

climba03003

Actually, I did not use this plugin in any of my project. But I do not want to see this blocked by pending reviewer. So, I will approve the change.

Overall, when I scan through the code. It looks good.

toni-sharpe force-pushed the 75-automatic-transactions branch from f95c7c9 to e3ef5c9 Compare February 9, 2021 11:17

toni-sharpe changed the title ~~WIP()~~ @toni-sharpe feature(automatic-transactions): Creates a decorator to make transactions easier for the developer Feb 9, 2021

toni-sharpe changed the title ~~@toni-sharpe feature(automatic-transactions): Creates a decorator to make transactions easier for the developer~~ feature(automatic-transactions): Creates a decorator to make transactions easier for the developer Feb 9, 2021

toni-sharpe force-pushed the 75-automatic-transactions branch 5 times, most recently from a2f82f6 to 0c403ec Compare February 11, 2021 08:45

nherment reviewed Feb 11, 2021

View reviewed changes

toni-sharpe force-pushed the 75-automatic-transactions branch from 4a10bac to 467853a Compare February 15, 2021 09:49

simoneb reviewed Feb 16, 2021

View reviewed changes

toni-sharpe force-pushed the 75-automatic-transactions branch from 467853a to 74a1f49 Compare February 16, 2021 16:28

toni-sharpe force-pushed the 75-automatic-transactions branch from 74a1f49 to 3de300e Compare February 16, 2021 16:31

simoneb requested changes Feb 16, 2021

View reviewed changes

climba03003 requested changes Feb 17, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

simoneb requested changes Feb 17, 2021

View reviewed changes

toni-sharpe force-pushed the 75-automatic-transactions branch 2 times, most recently from 0a75a81 to ac5441b Compare February 17, 2021 15:37

PR Comments

90febc6

Resolves fastify#75

toni-sharpe force-pushed the 75-automatic-transactions branch from ac5441b to 90febc6 Compare February 17, 2021 15:43

simoneb requested changes Feb 18, 2021

View reviewed changes

toni-sharpe force-pushed the 75-automatic-transactions branch from 52317fe to 5aed915 Compare February 18, 2021 09:36

toni-sharpe force-pushed the 75-automatic-transactions branch from f64fbd4 to 95623b9 Compare February 18, 2021 13:49

simoneb reviewed Feb 18, 2021

View reviewed changes

index.js Show resolved Hide resolved

simoneb reviewed Feb 18, 2021

View reviewed changes

toni-sharpe commented Feb 19, 2021

View reviewed changes

PR Comments

3d5c5ca

Resolves fastify#75

toni-sharpe requested a review from climba03003 February 19, 2021 07:49

Early returns for mismatching name and transact values

b9a417f

We needed to avoid the unnecessary addition of handlers in the case where a name and transact value mismatched (different names, or true and a name, etc.) Resolves fastify#75

simoneb requested changes Feb 19, 2021

View reviewed changes

simoneb approved these changes Feb 19, 2021

View reviewed changes

PR Comments

91fc85d

Resolves fastify#75

toni-sharpe force-pushed the 75-automatic-transactions branch from 8bf6407 to 91fc85d Compare February 19, 2021 13:42

toni-sharpe marked this pull request as ready for review February 19, 2021 13:58

mcollina requested changes Feb 19, 2021

View reviewed changes

jsumners reviewed Feb 20, 2021

View reviewed changes

add-handler.js Outdated Show resolved Hide resolved

Toni Sharpe added 2 commits February 22, 2021 09:00

PR Comments

9513db6

Resolves fastify#75

Update README with instructions for transact

3fea7fe

Adds in a README section covering the transact option Resolves fastify#75

toni-sharpe requested review from mcollina and jsumners February 22, 2021 08:47

mcollina approved these changes Feb 22, 2021

View reviewed changes

jsumners approved these changes Feb 22, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

simoneb approved these changes Feb 22, 2021

View reviewed changes

Update README.md

f4b730d

Resolves fastify#75 Co-authored-by: James Sumners <[email protected]>

climba03003 approved these changes Feb 23, 2021

View reviewed changes

mcollina merged commit 3b5d829 into fastify:master Feb 24, 2021

darkgl0w mentioned this pull request Sep 17, 2021

[Bug] Registering multiple named instances of this module fail. fastify/fastify#3323

Closed

2 tasks

		@@ -0,0 +1,12 @@
		module.exports = (existingHandler, newHandler) => {

feature(automatic-transactions): Creates a decorator to make transactions easier for the developer #76

feature(automatic-transactions): Creates a decorator to make transactions easier for the developer #76

Conversation

toni-sharpe commented Feb 9, 2021 • edited Loading

Description

Checklist

Questions

Choose a reason for hiding this comment

nherment commented Feb 11, 2021 • edited Loading

simoneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simoneb Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toni-sharpe commented Feb 19, 2021

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 22, 2021

climba03003 left a comment

Choose a reason for hiding this comment

toni-sharpe commented Feb 9, 2021 •

edited

Loading

nherment commented Feb 11, 2021 •

edited

Loading

simoneb Feb 16, 2021 •

edited

Loading