Skip to content

Enabling stores to opt into seeing the request #712

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,11 @@ req.session.reload(function(err) {
})
```

#### Session.load(sid, callback)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was removed from the docs and should not be merged back in again.

Copy link
Author

@boenrobot boenrobot May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that omission was an oversight, and since I was asked to add docs anyway, I thought I'll add one for this as well.

Why was it removed from the docs though? It's still part of the Store prototype after all. If it's deprecated for whatever reason, shouldn't it be documented as such?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is part of this module, though. Can you link to where it is in source? I see no load method on our prototype, unless I am just looking past it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Hmm, I wonder why it is not documented... We should get it documented as a separate PR (and add tests if those are missing too), not bundled into this one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one test that uses it (and in effect, covers it) here:
https://github.com/expressjs/session/blob/master/test/session.js#L1467

I modeled the test at
https://github.com/expressjs/session/pull/712/files#diff-306b67b120079e997613897e45b88194R2329
in accordance with that very test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I didn't say there were no tests, only that documenting it (and adding tests if those are missing too) just needs to be moved into a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, looking at that test, we indeed do not have store.load tests. It would be around the public usage of that method (which is what documenting it in the README is for) and would be similar in scope to the other documented methods like

describe('.reload()', function () {
or
describe('.destroy()', function(){
or
describe('.save()', function () {
etc.

But regardless of the above, I do wonder: why would a store who is opting for support req passing not then take the req as an argument to this load method like it does for all the others?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/expressjs/session/blob/master/session/store.js#L67

P.S. That would be documented as Store.load, not Session.load. That is why I didn't see it went it went to look, as it's a method on the Store class and not the Session class.


Loads the the session data for the given `sid` from the store and re-populates the
`req.session` object. Once complete, the `callback` will be invoked.

#### Session.save(callback)

Save the session back to the store, replacing the contents on the store with the
Expand Down Expand Up @@ -441,14 +446,16 @@ For an example implementation view the [connect-redis](http://github.com/visionm
This optional method is used to get all sessions in the store as an array. The
`callback` should be called as `callback(error, sessions)`.

### store.destroy(sid, callback)
### store.destroy(sid, [req], callback)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be good to use an object to pass the request to the store to make future changes easier if something else needs to be passed to the store.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That concern is also the other reason I went for arity initially.

With the session object and options now being in the request, I think whatever else is needed should be covered from the request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are only really interested in the host could we not pass an object with specific props. This comment can be applied to all the places we pass req.

Copy link
Author

@boenrobot boenrobot May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am interested in the hostname for my use case, but I imagine other people might be interested in some other features in the request to make a decision where to go. Maybe you're fragmenting stores by geo IP, maybe you're fragmenting stores by a custom header, maybe you're fragmenting by URL prefix, or by query string. Whatever you need to do, you could do it. Stores should expose a way for developers to return settings based on whatever they need to do.


**Required**

This required method is used to destroy/delete a session from the store given
a session ID (`sid`). The `callback` should be called as `callback(error)` once
the session is destroyed.

The request is passed into the `req` argument only if the store sets `store.passReq` to `true`.

### store.clear(callback)

**Optional**
Expand All @@ -463,7 +470,7 @@ This optional method is used to delete all sessions from the store. The
This optional method is used to get the count of all sessions in the store.
The `callback` should be called as `callback(error, len)`.

### store.get(sid, callback)
### store.get(sid, [req], callback)

**Required**

Expand All @@ -474,15 +481,19 @@ The `session` argument should be a session if found, otherwise `null` or
`undefined` if the session was not found (and there was no error). A special
case is made when `error.code === 'ENOENT'` to act like `callback(null, null)`.

### store.set(sid, session, callback)
The request is passed into the `req` argument only if the store sets `store.passReq` to `true`.

### store.set(sid, session, [req], callback)

**Required**

This required method is used to upsert a session into the store given a
session ID (`sid`) and session (`session`) object. The callback should be
called as `callback(error)` once the session has been set in the store.

### store.touch(sid, session, callback)
The request is passed into the `req` argument only if the store sets `store.passReq` to `true`.

### store.touch(sid, session, [req], callback)

**Recommended**

Expand All @@ -494,6 +505,17 @@ This is primarily used when the store will automatically delete idle sessions
and this method is used to signal to the store the given session is active,
potentially resetting the idle timer.

The request is passed into the `req` argument only if the store sets `store.passReq` to `true`.

### store.passReq

**Optional**

A property determining whether the request is passed to the store in the other methods.
Defaults to `false` if left undefined.

A store wishing to operate in either mode (in order to maintain compatibility with earlier versions of this package) may set this to `true`, but check the arguments given to each method.

## Compatible Session Stores

The following modules implement a session store that is compatible with this
Expand Down
51 changes: 44 additions & 7 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ function session(options) {

// get the session store
var store = opts.store || new MemoryStore()

// get the trust proxy setting
var trustProxy = opts.proxy

Expand Down Expand Up @@ -201,6 +200,27 @@ function session(options) {
return;
}

/**
* Load a `Session` instance via the given `sid`
* and invoke the callback `fn(err, sess)`.
*
* @param {String} sid
* @param {Function} fn
* @api public
*/
store.load = function(sid, fn){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, totally get it. But please remember that this is adding complexity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it adding complexity? load() already exists in the prototype. I merely moved it from there to here, so that even stores that don't extend from Store can take advantage of load() without having to explicitly add the req to the argument list or doing some uglier hack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would override any load method or property a store has defined. We need to validate that this will not conflict with any existing store implementations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're asking if stores implemented a previously undocumented method in the Store prototype that won't even be there if this PR is merged as is.... ok 😕

I checked whether the following stores define their own load methods (be it as an override to the prototype one or a custom one). Feel free to double check if you don't believe me...

Unless otherwise noted, all stores inherit from the Store prototype (either explicitly or from a supplied object's Store property), but don't implement load of their own.

aerospike-session-store-expressjs - no
cassandra-store - no
cluster-store - no (doesn't inherit from Store prototype in its memory store, and doesn't define a load() method)
connect-arango - no
connect-azuretables - no
connect-cloudant-store - no
connect-couchbase - no
connect-datacache - no
@google-cloud/connect-datastore - no
connect-db2 - no
connect-dynamodb - no
@google-cloud/connect-firestore - no
connect-hazelcast - no
connect-loki - no
connect-memcached - no
connect-memjs - no
connect-ml - no
connect-monetdb - no
connect-mongo - no
connect-mongodb-session - no
connect-mssql - no (side note: Abandoned; Even if there was an issue, users should move away anyway)
connect-pg-simple - no
connect-redis - no
connect-session-firebase - no
connect-session-knex - no
connect-session-sequelize - no
connect-sqlite3 - no
connect-typeorm - no
couchdb-expression - no
documentdb-session - no (Abandoned due to dep deprecation; Even if there was an issue, users should move away anyway)
dynamodb-store - no
express-etcd - no
express-mysql-session - no
express-nedb-session - no
express-oracle-session - no
express-session-cache-manager - no
express-session-etcd3 - no
express-session-level - no
express-session-rsdb - no
express-sessions - no
firestore-store - no
fortune-session - no
hazelcast-store - no
level-session-store - no
lowdb-session-store - no
medea-session-store - no
memorystore - no
mssql-session-store - no
nedb-session-store - no
restsession - no
sequelstore-connect - no
session-file-store - no
session-pouchdb-store - no
session-rethinkdb - no
sessionstore - no
tch-nedb-session - no

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, sorry, excuse me if I'm confused is all 😂 . So if none of the stores are implementing this besides what they get from the store base, why perform this override? Why not simply modify the base method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only time when load can even be used (with or without this PR) is in a manual call within the request. It seems redundant to require the request for something that can only be done in the request.

Copy link
Contributor

@dougwilson dougwilson May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure what you mean, but it sounds like this is going to introduce some sort of race condition. I didn't realize you were adding a method to the store (effectively a global) while closing over a request... would not a call to req.sessionStore.load by a user then then up with a different internal req reference?

var getCallback = function(err, sess){
if (err) return fn(err);
if (!sess) return fn();
fn(null, store.createSession(req, sess))
};
if (store.passReq) {
store.get(sid, req, getCallback);
} else {
store.get(sid, getCallback);
}
};

// backwards compatibility for signed cookies
// req.secret is passed from the cookie parser middleware
var secrets = secret || [req.secret];
Expand Down Expand Up @@ -303,14 +323,20 @@ function session(options) {
if (shouldDestroy(req)) {
// destroy session
debug('destroying');
store.destroy(req.sessionID, function ondestroy(err) {
var ondestroy = function(err) {
if (err) {
defer(next, err);
}

debug('destroyed');
writeend();
});
}

if (store.passReq) {
store.destroy(req.sessionID, req, ondestroy);
} else {
store.destroy(req.sessionID, ondestroy);
}

return writetop();
}
Expand Down Expand Up @@ -340,14 +366,20 @@ function session(options) {
} else if (storeImplementsTouch && shouldTouch(req)) {
// store implements touch method
debug('touching');
store.touch(req.sessionID, req.session, function ontouch(err) {
var ontouch = function(err) {
if (err) {
defer(next, err);
}

debug('touched');
writeend();
});
}

if (store.passReq) {
store.touch(req.sessionID, req.session, req, ontouch);
} else {
store.touch(req.sessionID, req.session, ontouch);
}

return writetop();
}
Expand Down Expand Up @@ -478,7 +510,7 @@ function session(options) {

// generate the session object
debug('fetching %s', req.sessionID);
store.get(req.sessionID, function(err, sess){
var getHandler = function(err, sess){
// error handling
if (err && err.code !== 'ENOENT') {
debug('error %j', err);
Expand All @@ -500,7 +532,12 @@ function session(options) {
}

next()
});
}
if (store.passReq) {
store.get(req.sessionID, req, getHandler);
} else {
store.get(req.sessionID, getHandler);
}
};
};

Expand Down
21 changes: 17 additions & 4 deletions session/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ defineMethod(Session.prototype, 'resetMaxAge', function resetMaxAge() {
*/

defineMethod(Session.prototype, 'save', function save(fn) {
this.req.sessionStore.set(this.id, this, fn || function(){});
if (this.req.sessionStore.passReq) {
this.req.sessionStore.set(this.id, this, this.req, fn || function(){});
} else {
this.req.sessionStore.set(this.id, this, fn || function(){});
}
return this;
});

Expand All @@ -89,12 +93,17 @@ defineMethod(Session.prototype, 'reload', function reload(fn) {
var req = this.req
var store = this.req.sessionStore

store.get(this.id, function(err, sess){
var getCallback = function(err, sess){
if (err) return fn(err);
if (!sess) return fn(new Error('failed to load session'));
store.createSession(req, sess);
fn();
});
};
if (store.passReq) {
store.get(this.id, req, getCallback);
} else {
store.get(this.id, getCallback);
}
return this;
});

Expand All @@ -108,7 +117,11 @@ defineMethod(Session.prototype, 'reload', function reload(fn) {

defineMethod(Session.prototype, 'destroy', function destroy(fn) {
delete this.req.session;
this.req.sessionStore.destroy(this.id, fn);
if (this.req.sessionStore.passReq) {
this.req.sessionStore.destroy(this.id, this.req, fn);
} else {
this.req.sessionStore.destroy(this.id, fn);
}
return this;
});

Expand Down
28 changes: 7 additions & 21 deletions session/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,29 +49,15 @@ util.inherits(Store, EventEmitter)

Store.prototype.regenerate = function(req, fn){
var self = this;
this.destroy(req.sessionID, function(err){
var destroyCallback = function(err){
self.generate(req);
fn(err);
});
};

/**
* Load a `Session` instance via the given `sid`
* and invoke the callback `fn(err, sess)`.
*
* @param {String} sid
* @param {Function} fn
* @api public
*/

Store.prototype.load = function(sid, fn){
var self = this;
this.get(sid, function(err, sess){
if (err) return fn(err);
if (!sess) return fn();
var req = { sessionID: sid, sessionStore: self };
fn(null, self.createSession(req, sess))
});
};
if (self.passReq) {
this.destroy(req.sessionID, req, destroyCallback);
} else {
this.destroy(req.sessionID, destroyCallback);
}
};

/**
Expand Down
7 changes: 4 additions & 3 deletions test/cookie.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,12 @@ describe('new Cookie()', function () {
})

it('should set maxAge', function () {
var expires = new Date(Date.now() + 60000)
var now = Date.now()
var expires = new Date(now + 60000)
var cookie = new Cookie({ expires: expires })

assert.ok(expires.getTime() - Date.now() - 1000 <= cookie.maxAge)
assert.ok(expires.getTime() - Date.now() + 1000 >= cookie.maxAge)
assert.ok(expires.getTime() - now - 1000 <= cookie.maxAge)
assert.ok(expires.getTime() - now + 1000 >= cookie.maxAge)
})
})

Expand Down
Loading