Skip to content

Commit

Permalink
🔨 (grapher) let chart configs inherit default values / TAS-565 (#3782)
Browse files Browse the repository at this point in the history
## Summary

Implements chart config inheritance and makes all chart configs inherit values from a parent config with default values.

## Details

- For all standalone charts, the `full` config is the `patch` config merged with the default layer
    - Whenever we save a new chart or update the existing one, we first compute the patch config by diffing against the parent/default config and then compute the full config by merging the patch with the parent
    - We usually operate on the full config, but we're baking the full config without defaults so that we're not baking the defaults again and again and again (makes a difference on explorer pages where we potentially bake tens/hundreds of configs)
    - We use the ~full~ patch config for version history. The way we currently do revisions doesn't always work[^1]. ~Using the full config for version history fixes this, but has the disadvantage that "old" default values (that have since been updated) are copied over when a config is restored.~
- The default grapher config is derived from the schema and lives in the codebase. It can be generated by running the `generate-default-object-from-schema.ts` with the `--save-ts` flag
    - Whenever Grapher defaults are updated, the default config should be regenerated, and all `full` configs of stand-alone charts should be recomputed
    - We could have also stored the default layer in the database, but it's convenient to have it at hand in the codebase (where it's also type-checked). If some external entity needs the default config, they can access https://files.ourworldindata.org/schemas/grapher-schema.default.latest.json
- Two new functions operate on chart configs and implement inheritance:
    - `mergeGrapherConfigs`: merges two or more patch configs
        - Some keys are excluded from inheritance ($schema, id, slug, version, isPublished)
        - Configs with different schema versions can't be merged
    - `diffGrapherConfigs`: diffs a given config against some reference config, where only values that are different in both configs are kept

## Caveats

- ~I left the chart revisions as is but now store the full config in the chart revisions table whenever a chart is saved – this could be a bit confusing~
- ~As I said above, storing the full config in version history comes at the cost of copying over old default values when a chart is restored – how big of a problem is that?~

## SVG tester

