fix 1444 clean-up some old code

[toni-sharpe]

• removes field which seems to have been sitting around waiting to be removed for some time, so here you go

Fixes #1444

Charts

The chart, 1237, does not exist in the admin/test area anymore, ignored

Authors

I've taken Hannah and Max's responses plus subsequent label changes to mean all is well with removing.

Figured the lack of interest from them meant I could just move on

Migration

Seems to work ...

Output details collapsed

Of course, there's a command!

And that looks good too.

yarn runDbMigrations
query: SELECT VERSION() AS `version`
query: SELECT * FROM `INFORMATION_SCHEMA`.`COLUMNS` WHERE `TABLE_SCHEMA` = 'grapher' AND `TABLE_NAME` = 'migrations'
query: SELECT * FROM `grapher`.`migrations` `migrations` ORDER BY `id` DESC
180 migrations are already loaded in the database.
134 migrations were found in the source code.
RemoveEntitiesDisplayName1720038601815 is the last executed migration. It was executed on Wed Jul 03 2024 20:30:01 GMT+0000 (GMT).
1 migrations are new migrations must be executed.
query: START TRANSACTION
query: -- sql
            UPDATE charts
            SET config = JSON_REMOVE(config, '$."hideLinesOutsideTolerance"')
            WHERE
                type = "ScatterPlot"
            AND
                slug IN (
                  "stunting-vs-level-of-prosperity-over-time",
                  "growth-of-income-and-trade"
                )

query: INSERT INTO `grapher`.`migrations`(`timestamp`, `name`) VALUES (?, ?) -- PARAMETERS: [1721137710001,"RemoveHideLinesOutsideToleranceFromConfig1721137710001"]
Migration RemoveHideLinesOutsideToleranceFromConfig1721137710001 has been  executed successfully.
query: COMMIT

[danyx23]

Looks good! I had some minor requests for changes, once those are in this would be good to merge :)

[danyx23]

It's better not to hard-code these slugs. Removing the property is the right thing to do in any case and there might be new charts created in the mean time between looking these up and the PR being merged.

[toni-sharpe]

Fair enough, I decided to keep it tight in case of breaking something unrelated, but the opposite is the case, I'll make that change

[danyx23]

You correctly bump the schema version in this PR (nice!) - but then to make it consistent we should also bump all existing schema versions in the charts table as part of this migration (since the migration above makes sure that they actually adhere to the new schema)

[toni-sharpe]

@danyx23 noted the positive feedback in the PR comment, I think this is a really good habit for devs in general, thanks :-)

[toni-sharpe]

Passed local re-run of yarn runDbMigrations after refreshing the local env

[danyx23]

@toni-sharpe thanks for the changes but there is an issue with it.

When I said to "bump all existing schema versions in the charts table" I meant to write the sql query to set this as part of the migration.

What you did was modify existing, old migrations. This is conceptually something that one should almost never do - migrations are a series of immutable up/down scripts to bring a database or other datastore from one know schema version to another. A database that the migrations are run against might be at the status of any point in between migrations and these migrations should always perform the same logic and have the same effect. If you modify migrations, this property is violated.

It would also not lead to all the rows in the charts table having the correct, new schema version in the $schema key in their config.

What we should do here is execute some sql like this:

update charts 
set config = json_set(config, '$.$schema', 'https://files.ourworldindata.org/schemas/grapher-schema.005.json');

[toni-sharpe]

@danyx23 right yes, apologies, my migration experience is a little rusty, it's been a while, let me make that change and I'll ping you when it's done.

[toni-sharpe]

@danyx23 New commit is actually just your example, updates the schema key in the JSON for charts config. The reverted commit should remove the mistake and a down part of the migration sets the schema back to 004.

[toni-sharpe]

@danyx23 I felt your suggested code was correct, re-building seems to work for me locally, I had missed the timestamp on the second migration, that's now added, the output for me:

I have now I think fully fixed this I really hope, getting useed to TypeORM, had to move the migrations so they were last, as I think they have to be last when added, to keep it right.

Local output

