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

✨ add transaction types and api route helpers #3271

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

danyx23
Copy link
Contributor

@danyx23 danyx23 commented Feb 29, 2024

This PR adds custom types for read only transactions and read/write transactions. Through some typescript magic this is set up so that functions that need a read only transaction (because they only run select statements) can also be given read/write transactions but functions that need read/write transactions do not accept a readonly transaction.

This PR also adds helper functions for API request handlers that also set up a transaction (which means that instead of writing a callback that takes a (request, response) tuple you write one that works with a (request, reqsponse, transaction) tuple).

Comment on lines +54 to +55
// TODO: consider adding this and whitelisting all promises that don't need to be awaited with "void"
// "@typescript-eslint/no-floating-promises": "error",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should do this once the whole stack is converted to knex. This will be a bit annoying because there are about 250 promises in our codebase that are not explicitly awaited, many of them totally fine (e.g. react event handlers that don't await the result of the handler); but now that we switch to transactions on all api router handlers, failing to await a promise could potentially try to access the database transaction after it has been commited or rolled back. Cases where this happens were an error before already (e.g. errors in these promises would not be reported back) and could be related to #3159 and other similar issues.

Copy link
Member

@marcelgerber marcelgerber left a comment

Choose a reason for hiding this comment

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

Nice, hard work!

What I did as part of this review:

  • Went over (pretty much) everything and checked that it makes rough sense
  • For apiRouter in particular, checked that we never use a transaction with the wrong kind of permissions
  • As a sanity check, I made the RO and RW types incompatible to see all the places where a RW transaction is passed to a method requiring a RO transaction - and all of these places make sense, i.e. the caller actually writes something.

It would be great if we could somehow also remove the .insert, .update etc. methods from the RO transaction altogether, but I don't believe this to be possible given the query builder nature of knex. A man can dream...

db/tests/basic.test.ts Outdated Show resolved Hide resolved
adminSiteServer/mockSiteRouter.tsx Outdated Show resolved Hide resolved
adminSiteServer/apiRouter.ts Outdated Show resolved Hide resolved
adminSiteServer/apiRouter.ts Show resolved Hide resolved
Copy link
Contributor Author

danyx23 commented Mar 26, 2024

Merge activity

  • Mar 26, 10:04 AM EDT: @danyx23 started a stack merge that includes this pull request via Graphite.
  • Mar 26, 10:08 AM EDT: Graphite rebased this pull request as part of a merge.
  • Mar 26, 10:09 AM EDT: @danyx23 merged this pull request with Graphite.

Base automatically changed from db-migrate-postlink to master March 26, 2024 14:07
@danyx23 danyx23 force-pushed the db-introduce-transaction-types branch from 05c8deb to b603737 Compare March 26, 2024 14:08
@danyx23 danyx23 merged commit fb7a78c into master Mar 26, 2024
16 of 20 checks passed
@danyx23 danyx23 deleted the db-introduce-transaction-types branch March 26, 2024 14:09
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