I dumped two new set of configs and reference charts for the SVG tester:
- [Patch configs](https://github.com/owid/owid-grapher-svgs/compare/configs-2024-july...configs-july-2024-defaults?short_path=4814afd#diff-4814afdf72df1537e0f28ca29966dde09611474e3ed214d1609754491faa83e0)
    - The config changes are due to diffing against the default layer (meaning, default values are removed)
    - There is one chart that is rendered differently, but it seems to be unrelated to the changes in this PR. 
- [Full configs](owid/owid-grapher-svgs@configs-2024-08-20...configs-2024-08-20-defaults-2)
    - Few charts are added and removed, probably because I refreshed the database in between
    - Other than that, no changes

## To do

- ~Add some real-world tests~ it's ok as is
- [x] Add a GitHub workflow that re-generates the default config and checks if the diff is clean
- [x] Delete `DEFAULT_GRAPHER_CONFIG_SCHEMA`

[^1]: When restoring a config from version history, we re-apply an old config version. But that doesn't take default values into account. For example, let's say the v1 version of a chart relies on the default value for `hasMapTab` and has thus no map tab. In v2, a map tab is added (the config now contains `{..., hasMapTab: true}`. Restoring v1 doesn't switch `hasMapTab` back to `false` since `hasMapTab` isn't contained in the v1 version of the config (since we were relying on the default).
  • Loading branch information
sophiamersmann authored Sep 3, 2024
1 parent 6c3057a commit 3cc05db
Show file tree
Hide file tree
Showing 30 changed files with 1,035 additions and 129 deletions.
54 changes: 54 additions & 0 deletions .github/workflows/check-default-grapher-config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
name: Check default grapher config
on:
push:
branches:
- "**"
- "!master"
paths:
- "packages/@ourworldindata/grapher/src/schema/**"

jobs:
commit-default-config:
runs-on: ubuntu-latest

steps:
- name: Clone repository
uses: actions/checkout@v4
with:
ref: ${{ github.head_ref }}

- uses: ./.github/actions/setup-node-yarn-deps
- uses: ./.github/actions/build-tsc

- uses: hustcer/setup-nu@v3
with:
version: "0.80" # Don't use 0.80 here, as it was a float number and will be convert to 0.8, you can use v0.80/0.80.0 or '0.80'

# Turn all yaml files in the schema directory into json (should only be one)
- name: Convert yaml schema to json
run: |
(ls packages/@ourworldindata/grapher/src/schema/*.yaml
| each {|yaml|
open $yaml.name
| to json
| save -f ($yaml.name
| path parse
| upsert extension "json"
| path join) })
shell: nu {0}

# Construct default config objects for all grapher schemas in the schema directory (should only be one)
- name: Generate default grapher config
run: |
(ls packages/@ourworldindata/grapher/src/schema/grapher-schema.*.json
| each {|json|
node itsJustJavascript/devTools/schema/generate-default-object-from-schema.js $json.name --save-ts packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts })
shell: nu {0}

- name: Run prettier
run: yarn fixPrettierAll

- uses: stefanzweifel/git-auto-commit-action@v5
with:
commit_message: "🤖 update default grapher config"
file_pattern: "packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts"
1 change: 1 addition & 0 deletions adminSiteClient/GrapherConfigGridEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ export class GrapherConfigGridEditor extends React.Component<GrapherConfigGridEd
selectedRowContent.id
)

// TODO(inheritance): use mergeGrapherConfigs instead
const mergedConfig = merge(grapherConfig, finalConfigLayer)
this.loadGrapherJson(mergedConfig)
}
Expand Down
209 changes: 147 additions & 62 deletions adminSiteServer/apiRouter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ import {
Json,
checkIsGdocPostExcludingFragments,
checkIsPlainObjectWithGuard,
mergeGrapherConfigs,
diffGrapherConfigs,
} from "@ourworldindata/utils"
import { applyPatch } from "../adminShared/patchHelper.js"
import {
Expand Down Expand Up @@ -86,6 +88,7 @@ import {
} from "@ourworldindata/types"
import { uuidv7 } from "uuidv7"
import {
defaultGrapherConfig,
getVariableDataRoute,
getVariableMetadataRoute,
} from "@ourworldindata/grapher"
Expand Down Expand Up @@ -269,6 +272,120 @@ const expectChartById = async (
throw new JsonError(`No chart found for id ${chartId}`, 404)
}

const saveNewChart = async (
knex: db.KnexReadWriteTransaction,
{ config, user }: { config: GrapherInterface; user: DbPlainUser }
): Promise<GrapherInterface> => {
// if the schema version is missing, assume it's the latest
if (!config.$schema) {
config.$schema = defaultGrapherConfig.$schema
}

// if isPublished is missing, add it
if (!config.isPublished) {
config.isPublished = false
}

// compute patch and full configs
const parentConfig = defaultGrapherConfig
const patchConfig = diffGrapherConfigs(config, parentConfig)
const fullConfig = mergeGrapherConfigs(parentConfig, patchConfig)

// insert patch & full configs into the chart_configs table
const configId = uuidv7()
await db.knexRaw(
knex,
`-- sql
INSERT INTO chart_configs (id, patch, full)
VALUES (?, ?, ?)
`,
[configId, JSON.stringify(patchConfig), JSON.stringify(fullConfig)]
)

// add a new chart to the charts table
const result = await db.knexRawInsert(
knex,
`-- sql
INSERT INTO charts (configId, lastEditedAt, lastEditedByUserId)
VALUES (?, ?, ?)
`,
[configId, new Date(), user.id]
)

// The chart config itself has an id field that should store the id of the chart - update the chart now so this is true
const chartId = result.insertId
patchConfig.id = chartId
fullConfig.id = chartId
await db.knexRaw(
knex,
`-- sql
UPDATE chart_configs cc
JOIN charts c ON c.configId = cc.id
SET
cc.patch=JSON_SET(cc.patch, '$.id', ?),
cc.full=JSON_SET(cc.full, '$.id', ?)
WHERE c.id = ?
`,
[chartId, chartId, chartId]
)

return patchConfig
}

const updateExistingChart = async (
knex: db.KnexReadWriteTransaction,
{
config,
user,
chartId,
}: { config: GrapherInterface; user: DbPlainUser; chartId: number }
): Promise<GrapherInterface> => {
// make sure that the id of the incoming config matches the chart id
config.id = chartId

// if the schema version is missing, assume it's the latest
if (!config.$schema) {
config.$schema = defaultGrapherConfig.$schema
}

// if isPublished is missing, add it
if (!config.isPublished) {
config.isPublished = false
}

// compute patch and full configs
const parentConfig = defaultGrapherConfig
const patchConfig = diffGrapherConfigs(config, parentConfig)
const fullConfig = mergeGrapherConfigs(parentConfig, patchConfig)

// update configs
await db.knexRaw(
knex,
`-- sql
UPDATE chart_configs cc
JOIN charts c ON c.configId = cc.id
SET
cc.patch=?,
cc.full=?
WHERE c.id = ?
`,
[JSON.stringify(patchConfig), JSON.stringify(fullConfig), chartId]
)

// update charts row
await db.knexRaw(
knex,
`-- sql
UPDATE charts
SET lastEditedAt=?, lastEditedByUserId=?
WHERE id = ?
`,
[new Date(), user.id, chartId]
)

return patchConfig
}

const saveGrapher = async (
knex: db.KnexReadWriteTransaction,
user: DbPlainUser,
Expand Down Expand Up @@ -349,68 +466,23 @@ const saveGrapher = async (
else newConfig.version = 1

// Execute the actual database update or creation
const now = new Date()
let chartId = existingConfig && existingConfig.id
const newJsonConfig = JSON.stringify(newConfig)
let chartId: number
if (existingConfig) {
await db.knexRaw(
knex,
`-- sql
UPDATE chart_configs cc
JOIN charts c ON c.configId = cc.id
SET
cc.patch=?,
cc.full=?
WHERE c.id = ?
`,
[newJsonConfig, newJsonConfig, chartId]
)
await db.knexRaw(
knex,
`-- sql
UPDATE charts
SET lastEditedAt=?, lastEditedByUserId=?
WHERE id = ?
`,
[now, user.id, chartId]
)
chartId = existingConfig.id!
newConfig = await updateExistingChart(knex, {
config: newConfig,
user,
chartId,
})
} else {
const configId = uuidv7()
await db.knexRaw(
knex,
`-- sql
INSERT INTO chart_configs (id, patch, full)
VALUES (?, ?, ?)
`,
[configId, newJsonConfig, newJsonConfig]
)
const result = await db.knexRawInsert(
knex,
`-- sql
INSERT INTO charts (configId, lastEditedAt, lastEditedByUserId)
VALUES (?, ?, ?)
`,
[configId, now, user.id]
)
chartId = result.insertId
// The chart config itself has an id field that should store the id of the chart - update the chart now so this is true
newConfig.id = chartId
await db.knexRaw(
knex,
`-- sql
UPDATE chart_configs cc
JOIN charts c ON c.configId = cc.id
SET
cc.patch=JSON_SET(cc.patch, '$.id', ?),
cc.full=JSON_SET(cc.full, '$.id', ?)
WHERE c.id = ?
`,
[chartId, chartId, chartId]
)
newConfig = await saveNewChart(knex, {
config: newConfig,
user,
})
chartId = newConfig.id!
}

// Record this change in version history

const chartRevisionLog = {
chartId: chartId as number,
userId: user.id,
Expand Down Expand Up @@ -461,7 +533,7 @@ const saveGrapher = async (
await db.knexRaw(
knex,
`UPDATE charts SET publishedAt=?, publishedByUserId=? WHERE id = ? `,
[now, user.id, chartId]
[new Date(), user.id, chartId]
)
await triggerStaticBuild(user, `Publishing chart ${newConfig.slug}`)
} else if (
Expand All @@ -479,7 +551,10 @@ const saveGrapher = async (
} else if (newConfig.isPublished)
await triggerStaticBuild(user, `Updating chart ${newConfig.slug}`)

return chartId
return {
chartId,
savedPatch: newConfig,
}
}

getRouteWithROTransaction(apiRouter, "/charts.json", async (req, res, trx) => {
Expand Down Expand Up @@ -756,7 +831,7 @@ apiRouter.get(
)

postRouteWithRWTransaction(apiRouter, "/charts", async (req, res, trx) => {
const chartId = await saveGrapher(trx, res.locals.user, req.body)
const { chartId } = await saveGrapher(trx, res.locals.user, req.body)

return { success: true, chartId: chartId }
})
Expand All @@ -779,10 +854,20 @@ putRouteWithRWTransaction(
async (req, res, trx) => {
const existingConfig = await expectChartById(trx, req.params.chartId)

await saveGrapher(trx, res.locals.user, req.body, existingConfig)
const { chartId, savedPatch } = await saveGrapher(
trx,
res.locals.user,
req.body,
existingConfig
)

const logs = await getLogsByChartId(trx, existingConfig.id as number)
return { success: true, chartId: existingConfig.id, newLog: logs[0] }
return {
success: true,
chartId,
savedPatch,
newLog: logs[0],
}
}
)

Expand Down
1 change: 1 addition & 0 deletions baker/GrapherBaker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ export async function renderDataPageV2(
// this is true for preview pages for datapages on the indicator level but false
// if we are on Grapher pages. Once we have a good way in the grapher admin for how
// to use indicator level defaults, we should reconsider how this works here.
// TODO(inheritance): use mergeGrapherConfigs instead
const grapher = useIndicatorGrapherConfigs
? mergePartialGrapherConfigs(grapherConfigForVariable, pageGrapher)
: pageGrapher ?? {}
Expand Down
1 change: 1 addition & 0 deletions baker/siteRenderers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,7 @@ export const renderExplorerPage = async (
config: row.grapherConfigETL as string,
})
: {}
// TODO(inheritance): use mergeGrapherConfigs instead
return mergePartialGrapherConfigs(etlConfig, adminConfig)
})

Expand Down
Loading

0 comments on commit 3cc05db

Please sign in to comment.