Skip to content
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

Use softdelete to destroy session to prevent race condition and re-save #26

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

0x0ece
Copy link
Collaborator

@0x0ece 0x0ece commented Jun 9, 2021

Discussion in #25.

@@ -21,7 +21,7 @@
"supertest": "^3.1.0",
"test-all-versions": "^4.1.0",
"tslint": "^5.8.0",
"typeorm": "^0.1.4",
"typeorm": "^0.2.34",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

soft delete is already impl in typeorm, but we need to upgrade to 0.2.3+. ok to upgrade to latest?

@@ -195,15 +237,26 @@ class Test {
this.express.get("/views", (req, res) => {
const session = nullthrows(req.session);

res.json(session.views || 0);
res.json((session as any).views || 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm having some issues with TS... I had to add "as any" in many places... will remove as soon as I understand how.

)
// @ts-ignore
.then(async () => {
try {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the new logic seems very different but looking at the actual queries is very similar.

repository.save does a select, then decides to do an update vs an insert.

the most important thing, however, is to add a condition that destroyedAt IS NULL in the update. typeorm soft delete doesn't add this clause by default, unfortunately. Without this condition, during the race condition the session is updated (in the test above, views becomes 3).

@nykula nykula merged commit b29dbb6 into freshgiammi-lab:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants