feat(azure-cosmosdb): add session context for a user mongodb #7436
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
dosubot
bot
added
the
size:M
dosubot
bot
added
the
auto:improvement
There was a problem hiding this comment.
Seems reasonable to me!
@sinedied do you mind giving this a look?
jacoblee93
added
question
There was a problem hiding this comment.
Thanks @crisjy !
The context handling looks fine here, but you're missing user handling, which is needed to list/delete sessions otherwise you have no distinction of users.
What we did in the other CosmosDB implementation is make userId parameter optional in the DB options and set it to anonymous by default to keep compatibility with other implementation (or when user handling is not needed).
| async clearAllSessions() { | ||
| await this.initialize(); | ||
| try { | ||
| await this.collection.deleteMany({}); |
There was a problem hiding this comment.
This would delete all sessions for all users, which might not be the intended behavior... I think you missed the user handling here
There was a problem hiding this comment.
yep, add user id as handle for sessions, thank you for your comment!
There was a problem hiding this comment.
I think you forgot to update this call to filter it for the current user only
There was a problem hiding this comment.
add user id as handler to clear sessions
|
@crisjy let me know when this is ready for another look! |
dosubot
bot
added
size:L
@jacoblee93 @sinedied resolve comments for this pr, sorry for delay, could you help me to have a look? thank you! |
|
|
||
| async getAllSessions(): Promise<ChatSessionMongo[]> { | ||
| await this.initialize(); | ||
| const documents = await this.collection.find().toArray(); |
| async clearAllSessions() { | ||
| await this.initialize(); | ||
| try { | ||
| await this.collection.deleteMany({}); |
There was a problem hiding this comment.
I think you forgot to update this call to filter it for the current user only
|
@sinedied @jacoblee93 comments resolved, could you help me have another look? Thank you! |
|
LGTM, thanks! |
|
@jacoblee93 Hi jacob, could you help me have another look at this PR, thanks! |
|
|
||
| try { | ||
| await this.collection.updateOne( | ||
| { [ID_KEY]: this.sessionId }, |
There was a problem hiding this comment.
Do we need [ID_USER]: this.userId, here too?
I assume this is sufficient in this case to just pass id?
jacoblee93
changed the title
Fixes # (issue)