From fc61042b886ab6719e95d51ac8bf88d23aa67516 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ra=C4=8D=C3=A1k?= Date: Fri, 1 Nov 2024 15:59:23 +0100 Subject: [PATCH] Update grapher configs in R2 when variable config changes (#4096) * Fix missing updates of configs in R2 after we update the configs in the DB on variable grapher config upsert/deletion in the API * Update mdim view grapher configs on variable grapher config upsert/deletion * Trigger baking if a published mdim config is created/updated * Update API tests to include mdims --- adminSiteServer/apiRouter.ts | 89 ++++++++++++++++----- adminSiteServer/app.test.tsx | 127 +++++++++++++++++++++++++++++ db/model/MultiDimDataPage.ts | 11 +++ db/model/Variable.ts | 151 ++++++++++++++++++++++++++++++----- db/tests/testHelpers.ts | 4 + 5 files changed, 344 insertions(+), 38 deletions(-) diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts index da81461138c..231a2314cda 100644 --- a/adminSiteServer/apiRouter.ts +++ b/adminSiteServer/apiRouter.ts @@ -8,7 +8,9 @@ import { BAKED_BASE_URL, ADMIN_BASE_URL, DATA_API_URL, + FEATURE_FLAGS, } from "../settings/serverSettings.js" +import { FeatureFlagFeature } from "../settings/clientSettings.js" import { expectInt, isValidSlug } from "../serverUtils/serverUtil.js" import { OldChartFieldList, @@ -34,6 +36,7 @@ import { updateGrapherConfigAdminOfVariable, updateGrapherConfigETLOfVariable, updateAllChartsThatInheritFromIndicator, + updateAllMultiDimViewsThatInheritFromIndicator, getAllChartsForIndicator, } from "../db/model/Variable.js" import { updateExistingFullConfig } from "../db/model/ChartConfigs.js" @@ -102,6 +105,7 @@ import { parseChartConfig, MultiDimDataPageConfigRaw, R2GrapherConfigDirectory, + ChartConfigsTableName, } from "@ourworldindata/types" import { uuidv7 } from "uuidv7" import { @@ -182,6 +186,7 @@ import { } from "./chartConfigR2Helpers.js" import { fetchImagesFromDriveAndSyncToS3 } from "../db/model/Image.js" import { createMultiDimConfig } from "./multiDim.js" +import { isMultiDimDataPagePublished } from "../db/model/MultiDimDataPage.js" const apiRouter = new FunctionalRouter() @@ -722,6 +727,23 @@ const saveGrapher = async ( } } +async function updateGrapherConfigsInR2( + knex: db.KnexReadonlyTransaction, + updatedCharts: { chartConfigId: string; isPublished: boolean }[], + updatedMultiDimViews: { chartConfigId: string; isPublished: boolean }[] +) { + const idsToUpdate = [ + ...updatedCharts.filter(({ isPublished }) => isPublished), + ...updatedMultiDimViews, + ].map(({ chartConfigId }) => chartConfigId) + const builder = knex(ChartConfigsTableName) + .select("id", "full", "fullMd5") + .whereIn("id", idsToUpdate) + for await (const { id, full, fullMd5 } of builder.stream()) { + await saveGrapherConfigToR2ByUUID(id, full, fullMd5) + } +} + getRouteWithROTransaction(apiRouter, "/charts.json", async (req, res, trx) => { const limit = parseIntOrUndefined(req.query.limit as string) ?? 10000 const charts = await db.knexRaw( @@ -1155,6 +1177,15 @@ putRouteWithRWTransaction( } const rawConfig = req.body as MultiDimDataPageConfigRaw const id = await createMultiDimConfig(trx, slug, rawConfig) + if ( + FEATURE_FLAGS.has(FeatureFlagFeature.MultiDimDataPage) && + (await isMultiDimDataPagePublished(trx, id)) + ) { + await triggerStaticBuild( + res.locals.user, + `Publishing multidimensional chart ${slug}` + ) + } return { success: true, id } } ) @@ -1647,11 +1678,13 @@ putRouteWithRWTransaction( throw new JsonError(`Variable with id ${variableId} not found`, 500) } - const { savedPatch, updatedCharts } = + const { savedPatch, updatedCharts, updatedMultiDimViews } = await updateGrapherConfigETLOfVariable(trx, variable, validConfig) - // trigger build if any published chart has been updated - if (updatedCharts.some((chart) => chart.isPublished)) { + await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews) + const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews] + + if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) { await triggerStaticBuild( res.locals.user, `Updating ETL config for variable ${variableId}` @@ -1708,18 +1741,25 @@ deleteRouteWithRWTransaction( }) } - // update all charts that inherit from the indicator + const updates = { + patchConfigAdmin: variable.admin?.patchConfig, + updatedAt: now, + } const updatedCharts = await updateAllChartsThatInheritFromIndicator( trx, variableId, - { - patchConfigAdmin: variable.admin?.patchConfig, - updatedAt: now, - } + updates ) + const updatedMultiDimViews = + await updateAllMultiDimViewsThatInheritFromIndicator( + trx, + variableId, + updates + ) + await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews) + const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews] - // trigger build if any published chart has been updated - if (updatedCharts.some((chart) => chart.isPublished)) { + if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) { await triggerStaticBuild( res.locals.user, `Updating ETL config for variable ${variableId}` @@ -1752,11 +1792,13 @@ putRouteWithRWTransaction( throw new JsonError(`Variable with id ${variableId} not found`, 500) } - const { savedPatch, updatedCharts } = + const { savedPatch, updatedCharts, updatedMultiDimViews } = await updateGrapherConfigAdminOfVariable(trx, variable, validConfig) - // trigger build if any published chart has been updated - if (updatedCharts.some((chart) => chart.isPublished)) { + await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews) + const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews] + + if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) { await triggerStaticBuild( res.locals.user, `Updating admin-authored config for variable ${variableId}` @@ -1804,18 +1846,25 @@ deleteRouteWithRWTransaction( [variable.admin.configId] ) - // update all charts that inherit from the indicator + const updates = { + patchConfigETL: variable.etl?.patchConfig, + updatedAt: now, + } const updatedCharts = await updateAllChartsThatInheritFromIndicator( trx, variableId, - { - patchConfigETL: variable.etl?.patchConfig, - updatedAt: now, - } + updates ) + const updatedMultiDimViews = + await updateAllMultiDimViewsThatInheritFromIndicator( + trx, + variableId, + updates + ) + await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews) + const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews] - // trigger build if any published chart has been updated - if (updatedCharts.some((chart) => chart.isPublished)) { + if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) { await triggerStaticBuild( res.locals.user, `Updating admin-authored config for variable ${variableId}` diff --git a/adminSiteServer/app.test.tsx b/adminSiteServer/app.test.tsx index 26b3cdca948..4062e43d3c8 100644 --- a/adminSiteServer/app.test.tsx +++ b/adminSiteServer/app.test.tsx @@ -51,6 +51,8 @@ import { ChartConfigsTableName, ChartsTableName, DatasetsTableName, + MultiDimDataPagesTableName, + MultiDimXChartConfigsTableName, VariablesTableName, } from "@ourworldindata/types" import path from "path" @@ -363,6 +365,67 @@ describe("OwidAdminApp: indicator-level chart configs", () => { }, ], } + const testMultiDimConfig = { + grapherConfigSchema: + "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + title: { + title: "Energy use", + titleVariant: "by energy source", + }, + views: [ + { + config: { title: "Total energy use" }, + dimensions: { + source: "all", + metric: "total", + }, + indicators: { + y: variableId, + }, + }, + { + dimensions: { + metric: "per_capita", + source: "all", + }, + indicators: { + y: otherVariableId, + }, + }, + ], + dimensions: [ + { + name: "Energy source", + slug: "source", + choices: [ + { + name: "All sources", + slug: "all", + group: "Aggregates", + description: "Total energy use", + }, + ], + }, + { + name: "Metric", + slug: "metric", + choices: [ + { + name: "Total consumption", + slug: "total", + description: + "The amount of energy consumed nationally per year", + }, + { + name: "Consumption per capita", + slug: "per_capita", + description: + "The average amount of energy each person consumes per year", + }, + ], + }, + ], + } beforeEach(async () => { await testKnexInstance!(DatasetsTableName).insert([dummyDataset]) @@ -432,6 +495,70 @@ describe("OwidAdminApp: indicator-level chart configs", () => { ...testVariableConfigAdmin, }) + // create mdim config that uses both of the variables + await makeRequestAgainstAdminApi({ + method: "PUT", + path: "/multi-dim/energy", + body: JSON.stringify(testMultiDimConfig), + }) + const mdim = await testKnexInstance!(MultiDimDataPagesTableName).first() + expect(mdim.slug).toBe("energy") + const savedMdimConfig = JSON.parse(mdim.config) + // variableId should be normalized to an array + expect(savedMdimConfig.views[0].indicators.y).toBeInstanceOf(Array) + + const [mdxcc1, mdxcc2] = await testKnexInstance!( + MultiDimXChartConfigsTableName + ) + expect(mdxcc1.multiDimId).toBe(mdim.id) + expect(mdxcc1.viewId).toBe("total__all") + expect(mdxcc1.variableId).toBe(variableId) + expect(mdxcc2.multiDimId).toBe(mdim.id) + expect(mdxcc2.viewId).toBe("per_capita__all") + expect(mdxcc2.variableId).toBe(otherVariableId) + + // view config should override the variable config + const expectedMergedViewConfig = { + ...mergedGrapherConfig, + title: "Total energy use", + selectedEntityNames: [], // mdims define their own default entities + } + const fullViewConfig1 = await testKnexInstance!(ChartConfigsTableName) + .where("id", mdxcc1.chartConfigId) + .first() + expect(JSON.parse(fullViewConfig1.full)).toEqual( + expectedMergedViewConfig + ) + + // update the admin-authored config for the variable + await makeRequestAgainstAdminApi({ + method: "PUT", + path: `/variables/${variableId}/grapherConfigAdmin`, + body: JSON.stringify({ + ...testVariableConfigAdmin, + subtitle: "Newly updated subtitle", + }), + }) + const expectedMergedViewConfigUpdated = { + ...expectedMergedViewConfig, + subtitle: "Newly updated subtitle", + } + const fullViewConfig1Updated = await testKnexInstance!( + ChartConfigsTableName + ) + .where("id", mdxcc1.chartConfigId) + .first() + expect(JSON.parse(fullViewConfig1Updated.full)).toEqual( + expectedMergedViewConfigUpdated + ) + + // clean-up the mdim tables + await testKnexInstance!(MultiDimXChartConfigsTableName).truncate() + await testKnexInstance!(MultiDimDataPagesTableName).delete() + await testKnexInstance!(ChartConfigsTableName) + .whereIn("id", [mdxcc1.chartConfigId, mdxcc2.chartConfigId]) + .delete() + // delete the admin-authored grapher config we just added // and verify that the merged config is now the same as the ETL config await makeRequestAgainstAdminApi({ diff --git a/db/model/MultiDimDataPage.ts b/db/model/MultiDimDataPage.ts index 31ff08a6c42..14254ae2fd4 100644 --- a/db/model/MultiDimDataPage.ts +++ b/db/model/MultiDimDataPage.ts @@ -23,6 +23,17 @@ export async function upsertMultiDimDataPage( return result[0] } +export async function isMultiDimDataPagePublished( + knex: KnexReadonlyTransaction, + id: number +): Promise { + const result = await knex(MultiDimDataPagesTableName) + .select(knex.raw("1")) + .where({ id, published: true }) + .first() + return Boolean(result) +} + const createOnlyPublishedFilter = ( onlyPublished: boolean ): Record => (onlyPublished ? { published: true } : {}) diff --git a/db/model/Variable.ts b/db/model/Variable.ts index 6b6afee2aee..14a3406f66a 100644 --- a/db/model/Variable.ts +++ b/db/model/Variable.ts @@ -33,6 +33,7 @@ import { DbEnrichedChartConfig, DbEnrichedVariable, DbPlainChart, + DbPlainMultiDimXChartConfig, } from "@ourworldindata/types" import { knexRaw, knexRawFirst } from "../db.js" import { @@ -218,17 +219,21 @@ async function findAllChartsThatInheritFromIndicator( trx: db.KnexReadonlyTransaction, variableId: number ): Promise< - { chartId: number; patchConfig: GrapherInterface; isPublished: boolean }[] + { + chartConfigId: string + patchConfig: GrapherInterface + isPublished: boolean + }[] > { const charts = await db.knexRaw<{ - chartId: DbPlainChart["id"] + chartConfigId: DbRawChartConfig["id"] patchConfig: DbRawChartConfig["patch"] isPublished: boolean }>( trx, `-- sql SELECT - c.id as chartId, + cc.id as chartConfigId, cc.patch as patchConfig, cc.full ->> "$.isPublished" as isPublished FROM charts c @@ -258,7 +263,7 @@ export async function updateAllChartsThatInheritFromIndicator( patchConfigETL?: GrapherInterface patchConfigAdmin?: GrapherInterface } -): Promise<{ chartId: number; isPublished: boolean }[]> { +): Promise<{ chartConfigId: string; isPublished: boolean }[]> { const inheritingCharts = await findAllChartsThatInheritFromIndicator( trx, variableId @@ -280,9 +285,14 @@ export async function updateAllChartsThatInheritFromIndicator( cc.full = ?, cc.updatedAt = ?, c.updatedAt = ? - WHERE c.id = ? + WHERE cc.id = ? `, - [JSON.stringify(fullConfig), updatedAt, updatedAt, chart.chartId] + [ + JSON.stringify(fullConfig), + updatedAt, + updatedAt, + chart.chartConfigId, + ] ) } @@ -290,13 +300,95 @@ export async function updateAllChartsThatInheritFromIndicator( return inheritingCharts } +async function findAllMultiDimViewsThatInheritFromIndicator( + trx: db.KnexReadonlyTransaction, + variableId: number +): Promise< + { + chartConfigId: string + patchConfig: GrapherInterface + isPublished: boolean + }[] +> { + const charts = await db.knexRaw<{ + chartConfigId: DbPlainMultiDimXChartConfig["chartConfigId"] + patchConfig: DbRawChartConfig["patch"] + isPublished: boolean + }>( + trx, + `-- sql + SELECT + mdxcc.chartConfigId as chartConfigId, + cc.patch as patchConfig, + md.published as isPublished + FROM multi_dim_data_pages md + JOIN multi_dim_x_chart_configs mdxcc ON mdxcc.multiDimId = md.id + JOIN chart_configs cc ON cc.id = mdxcc.chartConfigId + WHERE mdxcc.variableId = ? + `, + [variableId] + ) + return charts.map((chart) => ({ + ...chart, + patchConfig: parseChartConfig(chart.patchConfig), + })) +} + +export async function updateAllMultiDimViewsThatInheritFromIndicator( + trx: db.KnexReadWriteTransaction, + variableId: number, + { + updatedAt, + patchConfigETL, + patchConfigAdmin, + }: { + updatedAt: Date + patchConfigETL?: GrapherInterface + patchConfigAdmin?: GrapherInterface + } +): Promise< + { + chartConfigId: string + patchConfig: GrapherInterface + isPublished: boolean + }[] +> { + const inheritingViews = await findAllMultiDimViewsThatInheritFromIndicator( + trx, + variableId + ) + + for (const view of inheritingViews) { + const fullConfig = mergeGrapherConfigs( + patchConfigETL ?? {}, + patchConfigAdmin ?? {}, + view.patchConfig + ) + await db.knexRaw( + trx, + `-- sql + UPDATE chart_configs + SET + full = ?, + updatedAt = ? + WHERE id = ? + `, + [JSON.stringify(fullConfig), updatedAt, view.chartConfigId] + ) + } + + // let the caller know if any views were updated + return inheritingViews +} + export async function updateGrapherConfigETLOfVariable( trx: db.KnexReadWriteTransaction, variable: VariableWithGrapherConfigs, config: GrapherInterface ): Promise<{ savedPatch: GrapherInterface - updatedCharts: { chartId: number; isPublished: boolean }[] + updatedCharts: { chartConfigId: string; isPublished: boolean }[] + updatedMultiDimViews: { chartConfigId: string; isPublished: boolean }[] }> { const { variableId } = variable @@ -305,6 +397,9 @@ export async function updateGrapherConfigETLOfVariable( variableId, }) + // Set the updatedAt manually instead of letting the DB do it so it is the + // same across different tables. The inconsistency caused issues in the + // past in chart-sync. const now = new Date() if (variable.etl) { @@ -336,19 +431,27 @@ export async function updateGrapherConfigETLOfVariable( }) } + const updates = { + patchConfigETL: configETL, + patchConfigAdmin: variable.admin?.patchConfig, + updatedAt: now, + } const updatedCharts = await updateAllChartsThatInheritFromIndicator( trx, variableId, - { - patchConfigETL: configETL, - patchConfigAdmin: variable.admin?.patchConfig, - updatedAt: now, - } + updates ) + const updatedMultiDimViews = + await updateAllMultiDimViewsThatInheritFromIndicator( + trx, + variableId, + updates + ) return { savedPatch: configETL, updatedCharts, + updatedMultiDimViews, } } @@ -358,7 +461,8 @@ export async function updateGrapherConfigAdminOfVariable( config: GrapherInterface ): Promise<{ savedPatch: GrapherInterface - updatedCharts: { chartId: number; isPublished: boolean }[] + updatedCharts: { chartConfigId: string; isPublished: boolean }[] + updatedMultiDimViews: { chartConfigId: string; isPublished: boolean }[] }> { const { variableId } = variable @@ -377,6 +481,9 @@ export async function updateGrapherConfigAdminOfVariable( patchConfigAdmin ) + // Set the updatedAt manually instead of letting the DB do it so it is the + // same across different tables. The inconsistency caused issues in the + // past in chart-sync. const now = new Date() if (variable.admin) { @@ -395,19 +502,27 @@ export async function updateGrapherConfigAdminOfVariable( }) } + const updates = { + patchConfigETL: variable.etl?.patchConfig ?? {}, + patchConfigAdmin: patchConfigAdmin, + updatedAt: now, + } const updatedCharts = await updateAllChartsThatInheritFromIndicator( trx, variableId, - { - patchConfigETL: variable.etl?.patchConfig ?? {}, - patchConfigAdmin: patchConfigAdmin, - updatedAt: now, - } + updates ) + const updatedMultiDimViews = + await updateAllMultiDimViewsThatInheritFromIndicator( + trx, + variableId, + updates + ) return { savedPatch: patchConfigAdmin, updatedCharts, + updatedMultiDimViews, } } diff --git a/db/tests/testHelpers.ts b/db/tests/testHelpers.ts index d8eee85525c..47e3f231e55 100644 --- a/db/tests/testHelpers.ts +++ b/db/tests/testHelpers.ts @@ -4,6 +4,8 @@ import { ChartRevisionsTableName, ChartsTableName, DatasetsTableName, + MultiDimDataPagesTableName, + MultiDimXChartConfigsTableName, PostsGdocsTableName, UsersTableName, VariablesTableName, @@ -15,6 +17,8 @@ export const TABLES_IN_USE = [ ChartDimensionsTableName, ChartRevisionsTableName, ChartsTableName, + MultiDimXChartConfigsTableName, + MultiDimDataPagesTableName, VariablesTableName, ChartConfigsTableName, DatasetsTableName,