Enabling stores to opt into seeing the request #712

boenrobot · 2019-11-06T16:38:29Z

Enabled stores to receive the current request as a second to last argument, by detecting the available arguments the handling function expects.

See #711 for a use case.

This approach should even keep existing stores compatible.

This is similar in goal to #643, except that it lets the store handle the request, whereas that other PR would ask the application to swap out the store.

…ument, by detecting the available arguments the handling function expects.

dougwilson

Please add some documentation around how stores opt in/put of this as well to the docs on building a store.

index.js

README.md

session/session.js

…gth to determine if the request should be passed to the store or not. To that end, also exposed the session options in the request, and moved the load method so that it is overridden in the request, instead of defined at the store, i.e. all stores get it "for free". Tweaked the "should set maxAge" cookie test to store the time at the start, making it non flaky. Added more tests.

SerayaEryn · 2019-11-13T20:58:04Z

README.md

@@ -278,6 +278,12 @@ all the elements will be considered when verifying the signature in requests.

 The session store instance, defaults to a new `MemoryStore` instance.

+##### passReqToStore


I wonder if it would be better to leave the decision whether passing the request is supported to the store.

I think it is, but the only ways I see are arity and classes with a instanceof check before every call... @dougwilson is against arity, and a new base class would make implementation significantly more complex... also I don't know for sure if instaneof even works on ancient Node versions.

I've checked out node.green and indeed, instanceof is only supported in Node 6.17.1 and up, meaning it's not an option... for now at least.

I have another idea... there could be a new reccomended method that would probe for a feature, and expect a true/false result, and assume false for everything if the method does not exist.

e.g. store.isSupported('req')

That would also address the concern with future changes to the store API - each change would be a new string, with the middleware expected to handle all possible combinations, or at least some documented ones in a known order.

It also enables the options to not be exposed in the request.

@dougwilson Thoughts?

SerayaEryn · 2019-11-13T21:02:26Z

README.md

@@ -441,14 +460,18 @@ 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)


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.

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.

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.

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.

boenrobot · 2020-01-06T13:44:25Z

@dougwilson I'd really like to know your opinion on the store reporting support via a method, as opposed to the current option where the middleware dictates it.

It's doable (but not trivial) to switch to that, so I'd like to know your thoughts before spending time on it.

A support reporting method smells a little like DOM's now obsolete hasFeature(), but despite the bad smell, it seems like it may be a good fit to solve all concerns.

dougwilson · 2020-01-06T13:46:33Z

My initial thought was it would just be a boolean property on the store. I'm sorry i haven't had a chance to look at the changes and just restarted going through my backlog of github issues yesterday, so haven't gotten here yet.

dougwilson · 2020-01-06T13:50:54Z

If you think that is a bad idea, I'm fine with that. My concern is just that a user doesn't have to know if the store supports passing req or not and this module can just figure it out automatically, but not using arity. I didn't suggest a specific mechanism, as hopefully the concern is concrete enough to make a suggestion you think is appropriate 👍

boenrobot · 2020-01-06T13:58:57Z

I think it's a good idea to let the store report support... I just don't think dedicated flags in the store (be they a property or a method returning a boolean) are a good way to do feature detection in general. Instanceof is a better approach, as it ties you to an adjustable base, but since that's not supported in older NodeJS versions, I guess a boolean reporting property/method will have to do.

Anyhow, OK then, I'll adjust it to use a boolean property as an indicator.

dougwilson · 2020-01-06T14:04:13Z

To be clear too, I'm not tied to using a boolean property at all, which is why i didn't suggest it from the start. If you think there is a better, more reliable way, let's use that.

…n the store that claims this support, and is treated as false if not defined for the sake of BC.

boenrobot · 2020-01-07T14:38:00Z