query: INSERT INTO `grapher`.`migrations`(`timestamp`, `name`) VALUES (?, ?) -- PARAMETERS: [1722419367805,"RemoveHideLinesOutsideToleranceFromConfig1722419367805"]
Migration RemoveHideLinesOutsideToleranceFromConfig1722419367805 has been  executed successfully.
query: -- sql
            UPDATE charts
            SET config = JSON_SET(config, '$.$schema', 'https://files.ourworldindata.org/schemas/grapher-schema.005.json');

query: INSERT INTO `grapher`.`migrations`(`timestamp`, `name`) VALUES (?, ?) -- PARAMETERS: [1722419367806,"UpdateSchemaForChartConfigChange1722419367806"]
Migration UpdateSchemaForChartConfigChange1722419367806 has been  executed successfully.
query: COMMIT

[toni-sharpe]

I also had a go at brevity in the PR description, it's a thing I've wated to improve and Lars has helped with a few pointers, so I applied those for practice.

[toni-sharpe]

@danyx23 gentle reminder that this is still open.

There is now a conflict in the schema, which I will try to resolve over the next day or so. I thought I had this ready, but now I'll take a look at the new problem and hopefully get it ready for merge.

[toni-sharpe]

@danyx23 @larsyencken sorry to tag both, August is a barren month, this is ready I think, I'd quite like it merged before new migrations go in (I think the latest merged have to be the latest time-stamp).

[danyx23]

Hey @toni-sharpe, sorry for the radio silence, I had a pretty busy time. The changes look good now!

We'll need to remove this property also from the schema definition in the ETL repository - could you send a PR to the ETL repo that removes the lines following https://github.com/owid/etl/blob/b07710d343a44e4c16091070ff0924e6e1d99b0f/schemas/dataset-schema.json#L1597 as well?

I would then merge both next week.

[toni-sharpe]

@danyx23 owid/etl#3143

The second commit can be reverted, it sets schemas all equal and incremented

[danyx23]

@toni-sharpe thanks! I left a comment over there. We have some changes coming in that will also touch the schema versions in a more complex way and it will be easier to merge your PR after those PRs. So I'll wait a few more days, but thanks for the cleanup!

[toni-sharpe]

@danyx23 no problem, reverted on the other PR

[toni-sharpe]

@danyx23 this is ready for a merge I think, conscious that new migrations mean I'll need to change my migration numbers to retain sync (I think).

[toni-sharpe]

