diff --git a/.gitignore b/.gitignore index df95fda07..bae2a20a1 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ package-lock.json *.swp dist .DS_Store +.vscode/ diff --git a/.travis.yml b/.travis.yml index 61a7a79af..7ea347e9b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,7 +10,7 @@ env: node_js: - lts/dubnium - lts/erbium - - 13 + - 13.6 addons: postgresql: "10" diff --git a/packages/pg-pool/index.js b/packages/pg-pool/index.js index 1c7faf210..e144bb83b 100644 --- a/packages/pg-pool/index.js +++ b/packages/pg-pool/index.js @@ -25,6 +25,10 @@ class PendingItem { } } +function throwOnDoubleRelease () { + throw new Error('Release called on client which has already been released to the pool.') +} + function promisify (Promise, callback) { if (callback) { return { callback: callback, result: undefined } @@ -60,6 +64,18 @@ class Pool extends EventEmitter { constructor (options, Client) { super() this.options = Object.assign({}, options) + + if (options != null && 'password' in options) { + // "hiding" the password so it doesn't show up in stack traces + // or if the client is console.logged + Object.defineProperty(this.options, 'password', { + configurable: true, + enumerable: false, + writable: true, + value: options.password + }) + } + this.options.max = this.options.max || this.options.poolSize || 10 this.log = this.options.log || function () { } this.Client = this.options.Client || Client || require('pg').Client @@ -244,7 +260,7 @@ class Pool extends EventEmitter { client.release = (err) => { if (released) { - throw new Error('Release called on client which has already been released to the pool.') + throwOnDoubleRelease() } released = true @@ -280,7 +296,8 @@ class Pool extends EventEmitter { _release (client, idleListener, err) { client.on('error', idleListener) - if (err || this.ending) { + // TODO(bmc): expose a proper, public interface _queryable and _ending + if (err || this.ending || !client._queryable || client._ending) { this._remove(client) this._pulseQueue() return diff --git a/packages/pg-pool/package.json b/packages/pg-pool/package.json index dc0275699..a1fa1465e 100644 --- a/packages/pg-pool/package.json +++ b/packages/pg-pool/package.json @@ -34,6 +34,6 @@ "pg-cursor": "^1.3.0" }, "peerDependencies": { - "pg": ">5.0" + "pg": ">=8.0" } } diff --git a/packages/pg-pool/test/releasing-clients.js b/packages/pg-pool/test/releasing-clients.js new file mode 100644 index 000000000..8fccde36e --- /dev/null +++ b/packages/pg-pool/test/releasing-clients.js @@ -0,0 +1,54 @@ +const Pool = require('../') + +const expect = require('expect.js') +const net = require('net') + +describe('releasing clients', () => { + it('removes a client which cannot be queried', async () => { + // make a pool w/ only 1 client + const pool = new Pool({ max: 1 }) + expect(pool.totalCount).to.eql(0) + const client = await pool.connect() + expect(pool.totalCount).to.eql(1) + expect(pool.idleCount).to.eql(0) + // reach into the client and sever its connection + client.connection.end() + + // wait for the client to error out + const err = await new Promise((resolve) => client.once('error', resolve)) + expect(err).to.be.ok() + expect(pool.totalCount).to.eql(1) + expect(pool.idleCount).to.eql(0) + + // try to return it to the pool - this removes it because its broken + client.release() + expect(pool.totalCount).to.eql(0) + expect(pool.idleCount).to.eql(0) + + // make sure pool still works + const { rows } = await pool.query('SELECT NOW()') + expect(rows).to.have.length(1) + await pool.end() + }) + + it('removes a client which is ending', async () => { + // make a pool w/ only 1 client + const pool = new Pool({ max: 1 }) + expect(pool.totalCount).to.eql(0) + const client = await pool.connect() + expect(pool.totalCount).to.eql(1) + expect(pool.idleCount).to.eql(0) + // end the client gracefully (but you shouldn't do this with pooled clients) + client.end() + + // try to return it to the bool + client.release() + expect(pool.totalCount).to.eql(0) + expect(pool.idleCount).to.eql(0) + + // make sure pool still works + const { rows } = await pool.query('SELECT NOW()') + expect(rows).to.have.length(1) + await pool.end() + }) +}) diff --git a/packages/pg/Makefile b/packages/pg/Makefile index 52d0545d3..a5b0bc1da 100644 --- a/packages/pg/Makefile +++ b/packages/pg/Makefile @@ -62,6 +62,4 @@ test-pool: lint: @echo "***Starting lint***" - node -e "process.exit(Number(process.versions.node.split('.')[0]) < 8 ? 0 : 1)" \ - && echo "***Skipping lint (node version too old)***" \ - || node_modules/.bin/eslint lib + node_modules/.bin/eslint lib diff --git a/packages/pg/lib/client.js b/packages/pg/lib/client.js index 93807e48c..c929d26f3 100644 --- a/packages/pg/lib/client.js +++ b/packages/pg/lib/client.js @@ -30,7 +30,16 @@ var Client = function (config) { this.database = this.connectionParameters.database this.port = this.connectionParameters.port this.host = this.connectionParameters.host - this.password = this.connectionParameters.password + + // "hiding" the password so it doesn't show up in stack traces + // or if the client is console.logged + Object.defineProperty(this, 'password', { + configurable: true, + enumerable: false, + writable: true, + value: this.connectionParameters.password + }) + this.replication = this.connectionParameters.replication var c = config || {} diff --git a/packages/pg/lib/connection-parameters.js b/packages/pg/lib/connection-parameters.js index 0d5e0376d..cd6d3b8a9 100644 --- a/packages/pg/lib/connection-parameters.js +++ b/packages/pg/lib/connection-parameters.js @@ -52,9 +52,23 @@ var ConnectionParameters = function (config) { this.user = val('user', config) this.database = val('database', config) + + if (this.database === undefined) { + this.database = this.user + } + this.port = parseInt(val('port', config), 10) this.host = val('host', config) - this.password = val('password', config) + + // "hiding" the password so it doesn't show up in stack traces + // or if the client is console.logged + Object.defineProperty(this, 'password', { + configurable: true, + enumerable: false, + writable: true, + value: val('password', config) + }) + this.binary = val('binary', config) this.ssl = typeof config.ssl === 'undefined' ? useSsl() : config.ssl this.client_encoding = val('client_encoding', config) diff --git a/packages/pg/lib/defaults.js b/packages/pg/lib/defaults.js index 120b8c7b5..eb58550d6 100644 --- a/packages/pg/lib/defaults.js +++ b/packages/pg/lib/defaults.js @@ -15,7 +15,7 @@ module.exports = { user: process.platform === 'win32' ? process.env.USERNAME : process.env.USER, // name of database to connect - database: process.platform === 'win32' ? process.env.USERNAME : process.env.USER, + database: undefined, // database user's password password: null, diff --git a/packages/pg/lib/index.js b/packages/pg/lib/index.js index de33c086d..c73064cf2 100644 --- a/packages/pg/lib/index.js +++ b/packages/pg/lib/index.js @@ -7,25 +7,17 @@ * README.md file in the root directory of this source tree. */ -var util = require('util') var Client = require('./client') var defaults = require('./defaults') var Connection = require('./connection') var Pool = require('pg-pool') -const checkConstructor = require('./compat/check-constructor') const poolFactory = (Client) => { - var BoundPool = function (options) { - // eslint-disable-next-line no-eval - checkConstructor('pg.Pool', 'PG-POOL-NEW', () => eval('new.target')) - - var config = Object.assign({ Client: Client }, options) - return new Pool(config) + return class BoundPool extends Pool { + constructor (options) { + super(options, Client) + } } - - util.inherits(BoundPool, Pool) - - return BoundPool } var PG = function (clientConstructor) { @@ -44,20 +36,28 @@ if (typeof process.env.NODE_PG_FORCE_NATIVE !== 'undefined') { module.exports = new PG(Client) // lazy require native module...the native module may not have installed - module.exports.__defineGetter__('native', function () { - delete module.exports.native - var native = null - try { - native = new PG(require('./native')) - } catch (err) { - if (err.code !== 'MODULE_NOT_FOUND') { - throw err + Object.defineProperty(module.exports, 'native', { + configurable: true, + enumerable: false, + get() { + var native = null + try { + native = new PG(require('./native')) + } catch (err) { + if (err.code !== 'MODULE_NOT_FOUND') { + throw err + } + /* eslint-disable no-console */ + console.error(err.message) + /* eslint-enable no-console */ } - /* eslint-disable no-console */ - console.error(err.message) - /* eslint-enable no-console */ + + // overwrite module.exports.native so that getter is never called again + Object.defineProperty(module.exports, 'native', { + value: native + }) + + return native } - module.exports.native = native - return native }) } diff --git a/packages/pg/lib/native/client.js b/packages/pg/lib/native/client.js index 6859bc2cc..d06166573 100644 --- a/packages/pg/lib/native/client.js +++ b/packages/pg/lib/native/client.js @@ -43,7 +43,15 @@ var Client = module.exports = function (config) { // for the time being. TODO: deprecate all this jazz var cp = this.connectionParameters = new ConnectionParameters(config) this.user = cp.user - this.password = cp.password + + // "hiding" the password so it doesn't show up in stack traces + // or if the client is console.logged + const hiddenPassword = cp.password + Object.defineProperty(this, 'password', { + enumerable: false, + writable: true, + value: hiddenPassword + }) this.database = cp.database this.host = cp.host this.port = cp.port diff --git a/packages/pg/package.json b/packages/pg/package.json index 91e27a887..5ca6f262f 100644 --- a/packages/pg/package.json +++ b/packages/pg/package.json @@ -51,6 +51,6 @@ ], "license": "MIT", "engines": { - "node": ">= 4.5.0" + "node": ">= 8.0.0" } } diff --git a/packages/pg/test/integration/client/configuration-tests.js b/packages/pg/test/integration/client/configuration-tests.js index 87bb52d47..a6756ddee 100644 --- a/packages/pg/test/integration/client/configuration-tests.js +++ b/packages/pg/test/integration/client/configuration-tests.js @@ -14,7 +14,7 @@ for (var key in process.env) { suite.test('default values are used in new clients', function () { assert.same(pg.defaults, { user: process.env.USER, - database: process.env.USER, + database: undefined, password: null, port: 5432, rows: 0, @@ -54,6 +54,28 @@ suite.test('modified values are passed to created clients', function () { }) }) +suite.test('database defaults to user when user is non-default', () => { + { + pg.defaults.database = undefined + + const client = new Client({ + user: 'foo', + }) + + assert.strictEqual(client.database, 'foo') + } + + { + pg.defaults.database = 'bar' + + const client = new Client({ + user: 'foo', + }) + + assert.strictEqual(client.database, 'bar') + } +}) + suite.test('cleanup', () => { // restore process.env for (var key in realEnv) { diff --git a/packages/pg/test/integration/gh-issues/1542-tests.js b/packages/pg/test/integration/gh-issues/1542-tests.js new file mode 100644 index 000000000..4d30d6020 --- /dev/null +++ b/packages/pg/test/integration/gh-issues/1542-tests.js @@ -0,0 +1,25 @@ + +"use strict" +const helper = require('./../test-helper') +const assert = require('assert') + +const suite = new helper.Suite() + +suite.testAsync('BoundPool can be subclassed', async () => { + const Pool = helper.pg.Pool; + class SubPool extends Pool { + + } + const subPool = new SubPool() + const client = await subPool.connect() + client.release() + await subPool.end() + assert(subPool instanceof helper.pg.Pool) +}) + +suite.test('calling pg.Pool without new throws', () => { + const Pool = helper.pg.Pool; + assert.throws(() => { + const pool = Pool() + }) +}) diff --git a/packages/pg/test/integration/gh-issues/1992-tests.js b/packages/pg/test/integration/gh-issues/1992-tests.js new file mode 100644 index 000000000..1832f5f8a --- /dev/null +++ b/packages/pg/test/integration/gh-issues/1992-tests.js @@ -0,0 +1,11 @@ + +"use strict" +const helper = require('./../test-helper') +const assert = require('assert') + +const suite = new helper.Suite() + +suite.test('Native should not be enumerable', () => { + const keys = Object.keys(helper.pg) + assert.strictEqual(keys.indexOf('native'), -1) +}) diff --git a/packages/pg/test/integration/gh-issues/2064-tests.js b/packages/pg/test/integration/gh-issues/2064-tests.js new file mode 100644 index 000000000..64c150bd0 --- /dev/null +++ b/packages/pg/test/integration/gh-issues/2064-tests.js @@ -0,0 +1,32 @@ + +"use strict" +const helper = require('./../test-helper') +const assert = require('assert') +const util = require('util') + +const suite = new helper.Suite() + +const password = 'FAIL THIS TEST' + +suite.test('Password should not exist in toString() output', () => { + const pool = new helper.pg.Pool({ password }) + const client = new helper.pg.Client({ password }) + assert(pool.toString().indexOf(password) === -1); + assert(client.toString().indexOf(password) === -1); +}) + +suite.test('Password should not exist in util.inspect output', () => { + const pool = new helper.pg.Pool({ password }) + const client = new helper.pg.Client({ password }) + const depth = 20; + assert(util.inspect(pool, { depth }).indexOf(password) === -1); + assert(util.inspect(client, { depth }).indexOf(password) === -1); +}) + +suite.test('Password should not exist in json.stringfy output', () => { + const pool = new helper.pg.Pool({ password }) + const client = new helper.pg.Client({ password }) + const depth = 20; + assert(JSON.stringify(pool).indexOf(password) === -1); + assert(JSON.stringify(client).indexOf(password) === -1); +}) diff --git a/packages/pg/test/unit/connection-parameters/creation-tests.js b/packages/pg/test/unit/connection-parameters/creation-tests.js index 5d200be0a..fdb4e6627 100644 --- a/packages/pg/test/unit/connection-parameters/creation-tests.js +++ b/packages/pg/test/unit/connection-parameters/creation-tests.js @@ -16,8 +16,13 @@ test('ConnectionParameters construction', function () { }) var compare = function (actual, expected, type) { + const expectedDatabase = + expected.database === undefined + ? expected.user + : expected.database + assert.equal(actual.user, expected.user, type + ' user') - assert.equal(actual.database, expected.database, type + ' database') + assert.equal(actual.database, expectedDatabase, type + ' database') assert.equal(actual.port, expected.port, type + ' port') assert.equal(actual.host, expected.host, type + ' host') assert.equal(actual.password, expected.password, type + ' password') diff --git a/packages/pg/test/unit/connection-pool/configuration-tests.js b/packages/pg/test/unit/connection-pool/configuration-tests.js new file mode 100644 index 000000000..10c991839 --- /dev/null +++ b/packages/pg/test/unit/connection-pool/configuration-tests.js @@ -0,0 +1,14 @@ +'use strict' + +const assert = require('assert') +const helper = require('../test-helper') + +test('pool with copied settings includes password', () => { + const original = new helper.pg.Pool({ + password: 'original', + }) + + const copy = new helper.pg.Pool(original.options) + + assert.equal(copy.options.password, 'original') +})