OK, I've done it, and I'm ready... I ended up with a property called "passReq". I know you said I could do it another way if I had ideas, but really, a method and a property were the only ones I got. And a property is more efficient for the trivial cases (either supports it always, or always doesn't), and there's little possibilities of changes that would require additional such properties, so I opted for that.

The tests are failing due to timing issues with maxAge, which is unrelated I think.

I made one of the tests have a 1s tolerance, but I'm not sure I should do the same for the other one. The other one is about "originalMaxAge"... You'd think that "originalMaxAge" must always equal the option supplied at the constructor, but that's not the case... It's more like "the max age at the time the maxAge getter was first called since the last expires setter call", which could be one millisecond later than than Cookie object creation.

For the originalMaxAge failing tests, I made the originalMaxAge be recorded at the first request, and subsequently compared to two new requests, 100ms apart from each other and the original. That solves the timing issue, and even if originalMaxAge is changed to mean "maxAge when it was last set", that would still work fine.

boenrobot · 2020-02-14T13:24:50Z

Well, @dougwilson ?

Also, on a related note, @SerayaEryn , should I adapt this for fastify-session "as is" right now?

desmap · 2020-05-13T08:26:51Z

@dougwilson any news on this?

ghinks · 2020-05-13T11:55:31Z

just coming up to speed on this and have read the original issue 711.

Hello @boenrobot, I am trying to help out with some of the issues in express to give the main developers a bit of space to do work. I'm looking into the original issue, the comments made there and at this PR. Please give me until the end of this week to digest this issue and the PR.

boenrobot · 2020-05-13T12:00:42Z

Just FYI, there were conflicts with some of the tests... I had made a different solution for the time flaky tests... Rather than give them a 1ms tolerance, I tried to make them time independent. Although now it seems like my fix doesn't quite work out. Feel free to restore those time dependent tests.

ghinks

My main concern is that although, with a bit more work, this is possible to implement. For such a widely used package who is getting the benefit fo these changes?

The two most widely used stores are the redis and the mongo stores.

For this to be useful for folks it is those stores that would need to change to support this.

Or you would have to write stores to implement it?

Please update the unit tests so that it passed CICD and I will come back to review again.

So my main question for you is are you proposing to implement store changes or create new stores to effect this change.

This is a well used module and I have not seen much call for this particular change.

Additionally I pulled your branch and ran it against mongo and redis locally to see that the current use cases worked.

ghinks · 2020-05-15T12:54:07Z

README.md

@@ -348,6 +348,11 @@ req.session.reload(function(err) {
 })
 ```

+#### Session.load(sid, callback)


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

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?

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.

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

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.

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.

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.

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

session/test/session.js

Line 1611 in 3b08fc7

describe('.reload()', function () {

or

session/test/session.js

Line 1570 in 3b08fc7

describe('.destroy()', function(){

or

session/test/session.js

Line 1725 in 3b08fc7

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?

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.

ghinks · 2020-05-15T12:55:25Z

README.md

@@ -441,14 +460,18 @@ 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)


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.

ghinks · 2020-05-15T12:57:15Z

index.js

+     * @param {Function} fn
+     * @api public
+     */
+    store.load = function(sid, fn){


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

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.

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.

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

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?

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.

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?

boenrobot · 2020-05-15T14:22:30Z

For such a widely used package who is getting the benefit fo these changes?

Every store that opts into it, and every user of such a store.

In terms of applications, the typical use case are applications like mine, where the application logic is the same and is applied over multiple similar databases.

As a more concrete example, imagine that rather than having a blogging application, you have an application that offers a hosting of blogs via a single platform (think "wix", "squarespace" and other similar services). Each user of the blogging platform has their blog on a different DB instance to minimize bottlenecks caused by other users. Now, of course, since Nodejs is single threaded, there will be some bottlenecks even with that setup, so you'll want to make multiple instances and load balance the different users between them. Ideally, you will have one Nodejs instance per database, but that means you need more RAM and CPU allocated every time a new user registers. Load balancing users between fewer nodejs instances which in turn don't care which DB they connect to allows you to smoothly scale up and down as much as you can afford... that is, based on your active users, rather than your registered users.

For this to be useful for folks it is those stores that would need to change to support this.

Or you would have to write stores to implement it?
...
So my main question for you is are you proposing to implement store changes or create new stores to effect this change.

Stores would need to support this. Users would need to pick a store that supports this. If not already supported, they'd need to either contribute support for it, or make a custom store that does support it.

Please update the unit tests so that it passed CICD and I will come back to review again.

ok, I guess I'll restore the flaky tests with 1ms tolerance then.

…q-stores

ghinks · 2020-05-24T13:08:34Z

I have contacted both the redis and the mongo connect maintainers. I am not sure of the value of this feature unless those stores implement this. My suggestion is that you reach out to them and raise PRs.

redis store question
mongo store question

ghinks · 2020-05-28T12:35:46Z

I closed related issue 711 as we have this PR. However I do not see demand for this new feature from the broader community. I am closing this PR and will re-open if we can see that there is a need/want for this feature. Please do not let this be seen as discouragement from engaging with the express community. I just believe that the benefits of adding the feature may be less than you hoped for.

Atrox · 2021-12-04T21:42:51Z

Hey @ghinks, I just wanted to chime in here and say that I would love to have this feature as well. Can this be reconsidered?

My use case is pretty simple: I want to use additional data from the request in my custom store to save it alongside the session data.

ghinks · 2021-12-04T22:10:00Z

I'm no longer helping out with express 😢

Atrox · 2021-12-04T23:13:26Z

Oh, my bad! Sorry for the ping then :).
Do you happen to know anyone who could be asked about this? I guess alternatively I could just create a new issue and ask again there.

boenrobot force-pushed the req-stores branch from 84eb342 to 4f51317 Compare November 6, 2019 16:44

Enabled stores to receive the current request as a second to last arg…

b5533d0

…ument, by detecting the available arguments the handling function expects.

boenrobot force-pushed the req-stores branch 2 times, most recently from 302b99a to 113bb93 Compare November 6, 2019 17:20

Added tests for stores supporting req in their methods.

5b91bec

boenrobot force-pushed the req-stores branch from 113bb93 to 5b91bec Compare November 6, 2019 19:04

This was referenced Nov 7, 2019

Get/set/destroy session based on request details #711

Closed

Added the ability for stores to optionally accept the request as an argument. SerayaEryn/fastify-session#79

Closed

dougwilson suggested changes Nov 13, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

session/session.js Outdated Show resolved Hide resolved

dougwilson added enhancement pr labels Nov 13, 2019

boenrobot force-pushed the req-stores branch 2 times, most recently from 18324b9 to 75f14d3 Compare November 13, 2019 12:42

boenrobot force-pushed the req-stores branch from 75f14d3 to c206548 Compare November 13, 2019 13:01

boenrobot requested a review from dougwilson November 13, 2019 13:06

SerayaEryn reviewed Nov 13, 2019

View reviewed changes

Removed the passReqToStore middleware option in favor of a property o…

609fbd7

…n the store that claims this support, and is treated as false if not defined for the sake of BC.

boenrobot force-pushed the req-stores branch 2 times, most recently from eb03564 to f0d947a Compare January 7, 2020 17:07

dougwilson mentioned this pull request Apr 26, 2020

Use 'req.sessionStore' instead of directly using 'store' object. #643

Open

boenrobot force-pushed the req-stores branch from 30a9f3a to e1b46f3 Compare May 13, 2020 10:51

ghinks reviewed May 15, 2020

View reviewed changes

Merge branch 'master' of https://github.com/expressjs/session into re…

30abd38

…q-stores

boenrobot force-pushed the req-stores branch from e1b46f3 to 30abd38 Compare May 15, 2020 15:02

This was referenced May 15, 2020

impact by proposed changes to express-session tj/connect-redis#304

Closed

impact by proposed changes to express-session jdesboeufs/connect-mongo#361

Closed

ghinks closed this May 28, 2020

		@@ -278,6 +278,12 @@ all the elements will be considered when verifying the signature in requests.

		The session store instance, defaults to a new `MemoryStore` instance.

		##### passReqToStore

Enabling stores to opt into seeing the request #712

Enabling stores to opt into seeing the request #712

Conversation

boenrobot commented Nov 6, 2019 • edited Loading

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boenrobot Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

boenrobot Nov 15, 2019 • 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

boenrobot May 15, 2020 • edited Loading

Choose a reason for hiding this comment

boenrobot commented Jan 6, 2020 • edited Loading

dougwilson commented Jan 6, 2020

dougwilson commented Jan 6, 2020

boenrobot commented Jan 6, 2020

dougwilson commented Jan 6, 2020

boenrobot commented Jan 7, 2020 • edited Loading

boenrobot commented Feb 14, 2020

desmap commented May 13, 2020

ghinks commented May 13, 2020

boenrobot commented May 13, 2020

ghinks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boenrobot May 15, 2020 • 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

dougwilson May 15, 2020 • edited Loading

Choose a reason for hiding this comment

boenrobot commented May 15, 2020 • edited Loading

ghinks commented May 24, 2020

ghinks commented May 28, 2020

Atrox commented Dec 4, 2021

ghinks commented Dec 4, 2021

Atrox commented Dec 4, 2021

boenrobot commented Nov 6, 2019 •

edited

Loading

boenrobot Nov 14, 2019 •

edited

Loading

boenrobot Nov 15, 2019 •

edited

Loading

boenrobot May 15, 2020 •

edited

Loading

boenrobot commented Jan 6, 2020 •

edited

Loading

boenrobot commented Jan 7, 2020 •

edited

Loading

ghinks left a comment •

edited

Loading

boenrobot May 15, 2020 •

edited

Loading

dougwilson May 15, 2020 •

edited

Loading

boenrobot commented May 15, 2020 •

edited

Loading