chore: code cleanup (#110)

* chore: upgrade package dependencies

* chore: add a `lint:fix` command

* test: add new test cases and make test names more explicit

* refactor: improve code readability

* chore: upgrade package dependencies

* refactor (test): typo fix

* refactor (test): apply @mcollina suggestion to use await fastify.ready()

Author: darkgl0w

index.js

@@ -102,19 +102,21 @@ function fastifyPostgres (fastify, options, next) {
     if (db[name]) {
       return next(new Error(`fastify-postgres '${name}' is a reserved keyword`))
     } else if (!fastify.pg) {
-      fastify.decorate('pg', {})
+      fastify.decorate('pg', Object.create(null))
     } else if (fastify.pg[name]) {
       return next(new Error(`fastify-postgres '${name}' instance name has already been registered`))
     }
 
     fastify.pg[name] = db
   } else {
-    if (!fastify.pg) {
-      fastify.decorate('pg', db)
-    } else if (fastify.pg.pool) {
-      return next(new Error('fastify-postgres has already been registered'))
-    } else {
+    if (fastify.pg) {
+      if (fastify.pg.pool) {
+        return next(new Error('fastify-postgres has already been registered'))
+      }
+
       Object.assign(fastify.pg, db)
+    } else {
+      fastify.decorate('pg', db)
     }
   }
 
@@ -125,13 +127,11 @@ function fastifyPostgres (fastify, options, next) {
   fastify.addHook('onRoute', routeOptions => {
     const transact = routeOptions && routeOptions.pg && routeOptions.pg.transact
 
-    if (!transact) {
-      return
-    }
-    if (typeof transact === 'string' && transact !== name) {
-      return
-    }
-    if (name && transact === true) {
+    if (
+      !transact ||
+      (typeof transact === 'string' && transact !== name) ||
+      (name && transact === true)
+    ) {
       return
     }
 
@@ -140,7 +140,7 @@ function fastifyPostgres (fastify, options, next) {
 
       if (name) {
         if (!req.pg) {
-          req.pg = {}
+          req.pg = Object.create(null)
         }
 
         if (client[name]) {

package.json

@@ -6,6 +6,7 @@
   "types": "index.d.ts",
   "scripts": {
     "check-examples": "tsc --build examples/typescript/*",
+    "lint:fix": "standard --fix",
     "load-data": "docker exec -it fastify-postgres psql -c 'CREATE TABLE users(id serial PRIMARY KEY, username VARCHAR (50) NOT NULL);' -U postgres -d postgres",
     "postgres": "docker run -p 5432:5432 --name fastify-postgres -e POSTGRES_PASSWORD=postgres -d postgres:11-alpine",
     "test": "standard && tap -J test/*.test.js && npm run test:typescript",
@@ -36,15 +37,15 @@
     "fastify-plugin": "^3.0.0"
   },
   "devDependencies": {
-    "@tsconfig/node10": "^1.0.7",
-    "@types/pg": "^8.6.0",
-    "fastify": "^3.0.0",
-    "pg": "^8.2.1",
+    "@tsconfig/node10": "^1.0.8",
+    "@types/pg": "^8.6.1",
+    "fastify": "^3.21.5",
+    "pg": "^8.7.1",
     "pg-native": "^3.0.0",
-    "standard": "^16.0.0",
-    "tap": "^15.0.2",
+    "standard": "^16.0.3",
+    "tap": "^15.0.10",
     "tsd": "^0.17.0",
-    "typescript": "^4.0.2"
+    "typescript": "^4.4.3"
   },
   "peerDependencies": {
     "pg": ">=6.0.0"

test/add-handler.test.js

@@ -1,11 +1,10 @@
 'use strict'
 
-const t = require('tap')
-const test = t.test
+const { test } = require('tap')
 const addHandler = require('../lib/add-handler')
 
-test('addHandler - ', t => {
-  test('when existing handler is not defined', t => {
+test('The addHandler lib should return the right handlers structure', t => {
+  t.test('When the existingHandler argument is undefined', t => {
     t.plan(1)
 
     const handlers = addHandler(
@@ -15,7 +14,8 @@ test('addHandler - ', t => {
 
     t.same(handlers, ['test'])
   })
-  test('when existing handler is a array', t => {
+
+  t.test('When the existingHandler argument is an array', t => {
     t.plan(1)
 
     const handlers = addHandler(
@@ -25,7 +25,8 @@ test('addHandler - ', t => {
 
     t.same(handlers, ['test', 'again'])
   })
-  test('when existing handler is a function', t => {
+
+  t.test('When the existingHandler argument is a function', t => {
     t.plan(2)
 
     const stub = () => 'test'

test/initialization.test.js

@@ -1,7 +1,6 @@
 'use strict'
 
-const t = require('tap')
-const test = t.test
+const { test } = require('tap')
 const Fastify = require('fastify')
 const pg = require('pg')
 const fastifyPostgres = require('../index')
@@ -247,3 +246,36 @@ test('fastify.pg custom namespace should exist if a name is set', (t) => {
     t.ok(fastify.pg.test.Client)
   })
 })
+
+test('fastify.pg and a fastify.pg custom namespace should exist when registering a named instance before an unnamed instance)', async (t) => {
+  t.plan(10)
+
+  const fastify = Fastify()
+  t.teardown(() => fastify.close())
+
+  await fastify.register(fastifyPostgres, {
+    connectionString,
+    name: 'one'
+  })
+
+  await fastify.register(fastifyPostgres, {
+    connectionString
+  })
+
+  await fastify.ready().catch(err => t.error(err))
+
+  t.ok(fastify.pg)
+  t.ok(fastify.pg.connect)
+  t.ok(fastify.pg.pool)
+  t.ok(fastify.pg.Client)
+
+  t.ok(fastify.pg.one)
+  t.ok(fastify.pg.one.connect)
+  t.ok(fastify.pg.one.pool)
+  t.ok(fastify.pg.one.Client)
+
+  const result = await fastify.pg.query('SELECT NOW()')
+  const resultOne = await fastify.pg.one.query('SELECT NOW()')
+  t.same(result.rowCount, 1)
+  t.same(resultOne.rowCount, 1)
+})

test/req-initialization.test.js

@@ -1,15 +1,14 @@
 'use strict'
 
-const t = require('tap')
-const test = t.test
+const { test } = require('tap')
 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 => {
+test('When we use the fastify-postgres transaction route option', t => {
+  t.test('Should be able to execute queries provided to the request pg decorator', async t => {
     const fastify = Fastify()
     t.teardown(() => fastify.close())
 
@@ -40,7 +39,8 @@ test('fastify postgress useTransaction route option', t => {
 
     t.equal(extractUserCount(response), 2)
   })
-  test('queries that succeed provided to a namespace', async t => {
+
+  t.test('Should be able to execute queries provided to a namespaced request pg decorator', async t => {
     const fastify = Fastify()
     t.teardown(() => fastify.close())
 
@@ -73,7 +73,8 @@ test('fastify postgress useTransaction route option', t => {
 
     t.equal(extractUserCount(response), 2)
   })
-  test('queries that fail provided', async t => {
+
+  t.test('Should trigger a rollback when failing to execute a query provided to the request pg decorator', async t => {
     const fastify = Fastify()
     t.teardown(() => fastify.close())
 
@@ -92,7 +93,43 @@ test('fastify postgress useTransaction route option', t => {
     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'])
+      // This one should fail (unknown_table does not exist) and trigger a rollback
+      await req.pg.query('INSERT INTO unknown_table(username) VALUES($1) RETURNING id', ['fail-opt-in'])
+      reply.send('complete')
+    })
+
+    await fastify.inject({ url: '/fail' })
+
+    const response = await fastify.inject({
+      method: 'GET',
+      url: '/count-users'
+    })
+
+    t.equal(extractUserCount(response), 0)
+  })
+
+  t.test('Should trigger a rollback when failing to execute a query provided to a namespaced request pg decorator', async t => {
+    const fastify = Fastify()
+    t.teardown(() => fastify.close())
+
+    await fastify.register(fastifyPostgres, {
+      connectionString,
+      name: 'test'
+    })
+
+    await fastify.pg.test.query('TRUNCATE users')
+
+    fastify.get('/count-users', async (req, reply) => {
+      const result = await fastify.pg.test.query('SELECT COUNT(*) AS "userCount" FROM users WHERE username=\'fail-opt-in\'')
+
+      reply.send(result)
+    })
+
+    fastify.get('/fail', { pg: { transact: true } }, async (req, reply) => {
+      await req.pg.test.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-in'])
+      await req.pg.test.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['fail-opt-in'])
+      // This one should fail (unknown_table does not exist) and trigger a rollback
+      await req.pg.test.query('INSERT INTO unknown_table(username) VALUES($1) RETURNING id', ['fail-opt-in'])
       reply.send('complete')
     })
 
@@ -109,8 +146,8 @@ test('fastify postgress useTransaction route option', t => {
   t.end()
 })
 
-test('combinations of registrationOptions.name and routeOptions.pg.transact that should not add hooks', t => {
-  test('transact not set', t => {
+test('Should not add hooks with combinations of registration `options.name` and route options `pg.transact`', t => {
+  t.test('Should not add hooks when `transact` is not set', t => {
     t.plan(1)
 
     const fastify = Fastify()
@@ -126,7 +163,8 @@ test('combinations of registrationOptions.name and routeOptions.pg.transact that
 
     fastify.inject({ url: '/' })
   })
-  test('name set and transact not set', t => {
+
+  t.test('Should not add hooks when `name` is set and `transact` is not set', t => {
     t.plan(1)
 
     const fastify = Fastify()
@@ -143,7 +181,8 @@ test('combinations of registrationOptions.name and routeOptions.pg.transact that
 
     fastify.inject({ url: '/' })
   })
-  test('name set and transact set to true', t => {
+
+  t.test('Should not add hooks when `name` is set and `transact` is set to `true`', t => {
     t.plan(1)
 
     const fastify = Fastify()
@@ -160,7 +199,8 @@ test('combinations of registrationOptions.name and routeOptions.pg.transact that
 
     fastify.inject({ url: '/' })
   })
-  test('name not set and transact set to string', t => {
+
+  t.test('Should not add hooks when `name` is not set and `transact` is set and is a string', t => {
     t.plan(1)
 
     const fastify = Fastify()
@@ -176,7 +216,8 @@ test('combinations of registrationOptions.name and routeOptions.pg.transact that
 
     fastify.inject({ url: '/' })
   })
-  test('name and transact set to different strings', t => {
+
+  t.test('Should not add hooks when `name` and `transact` are set to different strings', t => {
     t.plan(1)
 
     const fastify = Fastify()
@@ -193,11 +234,12 @@ test('combinations of registrationOptions.name and routeOptions.pg.transact that
 
     fastify.inject({ url: '/' })
   })
+
   t.end()
 })
 
-test('incorrect combinations of registrationOptions.name and routeOptions.pg.transact should throw errors', t => {
-  t.test('name set as reserved keyword', t => {
+test('Should throw errors with incorrect combinations of registration `options.name` and route options `pg.transact`', t => {
+  t.test('Should throw an error when `name` is set as reserved keyword', t => {
     t.plan(2)
 
     const fastify = Fastify()
@@ -222,7 +264,7 @@ test('incorrect combinations of registrationOptions.name and routeOptions.pg.tra
     })
   })
 
-  t.test('named pg client has already registered', t => {
+  t.test('Should throw an error when pg client has already been registered with the same name', t => {
     t.plan(2)
 
     const fastify = Fastify()
@@ -249,7 +291,7 @@ test('incorrect combinations of registrationOptions.name and routeOptions.pg.tra
     })
   })
 
-  t.test('pg client has already registered', t => {
+  t.test('Should throw an error when pg client has already been registered', t => {
     t.plan(2)
 
     const fastify = Fastify()
@@ -272,5 +314,6 @@ test('incorrect combinations of registrationOptions.name and routeOptions.pg.tra
       })
     })
   })
+
   t.end()
 })