🔨 images need a RW transaction in case they need to update the db 😢 -…

… bubble this up (#3385) This is not a nice PR - the images code was wrongly tagged as needing only a read-only transaction but it can actually update the DB when a sync occurs. This then bubbled up to a lot of places unfortunately.
owid · Mar 26, 2024 · 2537c64 · 2537c64
2 parents 8501105 + 90dc009
commit 2537c64
Show file tree

Hide file tree

Showing 23 changed files with 198 additions and 97 deletions.
diff --git a/adminSiteServer/adminRouter.tsx b/adminSiteServer/adminRouter.tsx
@@ -45,7 +45,10 @@ import {
 import { getChartConfigBySlug } from "../db/model/Chart.js"
 import { getVariableMetadata } from "../db/model/Variable.js"
 import { DbPlainDatasetFile, DbPlainDataset } from "@ourworldindata/types"
-import { getPlainRouteWithROTransaction } from "./plainRouterHelpers.js"
+import {
+    getPlainRouteNonIdempotentWithRWTransaction,
+    getPlainRouteWithROTransaction,
+} from "./plainRouterHelpers.js"
 
 // Used for rate-limiting important endpoints (login, register) to prevent brute force attacks
 const limiterMiddleware = (
@@ -314,8 +317,8 @@ getPlainRouteWithROTransaction(
         return res.send(explorerPage)
     }
 )
-
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     adminRouter,
     "/datapage-preview/:id",
     async (req, res, trx) => {
@@ -336,8 +339,8 @@ getPlainRouteWithROTransaction(
         )
     }
 )
-
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     adminRouter,
     "/grapher/:slug",
     async (req, res, trx) => {

diff --git a/adminSiteServer/app.tsx b/adminSiteServer/app.tsx
@@ -126,7 +126,8 @@ export class OwidAdminApp {
         // Public preview of a Gdoc document
         app.get("/gdocs/:id/preview", async (req, res) => {
             try {
-                await db.knexReadonlyTransaction(async (knex) => {
+                // TODO: this transaction is only RW because somewhere inside it we fetch images
+                await db.knexReadWriteTransaction(async (knex) => {
                     const gdoc = await getAndLoadGdocById(
                         knex,
                         req.params.id,

diff --git a/adminSiteServer/mockSiteRouter.tsx b/adminSiteServer/mockSiteRouter.tsx
@@ -57,7 +57,10 @@ import { GdocPost } from "../db/model/Gdoc/GdocPost.js"
 import { GdocDataInsight } from "../db/model/Gdoc/GdocDataInsight.js"
 import * as db from "../db/db.js"
 import { calculateDataInsightIndexPageCount } from "../db/model/Gdoc/gdocUtils.js"
-import { getPlainRouteWithROTransaction } from "./plainRouterHelpers.js"
+import {
+    getPlainRouteNonIdempotentWithRWTransaction,
+    getPlainRouteWithROTransaction,
+} from "./plainRouterHelpers.js"
 import { DEFAULT_LOCAL_BAKE_DIR } from "../site/SiteConstants.js"
 
 require("express-async-errors")
@@ -68,7 +71,8 @@ const mockSiteRouter = Router()
 mockSiteRouter.use(express.urlencoded({ extended: true }))
 mockSiteRouter.use(express.json())
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/sitemap.xml",
     async (req, res, trx) => {
@@ -78,7 +82,8 @@ getPlainRouteWithROTransaction(
     }
 )
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/atom.xml",
     async (req, res, trx) => {
@@ -88,7 +93,8 @@ getPlainRouteWithROTransaction(
     }
 )
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/atom-no-topic-pages.xml",
     async (req, res, trx) => {
@@ -188,7 +194,8 @@ mockSiteRouter.get("/collection/custom", async (_, res) => {
     return res.send(await renderDynamicCollectionPage())
 })
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/grapher/:slug",
     async (req, res, trx) => {
@@ -204,12 +211,18 @@ getPlainRouteWithROTransaction(
     }
 )
 
-getPlainRouteWithROTransaction(mockSiteRouter, "/", async (req, res, trx) => {
-    const frontPage = await renderFrontPage(trx)
-    res.send(frontPage)
-})
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
+    mockSiteRouter,
+    "/",
+    async (req, res, trx) => {
+        const frontPage = await renderFrontPage(trx)
+        res.send(frontPage)
+    }
+)
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/donate",
     async (req, res, trx) => res.send(await renderDonatePage(trx))
@@ -219,7 +232,8 @@ mockSiteRouter.get("/thank-you", async (req, res) =>
     res.send(await renderThankYouPage())
 )
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/data-insights/:pageNumberOrSlug?",
     async (req, res, trx) => {
@@ -278,7 +292,8 @@ getPlainRouteWithROTransaction(
     }
 )
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/datapage-preview/:id",
     async (req, res, trx) => {
@@ -318,7 +333,8 @@ mockSiteRouter.get("/search", async (req, res) =>
     res.send(await renderSearchPage())
 )
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/latest",
     async (req, res, trx) => {
@@ -327,7 +343,8 @@ getPlainRouteWithROTransaction(
     }
 )
 
-getPlainRouteWithROTransaction(
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
     mockSiteRouter,
     "/latest/page/:pageno",
     async (req, res, trx) => {
@@ -444,23 +461,28 @@ getPlainRouteWithROTransaction(
     }
 )
 
-getPlainRouteWithROTransaction(mockSiteRouter, "/*", async (req, res, trx) => {
-    const slug = req.path.replace(/^\//, "")
+// TODO: this transaction is only RW because somewhere inside it we fetch images
+getPlainRouteNonIdempotentWithRWTransaction(
+    mockSiteRouter,
+    "/*",
+    async (req, res, trx) => {
+        const slug = req.path.replace(/^\//, "")
 
-    try {
-        const page = await renderGdocsPageBySlug(trx, slug)
-        res.send(page)
-    } catch (e) {
-        console.error(e)
-    }
+        try {
+            const page = await renderGdocsPageBySlug(trx, slug)
+            res.send(page)
+        } catch (e) {
+            console.error(e)
+        }
 
-    try {
-        const page = await renderPageBySlug(slug, trx)
-        res.send(page)
-    } catch (e) {
-        console.error(e)
-        res.status(404).send(await renderNotFoundPage())
+        try {
+            const page = await renderPageBySlug(slug, trx)
+            res.send(page)
+        } catch (e) {
+            console.error(e)
+            res.status(404).send(await renderNotFoundPage())
+        }
     }
-})
+)
 
 export { mockSiteRouter }
diff --git a/baker/DatapageHelpers.ts b/baker/DatapageHelpers.ts
@@ -16,7 +16,7 @@ import {
 } from "../db/model/Gdoc/GdocFactory.js"
 import { OwidGoogleAuth } from "../db/OwidGoogleAuth.js"
 import { GrapherInterface, OwidGdocBaseInterface } from "@ourworldindata/types"
-import { KnexReadonlyTransaction } from "../db/db.js"
+import { KnexReadWriteTransaction } from "../db/db.js"
 
 export const getDatapageDataV2 = async (
     variableMetadata: OwidVariableWithSource,
@@ -78,7 +78,8 @@ export const getDatapageDataV2 = async (
  * see https://github.com/owid/owid-grapher/issues/2121#issue-1676097164
  */
 export const getDatapageGdoc = async (
-    knex: KnexReadonlyTransaction,
+    // TODO: this transaction is only RW because somewhere inside it we fetch images
+    knex: KnexReadWriteTransaction,
     googleDocEditLinkOrId: string,
     isPreviewing: boolean
 ): Promise<OwidGdocBaseInterface | null> => {

diff --git a/baker/DeployUtils.ts b/baker/DeployUtils.ts
@@ -11,7 +11,7 @@ import {
 import { SiteBaker } from "../baker/SiteBaker.js"
 import { WebClient } from "@slack/web-api"
 import { DeployChange, DeployMetadata } from "@ourworldindata/utils"
-import { KnexReadonlyTransaction } from "../db/db.js"
+import { KnexReadWriteTransaction } from "../db/db.js"
 
 const deployQueueServer = new DeployQueueServer()
 
@@ -33,9 +33,11 @@ export const defaultCommitMessage = async (): Promise<string> => {
 /**
  * Initiate a deploy, without any checks. Throws error on failure.
  */
+
+// TODO: this transaction is only RW because somewhere inside it we fetch images
 const triggerBakeAndDeploy = async (
     deployMetadata: DeployMetadata,
-    knex: KnexReadonlyTransaction,
+    knex: KnexReadWriteTransaction,
     lightningQueue?: DeployChange[]
 ) => {
     // deploy to Buildkite if we're on master and BUILDKITE_API_ACCESS_TOKEN is set
@@ -160,7 +162,8 @@ let deploying = false
  * If there are no changes in the queue, a deploy won't be initiated.
  */
 export const deployIfQueueIsNotEmpty = async (
-    knex: KnexReadonlyTransaction
+    // TODO: this transaction is only RW because somewhere inside it we fetch images
+    knex: KnexReadWriteTransaction
 ) => {
     if (deploying) return
     deploying = true

diff --git a/baker/GrapherBaker.tsx b/baker/GrapherBaker.tsx
@@ -66,7 +66,7 @@ import { getGdocBaseObjectBySlug } from "../db/model/Gdoc/GdocFactory.js"
 const renderDatapageIfApplicable = async (
     grapher: GrapherInterface,
     isPreviewing: boolean,
-    knex: db.KnexReadonlyTransaction,
+    knex: db.KnexReadWriteTransaction,
     imageMetadataDictionary?: Record<string, Image>
 ) => {
     const variable = await getVariableOfDatapageIfApplicable(grapher)
@@ -90,9 +90,11 @@ const renderDatapageIfApplicable = async (
  *
  * Render a datapage if available, otherwise render a grapher page.
  */
+
+// TODO: this transaction is only RW because somewhere inside it we fetch images
 export const renderDataPageOrGrapherPage = async (
     grapher: GrapherInterface,
-    knex: db.KnexReadonlyTransaction,
+    knex: db.KnexReadWriteTransaction,
     imageMetadataDictionary?: Record<string, Image>
 ): Promise<string> => {
     const datapage = await renderDatapageIfApplicable(
@@ -133,7 +135,9 @@ export async function renderDataPageV2(
         pageGrapher?: GrapherInterface
         imageMetadataDictionary?: Record<string, ImageMetadata>
     },
-    knex: db.KnexReadonlyTransaction
+
+    // TODO: this transaction is only RW because somewhere inside it we fetch images
+    knex: db.KnexReadWriteTransaction
 ) {
     const grapherConfigForVariable = await getMergedGrapherConfigForVariable(
         variableId,
@@ -317,7 +321,9 @@ export async function renderDataPageV2(
  */
 export const renderPreviewDataPageOrGrapherPage = async (
     grapher: GrapherInterface,
-    knex: db.KnexReadonlyTransaction
+
+    // TODO: this transaction is only RW because somewhere inside it we fetch images
+    knex: db.KnexReadWriteTransaction
 ) => {
     const datapage = await renderDatapageIfApplicable(grapher, true, knex)
     if (datapage) return datapage
@@ -370,7 +376,7 @@ const bakeGrapherPageAndVariablesPngAndSVGIfChanged = async (
     bakedSiteDir: string,
     imageMetadataDictionary: Record<string, Image>,
     grapher: GrapherInterface,
-    knex: db.KnexReadonlyTransaction
+    knex: db.KnexReadWriteTransaction
 ) => {
     const htmlPath = `${bakedSiteDir}/grapher/${grapher.slug}.html`
     const isSameVersion = await chartIsSameVersion(htmlPath, grapher.version)
@@ -453,7 +459,9 @@ export interface BakeSingleGrapherChartArguments {
 
 export const bakeSingleGrapherChart = async (
     args: BakeSingleGrapherChartArguments,
-    knex: db.KnexReadonlyTransaction
+
+    // TODO: this transaction is only RW because somewhere inside it we fetch images
+    knex: db.KnexReadWriteTransaction
 ) => {
     const grapher: GrapherInterface = JSON.parse(args.config)
     grapher.id = args.id
@@ -475,7 +483,8 @@ export const bakeSingleGrapherChart = async (
 }
 
 export const bakeAllChangedGrapherPagesVariablesPngSvgAndDeleteRemovedGraphers =
-    async (bakedSiteDir: string, knex: db.KnexReadonlyTransaction) => {
+    // TODO: this transaction is only RW because somewhere inside it we fetch images
+    async (bakedSiteDir: string, knex: db.KnexReadWriteTransaction) => {
         const chartsToBake: { id: number; config: string; slug: string }[] =
             await knexRaw(
                 knex,