Feature/MongoDB Session Functions #1142
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1142 +/- ##
=======================================
Coverage 98.94% 98.94%
=======================================
Files 96 96
Lines 1712 1712
Branches 404 404
=======================================
Hits 1694 1694
Misses 18 18 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Hi @SimonGuethler 👋
Thank you for your PR. These methods were definitively missing for NoSQL databases.
I added some quick notes on the PR 👍
| * @param user | ||
| */ | ||
| async destroyAllSessionsOf(user: { id: string }): Promise<void> { | ||
| await this.collection.deleteMany({ 'state.userId': user.id.toString() }); |
There was a problem hiding this comment.
Why calling the toString() function? Isn't user.id sufficient in this case?
There was a problem hiding this comment.
I notice that if the ID of the user is _id, the functions won't work. Maybe we should change both functions of the MongoDB store and functions in TypeORM store in v3 to take the ID as parameter and not user objects.
There was a problem hiding this comment.
Yes that's correct, that's a little weird and needs a better solution. The problem lays in the missing ObjectID type, at least if it's dedicated to MongoDB and TypeORM. We could only take the id as a string as parameter which differs from the way Sessions are used with other databases in foal, but it would make it to be usable more universally across ORMs for MongoDB.
There was a problem hiding this comment.
After working with TypeORM v0.3 for Foal version 3, it appears that the ORM badly supports the use of id instead of _id for MongoDB with this version.
Yes, I think that we should then let the user use the form of their choice and thus take only the ID as parameter and not the user object. I updated the code of TypeORMStore accordingly for v3 to make the methods consistent across the framework (event though the ID type is different): #1152
| it('should destroy all sessions of a user', async () => { | ||
| const user = { id: 'asdf' }; | ||
| await store.destroyAllSessionsOf(user); | ||
| const sessions = await store.getSessionsOf(user); |
There was a problem hiding this comment.
I think we could use the MongoDB driver here and not the method getSessionsOf in order to make unit tests more independent from each other. If at some point the getSessionsOf method fails or is removed, this test won't fail and won't need a change.
There was a problem hiding this comment.
That makes sense. I'll implement that.
| await createSessionTestData(); | ||
| }); | ||
|
|
||
| it('should return all distinct user ids', async () => { |
There was a problem hiding this comment.
Maybe we should add another test or extend this one here to test the case that, when a user is not authenticated (i.e userId === null), the method does not return a null value in its returned array.
|
Hi @SimonGuethler 👋 Did you get a chance to move foward on this PR? 😊 |
|
Hi @SimonGuethler 👋 This PR is a year old, so I'm going to close it. If you want to still work on it, don't hesitate to reopen it. 👍 |
Issue
The MongoDB session store service is missing some functions, which we needed in a past project. In that, we were using FoalTS with MongoDB, and we extended those functionalities. This PR is supposed to contribute them back.
Solution and steps
Checklist