Skip to content

Commit 3f20296

Browse files
davidjbmweibel
authored andcommitted
Fix, simplify & test default options lookup
With #97, the default option for `tableName` was set as `Sessions`. However, given the previous logic for resolving options provided, the default was never utilised (`options.tableName` was used, which never [considered](https://github.com/mweibel/connect-session-sequelize/blob/394645289e17c2e962fe6650729b3f0e691a3134/lib/connect-session-sequelize.js#L68) the `defaultOptions` object). This was discovered on a database with no `CREATE` privileges where it tried to create a table called `Session`; this database had a pre-existing `Sessions` table that was created with an older version of `connect-session-sequelize` This change simplifies and cleans up the options lookup process, canonically using `this.options` for all options, ensuring `defaultOptions` is introduced during construction of the SequelizeStore. Additionally, this change fixes a subtle bug - the previous use of the `Object.assign` function had the side effect of modifying the first argument provided, which was the `defaultOptions` object. This would have impacted subsequent session stores as the defaults were accidentally modified by the first session store. The impact of this is isolated since an app with only 1 store wouldn't be affected and the original constructor didn't actually use the defaults (see above). This change adds various additional tests to ensure the option resolution succeeds.
1 parent e6f04dd commit 3f20296

File tree

2 files changed

+33
-10
lines changed

2 files changed

+33
-10
lines changed

lib/connect-session-sequelize.js

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,28 +44,25 @@ module.exports = function SequelizeSessionInit (Store) {
4444
class SequelizeStore extends Store {
4545
constructor (options) {
4646
super(options)
47-
this.options = options = options || {}
47+
this.options = { ...defaultOptions, ...(options || {}) }
4848

49-
if (!options.db) {
49+
if (!this.options.db) {
5050
throw new SequelizeStoreException('Database connection is required')
5151
}
5252

53-
this.options = Object.assign(defaultOptions, this.options)
54-
5553
this.startExpiringSessions()
5654

5755
// Check if specific table should be used for DB connection
58-
if (options.table) {
59-
debug('Using table: %s for sessions', options.table)
56+
if (this.options.table) {
57+
debug('Using table: %s for sessions', this.options.table)
6058
// Get Specifed Table from Sequelize Object
6159
this.sessionModel =
62-
options.db[options.table] || options.db.models[options.table]
60+
this.options.db[this.options.table] || this.options.db.models[this.options.table]
6361
} else {
6462
// No Table specified, default to ./model
6563
debug('No table specified, using default table.')
66-
const modelKey = options.modelKey || 'Session'
67-
this.sessionModel = options.db.define(modelKey, defaultModel, {
68-
tableName: options.tableName || modelKey
64+
this.sessionModel = this.options.db.define(this.options.modelKey, defaultModel, {
65+
tableName: this.options.tableName || this.options.modelKey
6966
})
7067
}
7168
}

test/test.js

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,32 @@ describe('store db', function () {
5151
var store = new SequelizeStore({ db: db, checkExpirationInterval: -1 })
5252
assert.strictEqual(store.sessionModel.name, 'Session')
5353
})
54+
55+
it('should use the default model key if not specified in options', function () {
56+
var store = new SequelizeStore({ db: db, checkExpirationInterval: -1 })
57+
assert.strictEqual(store.sessionModel.name, 'Session')
58+
})
59+
60+
it('should use an explicit model key', function () {
61+
var store = new SequelizeStore({ db: db, modelKey: 'CustomSessionModel', checkExpirationInterval: -1 })
62+
assert.strictEqual(store.sessionModel.name, 'CustomSessionModel')
63+
})
64+
65+
it('should use the default table name if not specified in options', function () {
66+
var store = new SequelizeStore({ db: db, checkExpirationInterval: -1 })
67+
assert.strictEqual(store.sessionModel.tableName, 'Sessions')
68+
})
69+
70+
it('should use an explicit table name', function () {
71+
var store = new SequelizeStore({ db: db, tableName: 'CustomSessionsTable', checkExpirationInterval: -1 })
72+
assert.strictEqual(store.sessionModel.tableName, 'CustomSessionsTable')
73+
})
74+
75+
it('should use explicit model/table options', function () {
76+
var store = new SequelizeStore({ db: db, modelKey: 'CustomSessionModel', tableName: 'CustomSessionsTable', checkExpirationInterval: -1 })
77+
assert.strictEqual(store.sessionModel.name, 'CustomSessionModel')
78+
assert.strictEqual(store.sessionModel.tableName, 'CustomSessionsTable')
79+
})
5480
})
5581

5682
describe('#set()', function () {

0 commit comments

Comments
 (0)