Merge pull request #4320 from owid/fix-image-updating

🐛 fix cloudflare image version management

adminSiteClient/ImagesIndexPage.tsx

@@ -554,10 +554,14 @@ export function ImageIndexPage() {
                     image: DbEnrichedImageWithUserId
                 }>(`/api/images/${id}`, payload, "PUT")
                 if (response.success) {
-                    setImages((prevMap) => ({
-                        ...prevMap,
-                        [id]: response.image,
-                    }))
+                    setImages((prevMap) => {
+                        const nextMap = { ...prevMap }
+                        delete nextMap[id]
+                        return {
+                            ...nextMap,
+                            [response.image.id]: response.image,
+                        }
+                    })
                 }
             },
             postUserImage: async (user, image) => {

adminSiteServer/apiRouter.ts

@@ -212,6 +212,7 @@ import {
     uploadToCloudflare,
     validateImagePayload,
 } from "./imagesHelpers.js"
+import pMap from "p-map"
 
 const apiRouter = new FunctionalRouter()
 
@@ -3152,7 +3153,10 @@ postRouteWithRWTransaction(apiRouter, "/images", async (req, res, trx) => {
     )
 
     const collision = await trx<DbEnrichedImage>("images")
-        .where("hash", "=", hash)
+        .where({
+            hash,
+            replacedBy: null,
+        })
         .first()
 
     if (collision) {
@@ -3201,9 +3205,9 @@ postRouteWithRWTransaction(apiRouter, "/images", async (req, res, trx) => {
 })
 
 /**
- * Similar to the POST route, but for updating an existing image. Deletes the image
- * from Cloudflare and re-uploads it with the new content. The filename will stay the same,
- * but the dimensions and cloudflareId will be updated.
+ * Similar to the POST route, but for updating an existing image.
+ * Creates a new image entry in the database and uploads the new image to Cloudflare.
+ * The old image is marked as replaced by the new image.
  */
 putRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => {
     const { type, content } = validateImagePayload(req.body)
@@ -3212,13 +3216,16 @@ putRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => {
         type
     )
     const collision = await trx<DbEnrichedImage>("images")
-        .where("hash", "=", hash)
+        .where({
+            hash,
+            replacedBy: null,
+        })
         .first()
 
     if (collision) {
         return {
             success: false,
-            error: `An image with this content already exists (filename: ${collision.filename})`,
+            error: `An exact copy of this image already exists (filename: ${collision.filename})`,
         }
     }
 
@@ -3242,23 +3249,34 @@ putRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => {
         )
     }
 
-    await deleteFromCloudflare(originalCloudflareId)
     const newCloudflareId = await uploadToCloudflare(originalFilename, asBlob)
 
     if (!newCloudflareId) {
         throw new JsonError("Failed to upload image", 500)
     }
 
-    await trx<DbEnrichedImage>("images").where("id", "=", id).update({
+    const [newImageId] = await trx<DbEnrichedImage>("images").insert({
+        filename: originalFilename,
         originalWidth: dimensions.width,
         originalHeight: dimensions.height,
-        updatedAt: new Date().getTime(),
         cloudflareId: newCloudflareId,
+        updatedAt: new Date().getTime(),
+        userId: res.locals.user.id,
         hash,
+        version: image.version + 1,
+    })
+
+    await trx<DbEnrichedImage>("images").where("id", "=", id).update({
+        replacedBy: newImageId,
     })
 
     const updated = await db.getCloudflareImage(trx, originalFilename)
 
+    await triggerStaticBuild(
+        res.locals.user,
+        `Updating image "${originalFilename}"`
+    )
+
     return {
         success: true,
         image: updated,
@@ -3296,32 +3314,39 @@ patchRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => {
     }
 })
 
-deleteRouteWithRWTransaction(
-    apiRouter,
-    "/images/:id",
-    async (req, res, trx) => {
-        const { id } = req.params
+deleteRouteWithRWTransaction(apiRouter, "/images/:id", async (req, _, trx) => {
+    const { id } = req.params
 
-        const image = await trx<DbEnrichedImage>("images")
-            .where("id", "=", id)
-            .first()
+    const image = await trx<DbEnrichedImage>("images")
+        .where("id", "=", id)
+        .first()
 
-        if (!image) {
-            throw new JsonError(`No image found for id ${id}`, 404)
-        }
-        if (!image.cloudflareId) {
-            throw new JsonError(`Image does not have a cloudflare ID`, 400)
-        }
+    if (!image) {
+        throw new JsonError(`No image found for id ${id}`, 404)
+    }
+    if (!image.cloudflareId) {
+        throw new JsonError(`Image does not have a cloudflare ID`, 400)
+    }
+
+    const replacementChain = await db.selectReplacementChainForImage(trx, id)
 
-        await deleteFromCloudflare(image.cloudflareId)
+    await pMap(
+        replacementChain,
+        async (image) => {
+            if (image.cloudflareId) {
+                await deleteFromCloudflare(image.cloudflareId)
+            }
+        },
+        { concurrency: 5 }
+    )
 
-        await trx("images").where({ id }).delete()
+    // There's an ON DELETE CASCADE which will delete the replacements
+    await trx("images").where({ id }).delete()
 
-        return {
-            success: true,
-        }
+    return {
+        success: true,
     }
-)
+})
 
 getRouteWithROTransaction(apiRouter, "/images/usage", async (_, __, trx) => {
     const usage = await db.getImageUsage(trx)

db/db.ts

@@ -373,7 +373,8 @@ export const getImageMetadataByFilenames = async (
             cloudflareId
         FROM
             images
-        WHERE filename IN (?)`,
+        WHERE filename IN (?)
+        AND replacedBy IS NULL`,
         [filenames]
     )
     return keyBy(rows, "filename")
@@ -716,6 +717,30 @@ export async function getLinkedIndicatorSlugs({
         .then((slugs) => new Set(slugs))
 }
 
+export async function selectReplacementChainForImage(
+    trx: KnexReadonlyTransaction,
+    id: string
+): Promise<DbEnrichedImage[]> {
+    return knexRaw<DbEnrichedImage>(
+        trx,
+        `-- sql
+        WITH RECURSIVE replacement_chain AS (
+            SELECT i.*
+            FROM images i
+            WHERE id = ?
+        
+            UNION ALL
+        
+            SELECT i.*
+            FROM images i
+            INNER JOIN replacement_chain rc ON i.replacedBy = rc.id
+        )
+        SELECT * FROM replacement_chain
+        `,
+        [id]
+    )
+}
+
 export function getCloudflareImages(
     trx: KnexReadonlyTransaction
 ): Promise<DbEnrichedImage[]> {
@@ -724,7 +749,8 @@ export function getCloudflareImages(
         `-- sql
         SELECT *
         FROM images
-        WHERE cloudflareId IS NOT NULL`
+        WHERE cloudflareId IS NOT NULL
+        AND replacedBy IS NULL`
     )
 }
 
@@ -737,7 +763,8 @@ export function getCloudflareImage(
         `-- sql
         SELECT * 
         FROM images
-        WHERE filename = ?`,
+        WHERE filename = ?
+        AND replacedBy IS NULL`,
         [filename]
     )
 }
@@ -771,6 +798,7 @@ export function getImageUsage(trx: KnexReadonlyTransaction): Promise<
         FROM posts_gdocs p
         JOIN posts_gdocs_x_images pi ON p.id = pi.gdocId 
         JOIN images i ON pi.imageId = i.id
+        WHERE i.replacedBy IS NULL
         GROUP BY i.id`
     ).then((results) =>
         Object.fromEntries(

db/migration/1734369183234-CloudflareImagesReplacedBy.ts

@@ -0,0 +1,50 @@
+import { MigrationInterface, QueryRunner } from "typeorm"
+
+export class CloudflareImagesReplacedBy1734369183234
+    implements MigrationInterface
+{
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(`-- sql
+            ALTER TABLE images
+            DROP KEY filename
+        `)
+
+        await queryRunner.query(`-- sql
+            ALTER TABLE images 
+            ADD COLUMN replacedBy INT NULL,
+            -- Necessary to create a unique constraint with filename, so that we can have multiple versions of the same image,
+            -- but not multiple images with the same filename and version.
+            -- i.e.
+            -- You can upload test.png and *replace* it with test.png, but you can't upload a *new* image named test.png,
+            -- because that would have a version of 0 and conflict with the first test.png that was uploaded
+            -- This was done because MySQL 8 doesn't support partial unique indexes (e.g. "UNIQUE(filename) WHERE replacedBy IS NULL")
+            -- Nor a unique constraint when the column is nullable (e.g. "UNIQUE(filename, replacedBy)" would allow multiple rows with ["test.png", null])
+            ADD COLUMN version INT NOT NULL DEFAULT 0,
+            ADD CONSTRAINT fk_images_replaced_by 
+                FOREIGN KEY (replacedBy) 
+                REFERENCES images(id)
+                ON DELETE CASCADE
+                ON UPDATE CASCADE,
+            ADD CONSTRAINT uk_images_filename_version
+                UNIQUE (filename, version)
+        `)
+    }
+
+    public async down(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(`-- sql
+            ALTER TABLE images
+            DROP FOREIGN KEY fk_images_replaced_by
+        `)
+
+        await queryRunner.query(`-- sql
+            ALTER TABLE images
+            DROP COLUMN replacedBy,
+            DROP COLUMN version
+        `)
+
+        await queryRunner.query(`-- sql
+            ALTER TABLE images
+            ADD UNIQUE KEY filename (filename)
+        `)
+    }
+}

db/model/Gdoc/GdocBase.ts

@@ -734,7 +734,7 @@ export class GdocBase implements OwidGdocBaseInterface {
                 if (!this.imageMetadata[filename]) {
                     errors.push({
                         property: "imageMetadata",
-                        message: `No image named ${filename} found in Drive`,
+                        message: `No image named ${filename} found in the admin`,
                         type: OwidGdocErrorMessageType.Error,
                     })
                 } else if (!this.imageMetadata[filename].defaultAlt) {

db/model/Gdoc/GdocFactory.ts

@@ -648,8 +648,7 @@ export async function addImagesToContentGraph(
     await trx.table(PostsGdocsXImagesTableName).where({ gdocId: id }).delete()
     const filenames = gdoc.filenames
 
-    // Includes fragments so that images in data pages are
-    // synced to S3 and ultimately baked in bakeDriveImages().
+    // Includes fragments so that images in data pages are also tracked
     if (filenames.length && gdoc.published) {
         const images = await getImageMetadataByFilenames(trx, filenames)
         const gdocXImagesToInsert: DbInsertPostGdocXImage[] = []

db/model/Image.ts

@@ -2,9 +2,15 @@ import { DbEnrichedImage, DbRawImage } from "@ourworldindata/types"
 import { parseImageRow } from "@ourworldindata/utils"
 import { KnexReadonlyTransaction } from "../db.js"
 
+/**
+ * Get all images that haven't been replaced
+ **/
 export async function getAllImages(
     knex: KnexReadonlyTransaction
 ): Promise<DbEnrichedImage[]> {
-    const images = await knex.table("images").select<DbRawImage[]>()
+    const images = await knex
+        .table("images")
+        .where("replacedBy", null)
+        .select<DbRawImage[]>()
     return images.map(parseImageRow)
 }

packages/@ourworldindata/types/src/dbTypes/Images.ts

@@ -12,6 +12,17 @@ export interface DbInsertImage {
     cloudflareId?: string | null
     hash?: string | null
     userId?: number | null
+    replacedBy?: number | null
+    /**
+     * Necessary to create a unique constraint with filename, so that we can have multiple versions of the same image,
+     * but not multiple images with the same filename and version.
+     * i.e.
+     * You can upload test.png and *replace* it with test.png, but you can't upload a *new* image named test.png,
+     * because that would have a version of 0 and conflict with the first test.png that was uploaded
+     * This was done because MySQL 8 doesn't support partial unique indexes (e.g. "UNIQUE(filename) WHERE replacedBy IS NULL")
+     * Nor a unique constraint when the column is nullable (e.g. "UNIQUE(filename, replacedBy)" would allow multiple rows with ["test.png", null])
+     */
+    version?: number
 }
 export type DbRawImage = Required<DbInsertImage>