Skip to content

Enable migrations from older database schemas.#1764

Merged
visr merged 9 commits intomainfrom
feat/migrations
Aug 29, 2024
Merged

Enable migrations from older database schemas.#1764
visr merged 9 commits intomainfrom
feat/migrations

Conversation

@evetion
Copy link
Copy Markdown
Member

@evetion evetion commented Aug 26, 2024

Fixes Deltares/Ribasim-NL#138

This has no schema version (stored in the geopackage) yet, but this is something we could implement in the future.

@evetion evetion requested a review from deltamarnix August 26, 2024 16:07
@deltamarnix
Copy link
Copy Markdown
Contributor

deltamarnix commented Aug 27, 2024

I would like to opt to add a geopackage version in this PR and let's call everything that doesn't have it version 0, and from now on we will write a modelversion: 1. Otherwise we might get conflicts when we figure out that more backwards compatibility is required in the coming months. Then while writing migration, first check if the version is correct.

  • change QGIS writing
  • change Ribasim python writing
  • Write the geopackage version during migration if it was successful? (Maybe not, since it's only in memory migration)
  • Check in your migration code what the version is to migrate

For now, let's keep it as simple as you have it, although I would rather see a pipeline that migrates step by step. But that's for another moment in time.

@visr
Copy link
Copy Markdown
Member

visr commented Aug 27, 2024

This doesn't yet include removing listen_node_type columns:

ValidationError: 1 validation error for TableModel[DiscreteControlVariableSchema]
df
  Value error, Unrecognized column 'listen_node_type'.

@evetion
Copy link
Copy Markdown
Member Author

evetion commented Aug 28, 2024

@deltamarnix This now writes the schema version to the geopackage (also from QGIS), which is used to check whether to migrate or not. @visr This now also migrates the other tables with listen_node_type.

Comment thread python/ribasim/ribasim/db_utils.py Outdated
Copy link
Copy Markdown
Contributor

@deltamarnix deltamarnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to see tests for the migration and the writing of the version files (both qgis and ribasim python)

Comment thread python/ribasim/ribasim/db_utils.py Outdated
Comment thread python/ribasim/ribasim/migrations.py
Comment thread ribasim_qgis/core/geopackage.py
evetion and others added 2 commits August 29, 2024 08:57
Co-authored-by: Marnix <150045289+deltamarnix@users.noreply.github.com>
@evetion evetion requested a review from deltamarnix August 29, 2024 11:44
Copy link
Copy Markdown
Contributor

@deltamarnix deltamarnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @SnippenE agrees that loading an old model in QGIS does not do the migration, but crashes instead, I am okay with the changes so far.

@visr visr merged commit 8d89e74 into main Aug 29, 2024
@visr visr deleted the feat/migrations branch August 29, 2024 14:15
@visr visr mentioned this pull request Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Updater from 2024.10.0 to main

3 participants