@danyx23 latext commit to keep migrations latest (I'm sure they have to be)

[toni-sharpe]

@danyx23

I can see from your comments that you wanted to wait before the merge with this, but, the waiting is creating updating, which are fiddly with migrations,

I've rebased and force pushed, but I hit a conflict I had to manage manually, specifically, I removed this:

<<<<<<< HEAD:packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml
$id: "https://files.ourworldindata.org/schemas/grapher-schema.004.json"
# if you update the required keys, make sure that the mergeGrapherConfigs and
# diffGrapherConfigs functions both reflect this change
=======

Also would this technically be schema 6? I'd suggest putting both as 5 just for the sake of simplicity, the sooner this is merged, the better IMO.

Please let me know what's going on with the bigger picture, if you have the time, for the sake of my broader knowledge as well as this PR.

[danyx23]

Hi @toni-sharpe, sorry again that I wasn't more active, we were pretty busy trying to push a bigger project forward before a few key people go on vacation.

We have merged a bunch of changes recently that are relevant for this PR. Key among them is that we introduced a chart_configs table that holds different kinds of grapher chart configs - not just the ones for standalone charts defined in the charts table, but also e.g. indicator defaults so we can plot just an indicator on it's own.

Unfortunately this means that this PR has to change a bit now - the migration you had in place touched the config column in the charts table - now this migration has to change to instead touch both the partial and full config of the chart_configs table.

The other thing I noticed is that the schema yaml had a change that moved the $defs section to the bottom. I guess something must have happened in your rebase because your diff now adds this back at the top but this would then lead to it existing both at the top and the bottom which would be invalid. Probably the easiest is to just remove your section at the top.

I'm sorry that this PR was stalled for so long and that now non-trivial changes are required for it to be merged. If you like I'd be happy to take over and make the remaining adjustments and merge, but you are also welcome to make them of course if you are still up for it!

[danyx23]

One more tipp: you can run the command make dbtest to run our database tests. This is a pretty small suite of tests so far but they run against a real database. One thing that they test amongst other is the validit of the migrations which can be useful as a sanity check.

[toni-sharpe]

@danyx23 no problem, I'll try;

Appreciate the situation, work is based on what the team prioritises, only reason I pushed herre was the DB migration angle.

I'll run the tests, and I'll check the migration work, let's see where I get,

[toni-sharpe]

@danyx23 I think that's it:

• the chart config moved so now the focus is on a specific chart_configs table, not the charts table
• didn't see the need to restrict by type, it would be a slower query,
• did away with all the 005 schema stuff, assuming the new chart stuff covered that (my merge issues suggest this in fact)

Then also gotr rid of the dupe from my rebase.

Tests seemed to work, but I saw the Waiting ...... message which never ends, I had to assume the tests had passed (I have a hacky fix for this). I gave it longer than I would to build the working DB.

Instead I flattened everything and re-run the app, was able to see migrations run, TMux run and to login at :3030/admin/test

Migration output

query: INSERT INTO `grapher`.`migrations`(`timestamp`, `name`) VALUES (?, ?) -- PARAMETERS: [1724249694337,"CreateMultiDimTable1724249694337"]
Migration CreateMultiDimTable1724249694337 has been  executed successfully.
query: -- sql
          UPDATE chart_configs cc
          JOIN charts c ON c.configId = cc.id
          SET
              -- remove NULLs from the patch config
              cc.patch = JSON_REMOVE(cc.patch, '$.stackMode'),
              -- replace NULLs with the default value in the full config
              cc.full = JSON_REPLACE(cc.full, '$.stackMode', 'absolute')
          WHERE cc.patch ->> '$.stackMode' = 'null'

query: INSERT INTO `grapher`.`migrations`(`timestamp`, `name`) VALUES (?, ?) -- PARAMETERS: [1725540224795,"CleanUpChartConfigStackMode1725540224795"]
Migration CleanUpChartConfigStackMode1725540224795 has been  executed successfully.
query: -- sql
            UPDATE
                chart_configs
            SET
                full = JSON_REMOVE(
          full,
          '$."hideLinesOutsideTolerance"'
        ),
                patch = JSON_REMOVE(
          patch,
          '$."hideLinesOutsideTolerance"'
        )

query: INSERT INTO `grapher`.`migrations`(`timestamp`, `name`) VALUES (?, ?) -- PARAMETERS: [1725540224800,"RemoveHideLinesOutsideToleranceFromChartConfig1725540224800"]
Migration RemoveHideLinesOutsideToleranceFromChartConfig1725540224800 has been  executed successfully.
query: COMMIT

Charts seem to be loading

[toni-sharpe]

@danyx23

few key people go on vacation

Is there anything I can do to help with that?

[toni-sharpe]

I've removed lines 529 & 530

And added : string to my fn, but also noticed that's not the style of the rest of the code

[danyx23]

Hey @toni-sharpe! I'm doing a bit of cleanup on your PR before I merge. Thanks a lot for the work, I'll merge it today!

[toni-sharpe]

@danyx23 I was interested in the fix and errors

Linting done, not sure 530 is needed? It's based on defaultGrapherConfig which I didn't dig into yet, but seems clean here. Plus, removing that line passes the test

I'll not push

[danyx23]

@toni-sharpe yeah I didn't run the lint locally - I did this a little differently now. The point of this is that the function saveGrapher should be able to accept older schema versions (e.g. version 4 that still had this field). The typescript type definition claims that the newConfig value will match GrapherInterface which only models the latest version of the schema so this is not stricly speaking the correct type but at this point it's not worth the hassle of typing this fully correctly but also not better to declare newConfig as any type in the params since this makes typos etc more likely.

[toni-sharpe]

OK so there's a cross-over between the old and new ... the new won't, the old might, so only delete on the old

grep any the way to find out if they do TypeScript :-)

Thanks for the more detailed description.