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..945d6b66700 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,13 +3216,16 @@ putRouteWithRWTransaction(apiRouter, "/images/:id", async (req, res, trx) => { type ) const collision = await trx("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("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, + version: image.version + 1, + }) + + 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 +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("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..1b5aaf717ac --- /dev/null +++ b/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 { + 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 { + 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) + `) + } +} 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..318908573c6 100644 --- a/packages/@ourworldindata/types/src/dbTypes/Images.ts +++ b/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