🔨 images need a RW transaction in case they need to update the db 😢 - bubble this up

[danyx23]

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.

[danyx23]

• 🔨 refactor suggested chart revision tool to use knex #3396
• Fix remaining refactor issues #3391
• 🔨 images need a RW transaction in case they need to update the db 😢 - bubble this up #3385 👈
• ✨ unify transaction creation and close db connection in same call for scripts #3360
• 🔨 switch almost all remaining typeorm util function holdouts to knex #3359
• 🔨 set up eslint rule to error on dangling promises + fix 245 violations #3358
• 🚧 remove typeorm annotations from GDocs classes #3299 : 1 other dependent PR (#3372 )
• 🐝 remove obsolete code #3298
• 🔨 migrate charts from typeorm to knex #3293
• 🔨 clean up posts access via knex #3292
• ✨ add transaction types and api route helpers #3271
• 🔨 migrate PostLink to knex #3259
• 💄 reorder parameters of knexRaw and knexRawFirst to have knex in first pos #3258
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danyx23 and the rest of your teammates on Graphite

[ikesau]

Completely brutal 😅

At least, in a way, it's a vindication of all the work you put in - we now can change the transaction signature in a single place and programmatically know all the places it has to be updated.

Just a shame it happened like this 😛

Not sure if it's something that's wrong with my data, but when I try to load the mock site homepage, I get:

TypeError: Cannot read properties of null (reading 'toLocaleDateString')
    at formatDate (grapher/packages/@ourworldindata/utils/src/Util.ts:1306:17)
    at <anonymous> (grapher/db/model/Gdoc/GdocBase.ts:889:36)
    at Array.map (<anonymous>)
    at getMinimalGdocPostsByIds (grapher/db/model/Gdoc/GdocBase.ts:883:17)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at GdocHomepage.loadLinkedDocuments (grapher/db/model/Gdoc/GdocBase.ts:673:13)
    at GdocHomepage.loadState (grapher/db/model/Gdoc/GdocBase.ts:823:9)
    at loadGdocFromGdocBase (grapher/db/model/Gdoc/GdocFactory.ts:326:5)
    at renderFrontPage

[larsyencken]

I'm not sure I've understood what's happening here exactly. Is there a side effect happening? E.g. we bake/preview a page, but the process of previewing can refresh/resolve some image data that was not yet resolved/

[danyx23]

Yeah - the image handling code is written in a way that whenever we are previewing some kind of content via the admin, we collect a list of image resources. If these images are already registered in the images table then it's all good. But if one or more images are not yet registered there then we do a call to the google gdrive api and check if the image exists there. If it does, we copy the image over to r2 and register it in the database.

This works but it means that a lot of code that is semantically read only now sometimes wants to write to the database (plus the latency of that request is much higher in those cases).

I think we'll probably want to redo our image handling and switch to CF images and a once-a-minute cron job or an on-write-event notification handler on gdrive that sync images to CF images. When we do this, then all the places marked in this PR can revert to only opening a read-only transaction

[danyx23]

Merge activity

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