Skip to content

Commit

Permalink
✨ automatically migrate outdated configs / TAS-623 (#3967)
Browse files Browse the repository at this point in the history 
* ✨ automatically migrate outdated configs

* ✅ add db tests

* 📜 (grapher schema) update documentation

* 🔨 move config migration into saveGrapher

* 🔨 use migrate function in db migration
  • Loading branch information
@sophiamersmann
sophiamersmann authored Oct 7, 2024
1 parent f7503b9 commit 8c3b4a4
Show file tree
Hide file tree
Showing 12 changed files with 443 additions and 49 deletions.
96 changes: 59 additions & 37 deletions adminSiteServer/apiRouter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ import {
import { uuidv7 } from "uuidv7"
import {
defaultGrapherConfig,
migrateGrapherConfigToLatestVersion,
getVariableDataRoute,
getVariableMetadataRoute,
} from "@ourworldindata/grapher"
Expand Down Expand Up @@ -501,7 +502,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
Expand All @@ -511,6 +512,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>(
Expand Down Expand Up @@ -587,22 +591,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
Expand Down Expand Up @@ -1042,13 +1030,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(
Expand All @@ -1074,19 +1066,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),
}
}
}
)
Expand Down Expand Up @@ -1617,13 +1619,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)) {
Expand Down Expand Up @@ -1712,13 +1724,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)) {
Expand Down
57 changes: 50 additions & 7 deletions adminSiteServer/app.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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",
}
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
})
})
52 changes: 52 additions & 0 deletions db/migration/1726588731621-MigrateOutdatedConfigsToLatestVersion.ts
View file
Original file line number Diff line number Diff line change
@@ -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"
)
}
}
5 changes: 0 additions & 5 deletions db/model/Variable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading

0 comments on commit 8c3b4a4

Please sign in to comment.