From 43044cf317f4f5cf5b08c578e9a729263dffeabf Mon Sep 17 00:00:00 2001 From: Ike Saunders Date: Mon, 16 Dec 2024 18:14:15 -0500 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=90=9B=20fix=20cloudflare=20image=20v?= =?UTF-8?q?ersion=20management?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteClient/ImagesIndexPage.tsx | 12 ++- adminSiteServer/apiRouter.ts | 80 ++++++++++++------- db/db.ts | 34 +++++++- ...734369183234-CloudflareImagesReplacedBy.ts | 52 ++++++++++++ db/model/Gdoc/GdocBase.ts | 2 +- db/model/Gdoc/GdocFactory.ts | 3 +- db/model/Image.ts | 8 +- .../types/src/dbTypes/Images.ts | 1 + 8 files changed, 153 insertions(+), 39 deletions(-) create mode 100644 db/migration/1734369183234-CloudflareImagesReplacedBy.ts diff --git a/adminSiteClient/ImagesIndexPage.tsx b/adminSiteClient/ImagesIndexPage.tsx index 5b62e77c172..24ba88b42cc 100644 --- a/adminSiteClient/ImagesIndexPage.tsx +++ b/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) => { diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts index 50284ee7c66..953ed3c9386 100644 --- a/adminSiteServer/apiRouter.ts +++ b/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("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,7 +3216,10 @@ putRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => { type ) const collision = await trx("images") - .where("hash", "=", hash) + .where({ + hash, + replacedBy: null, + }) .first() if (collision) { @@ -3242,23 +3249,33 @@ 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("images").where("id", "=", id).update({ + const [newImageId] = await trx("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, }) + await trx("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 +3313,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("images") - .where("id", "=", id) - .first() + const image = await trx("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) diff --git a/db/db.ts b/db/db.ts index cea22490e61..ab9aa2e1ada 100644 --- a/db/db.ts +++ b/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 { + return knexRaw( + 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 { @@ -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( diff --git a/db/migration/1734369183234-CloudflareImagesReplacedBy.ts b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts new file mode 100644 index 00000000000..48fbf603b3f --- /dev/null +++ b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts @@ -0,0 +1,52 @@ +import { MigrationInterface, QueryRunner } from "typeorm" + +export class CloudflareImagesReplacedBy1734369183234 + implements MigrationInterface +{ + public async up(queryRunner: QueryRunner): Promise { + // Drop the existing unique key on filename + await queryRunner.query(`-- sql + ALTER TABLE images + DROP KEY filename + `) + + // Add the replacedBy column + await queryRunner.query(`-- sql + ALTER TABLE images + ADD COLUMN replacedBy INT NULL, + ADD CONSTRAINT fk_images_replaced_by + FOREIGN KEY (replacedBy) + REFERENCES images(id) + ON DELETE CASCADE + ON UPDATE CASCADE + `) + + // Add unique constraint on replacedBy and filename + await queryRunner.query(`-- sql + ALTER TABLE images + ADD CONSTRAINT uk_images_replaced_by_filename + UNIQUE (replacedBy, filename) + `) + } + + public async down(queryRunner: QueryRunner): Promise { + // Remove the combined unique constraint + await queryRunner.query(`-- sql + ALTER TABLE images + DROP KEY uk_images_replaced_by_filename + `) + + // Remove the foreign key constraint and column + await queryRunner.query(`-- sql + ALTER TABLE images + DROP FOREIGN KEY fk_images_replaced_by, + DROP COLUMN replacedBy + `) + + // Restore the original unique key on filename + await queryRunner.query(`-- sql + ALTER TABLE images + ADD UNIQUE KEY filename (filename) + `) + } +} diff --git a/db/model/Gdoc/GdocBase.ts b/db/model/Gdoc/GdocBase.ts index 5e7a0fb7a33..e1e7218ef3f 100644 --- a/db/model/Gdoc/GdocBase.ts +++ b/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) { diff --git a/db/model/Gdoc/GdocFactory.ts b/db/model/Gdoc/GdocFactory.ts index b9ea5fdda81..0a192d45dbe 100644 --- a/db/model/Gdoc/GdocFactory.ts +++ b/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[] = [] diff --git a/db/model/Image.ts b/db/model/Image.ts index de5b3edd729..e4e241d35ab 100644 --- a/db/model/Image.ts +++ b/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 { - const images = await knex.table("images").select() + const images = await knex + .table("images") + .where("replacedBy", null) + .select() return images.map(parseImageRow) } diff --git a/packages/@ourworldindata/types/src/dbTypes/Images.ts b/packages/@ourworldindata/types/src/dbTypes/Images.ts index a17431a5a44..a47f762f634 100644 --- a/packages/@ourworldindata/types/src/dbTypes/Images.ts +++ b/packages/@ourworldindata/types/src/dbTypes/Images.ts @@ -12,6 +12,7 @@ export interface DbInsertImage { cloudflareId?: string | null hash?: string | null userId?: number | null + replacedBy?: number | null } export type DbRawImage = Required From d69164794b573ef812f221b060f5ef0d9e871667 Mon Sep 17 00:00:00 2001 From: Ike Saunders Date: Tue, 17 Dec 2024 15:58:20 -0500 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=90=9B=20fix=20revert=20migration=20f?= =?UTF-8?q?or=20replacedBy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../1734369183234-CloudflareImagesReplacedBy.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/db/migration/1734369183234-CloudflareImagesReplacedBy.ts b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts index 48fbf603b3f..852fcfa514d 100644 --- a/db/migration/1734369183234-CloudflareImagesReplacedBy.ts +++ b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts @@ -30,16 +30,21 @@ export class CloudflareImagesReplacedBy1734369183234 } public async down(queryRunner: QueryRunner): Promise { - // Remove the combined unique constraint + // First remove the foreign key constraint + await queryRunner.query(`-- sql + ALTER TABLE images + DROP FOREIGN KEY fk_images_replaced_by + `) + + // Then remove the combined unique constraint await queryRunner.query(`-- sql ALTER TABLE images DROP KEY uk_images_replaced_by_filename `) - // Remove the foreign key constraint and column + // Remove the column await queryRunner.query(`-- sql ALTER TABLE images - DROP FOREIGN KEY fk_images_replaced_by, DROP COLUMN replacedBy `) From b65842b2ce6ab7c9cdb014bd1d6dcd9f05734944 Mon Sep 17 00:00:00 2001 From: Ike Saunders Date: Tue, 17 Dec 2024 18:54:17 -0500 Subject: [PATCH 3/4] =?UTF-8?q?=E2=9C=A8=20add=20version=20to=20images=20t?= =?UTF-8?q?able?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteServer/apiRouter.ts | 3 ++- ...734369183234-CloudflareImagesReplacedBy.ts | 26 +++++-------------- .../types/src/dbTypes/Images.ts | 7 +++++ 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts index 953ed3c9386..945d6b66700 100644 --- a/adminSiteServer/apiRouter.ts +++ b/adminSiteServer/apiRouter.ts @@ -3225,7 +3225,7 @@ putRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => { 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})`, } } @@ -3263,6 +3263,7 @@ putRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => { updatedAt: new Date().getTime(), userId: res.locals.user.id, hash, + version: image.version + 1, }) await trx("images").where("id", "=", id).update({ diff --git a/db/migration/1734369183234-CloudflareImagesReplacedBy.ts b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts index 852fcfa514d..13f46c7e867 100644 --- a/db/migration/1734369183234-CloudflareImagesReplacedBy.ts +++ b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts @@ -4,51 +4,37 @@ export class CloudflareImagesReplacedBy1734369183234 implements MigrationInterface { public async up(queryRunner: QueryRunner): Promise { - // Drop the existing unique key on filename await queryRunner.query(`-- sql ALTER TABLE images DROP KEY filename `) - // Add the replacedBy column await queryRunner.query(`-- sql ALTER TABLE images ADD COLUMN replacedBy INT 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 unique constraint on replacedBy and filename - await queryRunner.query(`-- sql - ALTER TABLE images - ADD CONSTRAINT uk_images_replaced_by_filename - UNIQUE (replacedBy, filename) + ON UPDATE CASCADE, + ADD CONSTRAINT uk_images_filename_version + UNIQUE (filename, version) `) } public async down(queryRunner: QueryRunner): Promise { - // First remove the foreign key constraint await queryRunner.query(`-- sql ALTER TABLE images DROP FOREIGN KEY fk_images_replaced_by `) - // Then remove the combined unique constraint - await queryRunner.query(`-- sql - ALTER TABLE images - DROP KEY uk_images_replaced_by_filename - `) - - // Remove the column await queryRunner.query(`-- sql ALTER TABLE images - DROP COLUMN replacedBy + DROP COLUMN replacedBy, + DROP COLUMN version `) - // Restore the original unique key on filename await queryRunner.query(`-- sql ALTER TABLE images ADD UNIQUE KEY filename (filename) diff --git a/packages/@ourworldindata/types/src/dbTypes/Images.ts b/packages/@ourworldindata/types/src/dbTypes/Images.ts index a47f762f634..c83961c6591 100644 --- a/packages/@ourworldindata/types/src/dbTypes/Images.ts +++ b/packages/@ourworldindata/types/src/dbTypes/Images.ts @@ -13,6 +13,13 @@ export interface DbInsertImage { 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 + */ + version?: number } export type DbRawImage = Required From 7c227c31dccbe872a856d85961330f3b57e53a49 Mon Sep 17 00:00:00 2001 From: Ike Saunders Date: Wed, 18 Dec 2024 11:50:55 -0500 Subject: [PATCH 4/4] =?UTF-8?q?=E2=9C=A8=20improve=20version=20explanation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- db/migration/1734369183234-CloudflareImagesReplacedBy.ts | 7 +++++++ packages/@ourworldindata/types/src/dbTypes/Images.ts | 5 ++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/db/migration/1734369183234-CloudflareImagesReplacedBy.ts b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts index 13f46c7e867..1b5aaf717ac 100644 --- a/db/migration/1734369183234-CloudflareImagesReplacedBy.ts +++ b/db/migration/1734369183234-CloudflareImagesReplacedBy.ts @@ -12,6 +12,13 @@ export class CloudflareImagesReplacedBy1734369183234 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) diff --git a/packages/@ourworldindata/types/src/dbTypes/Images.ts b/packages/@ourworldindata/types/src/dbTypes/Images.ts index c83961c6591..318908573c6 100644 --- a/packages/@ourworldindata/types/src/dbTypes/Images.ts +++ b/packages/@ourworldindata/types/src/dbTypes/Images.ts @@ -16,8 +16,11 @@ export interface DbInsertImage { /** * 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, + * 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 }