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.

Sign up for GitHub

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
Merged

✨ add transaction types and api route helpers #3271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ module.exports = {
"react/no-render-return-value": "warn",
"react/no-unescaped-entities": ["warn", { forbid: [">", "}"] }],
"react/prop-types": "warn",
// TODO: consider adding this and whitelisting all promises that don't need to be awaited with "void"
// "@typescript-eslint/no-floating-promises": "error",
Comment on lines +54 to +55
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.

},
settings: {
"import/resolver": {
Expand Down
58 changes: 30 additions & 28 deletions adminSiteServer/adminRouter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ adminRouter.get("/logout", logOut)
adminRouter.get("/datasets/:datasetId.csv", async (req, res) => {
const datasetId = expectInt(req.params.datasetId)

await db.knexInstance().transaction(async (t) => {
await db.knexReadonlyTransaction(async (t) => {
const datasetName = (
await db.knexRawFirst<Pick<DbPlainDataset, "name">>(
t,
Expand All @@ -166,19 +166,19 @@ adminRouter.get("/datasets/:datasetId/downloadZip", async (req, res) => {

res.attachment("additional-material.zip")

const knex = db.knexInstance()

const file = await db.knexRawFirst<
Pick<DbPlainDatasetFile, "filename" | "file">
>(knex, `SELECT filename, file FROM dataset_files WHERE datasetId=?`, [
datasetId,
])
const file = await await db.knexReadonlyTransaction((knex) =>
db.knexRawFirst<Pick<DbPlainDatasetFile, "filename" | "file">>(
knex,
`SELECT filename, file FROM dataset_files WHERE datasetId=?`,
[datasetId]
)
)
res.send(file?.file)
})

adminRouter.get("/posts/preview/:postId", async (req, res) => {
const postId = expectInt(req.params.postId)
const preview = await db.knexInstance().transaction(async (knex) => {
const preview = await db.knexReadonlyTransaction(async (knex) => {
return renderPreview(postId, knex)
})
res.send(preview)
Expand All @@ -187,16 +187,16 @@ adminRouter.get("/posts/preview/:postId", async (req, res) => {
adminRouter.get("/posts/compare/:postId", async (req, res) => {
const postId = expectInt(req.params.postId)

const [wpPage, archieMlText] = await db
.knexInstance()
.transaction(async (t) => {
const [wpPage, archieMlText] = await db.knexReadonlyTransaction(
async (t) => {
const wpPage = await renderPreview(postId, t)
const archieMlText = await Post.select(
"archieml",
"archieml_update_statistics"
).from(t(Post.postsTable).where({ id: postId }))
return [wpPage, archieMlText]
})
}
)

if (
archieMlText.length === 0 ||
Expand Down Expand Up @@ -275,15 +275,17 @@ adminRouter.get(`/${GetAllExplorersRoute}`, async (req, res) => {

adminRouter.get(`/${GetAllExplorersTagsRoute}`, async (_, res) => {
return res.send({
explorers: await db.getExplorerTags(db.knexInstance()),
explorers: await db.knexReadonlyTransaction((trx) =>
db.getExplorerTags(trx)
),
})
})

adminRouter.get(`/${EXPLORERS_PREVIEW_ROUTE}/:slug`, async (req, res) => {
const slug = slugify(req.params.slug)
const filename = slug + EXPLORER_FILE_SUFFIX

const explorerPage = await db.knexInstance().transaction(async (knex) => {
const explorerPage = await db.knexReadonlyTransaction(async (knex) => {
if (slug === DefaultNewExplorerSlug)
return renderExplorerPage(
new ExplorerProgram(DefaultNewExplorerSlug, ""),
Expand All @@ -307,14 +309,16 @@ adminRouter.get("/datapage-preview/:id", async (req, res) => {
if (!variableMetadata) throw new JsonError("No such variable", 404)

res.send(
await renderDataPageV2(
{
variableId,
variableMetadata,
isPreviewing: true,
useIndicatorGrapherConfigs: true,
},
db.knexInstance()
await db.knexReadonlyTransaction((trx) =>
renderDataPageV2(
{
variableId,
variableMetadata,
isPreviewing: true,
useIndicatorGrapherConfigs: true,
},
trx
)
)
)
})
Expand All @@ -323,11 +327,9 @@ adminRouter.get("/grapher/:slug", async (req, res) => {
const entity = await Chart.getBySlug(req.params.slug)
if (!entity) throw new JsonError("No such chart", 404)

const previewDataPageOrGrapherPage = db
.knexInstance()
.transaction(async (knex) =>
renderPreviewDataPageOrGrapherPage(entity.config, knex)
)
const previewDataPageOrGrapherPage = db.knexReadonlyTransaction(
async (knex) => renderPreviewDataPageOrGrapherPage(entity.config, knex)
)
res.send(previewDataPageOrGrapherPage)
})

Expand Down
Loading
Loading