✨ automatically migrate outdated configs / TAS-623

adminSiteServer/apiRouter.ts

@@ -106,6 +106,7 @@ import {
 import { uuidv7 } from "uuidv7"
 import {
     defaultGrapherConfig,
+    migrateGrapherConfigToLatestVersion,
     getVariableDataRoute,
     getVariableMetadataRoute,
 } from "@ourworldindata/grapher"
@@ -497,7 +498,7 @@ const saveGrapher = async (
         referencedVariablesMightChange = true,
     }: {
         user: DbPlainUser
-        newConfig: GrapherInterface // Note that it is valid for newConfig to be of an older schema version which means that GrapherInterface as a type is slightly misleading
+        newConfig: GrapherInterface
         existingConfig?: GrapherInterface
         // if undefined, keep inheritance as is.
         // if true or false, enable or disable inheritance
@@ -507,6 +508,9 @@ const saveGrapher = async (
         referencedVariablesMightChange?: boolean
     }
 ) => {
+    // Try to migrate the new config to the latest version
+    newConfig = migrateGrapherConfigToLatestVersion(newConfig)
+
     // Slugs need some special logic to ensure public urls remain consistent whenever possible
     async function isSlugUsedInRedirect() {
         const rows = await db.knexRaw<DbPlainChartSlugRedirect>(
@@ -583,22 +587,6 @@ const saveGrapher = async (
         newConfig.version += 1
     else newConfig.version = 1
 
-    // if the schema version is missing, assume it's the latest
-    if (newConfig.$schema === undefined) {
-        newConfig.$schema = defaultGrapherConfig.$schema
-    } else if (
-        newConfig.$schema ===
-        "https://files.ourworldindata.org/schemas/grapher-schema.004.json"
-    ) {
-        // TODO: find a more principled way to do schema upgrades
-
-        // grapher-schema.004 -> grapher-schema.005 removed the obsolete hideLinesOutsideTolerance field
-        const configForMigration = newConfig as any
-        delete configForMigration.hideLinesOutsideTolerance
-        configForMigration.$schema = defaultGrapherConfig.$schema
-        newConfig = configForMigration
-    }
-
     // add the isPublished field if is missing
     if (newConfig.isPublished === undefined) {
         newConfig.isPublished = false
@@ -1038,13 +1026,17 @@ postRouteWithRWTransaction(apiRouter, "/charts", async (req, res, trx) => {
         shouldInherit = req.query.inheritance === "enable"
     }
 
-    const { chartId } = await saveGrapher(trx, {
-        user: res.locals.user,
-        newConfig: req.body,
-        shouldInherit,
-    })
+    try {
+        const { chartId } = await saveGrapher(trx, {
+            user: res.locals.user,
+            newConfig: req.body,
+            shouldInherit,
+        })
 
-    return { success: true, chartId: chartId }
+        return { success: true, chartId: chartId }
+    } catch (err) {
+        return { success: false, error: String(err) }
+    }
 })
 
 postRouteWithRWTransaction(
@@ -1070,19 +1062,29 @@ putRouteWithRWTransaction(
 
         const existingConfig = await expectChartById(trx, req.params.chartId)
 
-        const { chartId, savedPatch } = await saveGrapher(trx, {
-            user: res.locals.user,
-            newConfig: req.body,
-            existingConfig,
-            shouldInherit,
-        })
+        try {
+            const { chartId, savedPatch } = await saveGrapher(trx, {
+                user: res.locals.user,
+                newConfig: req.body,
+                existingConfig,
+                shouldInherit,
+            })
 
-        const logs = await getLogsByChartId(trx, existingConfig.id as number)
-        return {
-            success: true,
-            chartId,
-            savedPatch,
-            newLog: logs[0],
+            const logs = await getLogsByChartId(
+                trx,
+                existingConfig.id as number
+            )
+            return {
+                success: true,
+                chartId,
+                savedPatch,
+                newLog: logs[0],
+            }
+        } catch (err) {
+            return {
+                success: false,
+                error: String(err),
+            }
         }
     }
 )
@@ -1613,13 +1615,23 @@ putRouteWithRWTransaction(
     async (req, res, trx) => {
         const variableId = expectInt(req.params.variableId)
 
+        let validConfig: GrapherInterface
+        try {
+            validConfig = migrateGrapherConfigToLatestVersion(req.body)
+        } catch (err) {
+            return {
+                success: false,
+                error: String(err),
+            }
+        }
+
         const variable = await getGrapherConfigsForVariable(trx, variableId)
         if (!variable) {
             throw new JsonError(`Variable with id ${variableId} not found`, 500)
         }
 
         const { savedPatch, updatedCharts } =
-            await updateGrapherConfigETLOfVariable(trx, variable, req.body)
+            await updateGrapherConfigETLOfVariable(trx, variable, validConfig)
 
         // trigger build if any published chart has been updated
         if (updatedCharts.some((chart) => chart.isPublished)) {
@@ -1708,13 +1720,23 @@ putRouteWithRWTransaction(
     async (req, res, trx) => {
         const variableId = expectInt(req.params.variableId)
 
+        let validConfig: GrapherInterface
+        try {
+            validConfig = migrateGrapherConfigToLatestVersion(req.body)
+        } catch (err) {
+            return {
+                success: false,
+                error: String(err),
+            }
+        }
+
         const variable = await getGrapherConfigsForVariable(trx, variableId)
         if (!variable) {
             throw new JsonError(`Variable with id ${variableId} not found`, 500)
         }
 
         const { savedPatch, updatedCharts } =
-            await updateGrapherConfigAdminOfVariable(trx, variable, req.body)
+            await updateGrapherConfigAdminOfVariable(trx, variable, validConfig)
 
         // trigger build if any published chart has been updated
         if (updatedCharts.some((chart) => chart.isPublished)) {

adminSiteServer/app.test.tsx

@@ -53,7 +53,6 @@ import {
     DatasetsTableName,
     VariablesTableName,
 } from "@ourworldindata/types"
-import { defaultGrapherConfig } from "@ourworldindata/grapher"
 import path from "path"
 import fs from "fs"
 import { omitUndefinedValues } from "@ourworldindata/utils"
@@ -191,6 +190,8 @@ async function makeRequestAgainstAdminApi(
 
 describe("OwidAdminApp", () => {
     const testChartConfig = {
+        $schema:
+            "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
         slug: "test-chart",
         title: "Test chart",
         type: "LineChart",
@@ -252,7 +253,7 @@ describe("OwidAdminApp", () => {
         )
         expect(fullConfig).toHaveProperty(
             "$schema",
-            defaultGrapherConfig.$schema
+            "https://files.ourworldindata.org/schemas/grapher-schema.005.json"
         )
         expect(fullConfig).toHaveProperty("id", chartId) // must match the db id
         expect(fullConfig).toHaveProperty("version", 1) // automatically added
@@ -267,7 +268,8 @@ describe("OwidAdminApp", () => {
             `/charts/${chartId}.patchConfig.json`
         )
         expect(patchConfig).toEqual({
-            $schema: defaultGrapherConfig["$schema"],
+            $schema:
+                "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
             id: chartId,
             version: 1,
             isPublished: false,
@@ -321,12 +323,16 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
         display: '{ "unit": "kg", "shortUnit": "kg" }',
     }
     const testVariableConfigETL = {
+        $schema:
+            "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
         hasMapTab: true,
         note: "Indicator note",
         selectedEntityNames: ["France", "Italy", "Spain"],
         hideRelativeToggle: false,
     }
     const testVariableConfigAdmin = {
+        $schema:
+            "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
         title: "Admin title",
         subtitle: "Admin subtitle",
     }
@@ -337,10 +343,14 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
         id: otherVariableId,
     }
     const otherTestVariableConfig = {
+        $schema:
+            "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
         note: "Other indicator note",
     }
 
     const testChartConfig = {
+        $schema:
+            "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
         slug: "test-chart",
         title: "Test chart",
         type: "Marimekko",
@@ -382,12 +392,11 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
         // for ETL configs, patch and full configs should be the same
         expect(patchConfigETL).toEqual(fullConfigETL)
 
-        // check that $schema and dimensions field were added to the config
+        // check that the dimensions field were added to the config
         const processedTestVariableConfigETL = {
             ...testVariableConfigETL,
 
             // automatically added
-            $schema: defaultGrapherConfig.$schema,
             dimensions: [
                 {
                     property: "y",
@@ -503,7 +512,8 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
             `/charts/${chartId}.patchConfig.json`
         )
         expect(patchConfig).toEqual({
-            $schema: defaultGrapherConfig["$schema"],
+            $schema:
+                "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
             id: chartId,
             version: 1,
             isPublished: false,
@@ -558,7 +568,8 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
             `/charts/${chartId}.patchConfig.json`
         )
         expect(patchConfigAfterDelete).toEqual({
-            $schema: defaultGrapherConfig["$schema"],
+            $schema:
+                "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
             id: chartId,
             version: 1,
             isPublished: false,
@@ -765,4 +776,36 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
         expect(configAfterUpdate).not.toBeNull()
         expect(chartAfterUpdate).toEqual(configAfterUpdate)
     })
+
+    it("should return an error if the schema is missing", async () => {
+        const invalidConfig = {
+            title: "Title",
+            // note that the $schema field is missing
+        }
+        const json = await makeRequestAgainstAdminApi(
+            {
+                method: "PUT",
+                path: `/variables/${variableId}/grapherConfigETL`,
+                body: JSON.stringify(invalidConfig),
+            },
+            { verifySuccess: false }
+        )
+        expect(json.success).toBe(false)
+    })
+
+    it("should return an error if the schema is invalid", async () => {
+        const invalidConfig = {
+            $schema: "invalid", // note that the $schema field is invalid
+            title: "Title",
+        }
+        const json = await makeRequestAgainstAdminApi(
+            {
+                method: "PUT",
+                path: `/variables/${variableId}/grapherConfigETL`,
+                body: JSON.stringify(invalidConfig),
+            },
+            { verifySuccess: false }
+        )
+        expect(json.success).toBe(false)
+    })
 })

db/migration/1726588731621-MigrateOutdatedConfigsToLatestVersion.ts

@@ -0,0 +1,52 @@
+import { migrateGrapherConfigToLatestVersion } from "@ourworldindata/grapher"
+import { GrapherInterface } from "@ourworldindata/types"
+import { MigrationInterface, QueryRunner } from "typeorm"
+
+export class MigrateOutdatedConfigsToLatestVersion1726588731621
+    implements MigrationInterface
+{
+    private migrateConfig(config: Record<string, any>): GrapherInterface {
+        try {
+            return migrateGrapherConfigToLatestVersion(config)
+        } catch {
+            // if the migration function throws, then the $schema field
+            // is either missing or invalid. when that happens, we assume
+            // a schema v1, and try again
+            config.$schema =
+                "https://files.ourworldindata.org/schemas/grapher-schema.001.json"
+            return migrateGrapherConfigToLatestVersion(config)
+        }
+    }
+
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        const outdatedConfigs = await queryRunner.query(
+            `-- sql
+                SELECT id, patch, full
+                FROM chart_configs
+                WHERE
+                    patch ->> '$.$schema' != 'https://files.ourworldindata.org/schemas/grapher-schema.005.json'
+                    OR full ->> '$.$schema' != 'https://files.ourworldindata.org/schemas/grapher-schema.005.json'
+            `
+        )
+
+        for (const { id, patch, full } of outdatedConfigs) {
+            const updatedPatch = this.migrateConfig(JSON.parse(patch))
+            const updatedFull = this.migrateConfig(JSON.parse(full))
+
+            await queryRunner.query(
+                `-- sql
+                    UPDATE chart_configs
+                    SET patch = ?, full = ?
+                    WHERE id = ?
+                `,
+                [JSON.stringify(updatedPatch), JSON.stringify(updatedFull), id]
+            )
+        }
+    }
+
+    public async down(): Promise<void> {
+        throw new Error(
+            "Can't revert migration MigrateOutdatedConfigsToLatestVersion1726588731621"
+        )
+    }
+}

db/model/Variable.ts

@@ -189,11 +189,6 @@ function makeConfigValidForIndicator({
 }): GrapherInterface {
     const updatedConfig = { ...config }
 
-    // if no schema is given, assume it's the latest
-    if (!updatedConfig.$schema) {
-        updatedConfig.$schema = defaultGrapherConfig.$schema
-    }
-
     // validate the given y-dimensions
     const defaultDimension = { property: DimensionProperty.y, variableId }
     const [yDimensions, otherDimensions] = _.partition(