[rel-db] Initial migration #3152
Conversation
…/openslides-backend into RelDB-Migration
…s-backend into RelDB-Migration
write migration index readd old schema for migration
* remove 'progress' and 'clear-collectionfield-tables'
Co-authored-by: Alexej <[email protected]>
luisa-beerboom
left a comment
luisa-beerboom
left a comment
There was a problem hiding this comment.
Only looked at some of it so far
|
|
||
| def setup_migration_relations(self) -> None: | ||
| """Sets the tables and views used within the migration and copies their data.""" | ||
| # TODO this method needs to be tested. |
There was a problem hiding this comment.
Is it even possible to test this before the first post-relDB Migration happens? If it isn't, perhaps write an issue?
| ).fetchone() | ||
| assert ( | ||
| version_table_exists | ||
| ), "Something really blew up checking the version tables existence." |
luisa-beerboom
left a comment
luisa-beerboom
left a comment
There was a problem hiding this comment.
Looked at everything that is there so far
| The test cases initially may seem to be spartanic caused by the lack of testing of integrity | ||
| after transfering the data from the key-value-store into their respective tables. | ||
| What is tested is the correct creation of intermediate tables and simple relations exemplary as we | ||
| can trust that it works for other tables if it worked for one. | ||
| Also it is tested that the new tables are created on top of an old basis and the old tables are deleted. | ||
| It should be only used like this in this migration test since it leads to a performance problem | ||
| once the actual connection context is entered the first time after the database was dropped and recreated. |
There was a problem hiding this comment.
This comment seems to be under every single test case. Isn't there a more central place this could be put? The check_data function or the top of the file maybe?
Also the text seems to suggest that the integrity of the complete instance is not checked. That breaks with our previous MO as not everything that is changed is also checked. Thinking in terms of what this would mean in the old system: It would risk missing bugs in something as critical as this migration and getting corrupt data as a result. Or missing data. Idk how well that philosophy translates into the new system, but if the risk exists, considering how important this is, I'd suggest testing the content of the new tables more thoroughly in at least one test.
There was a problem hiding this comment.
In the old system, as far as I recall, the database check was always called automatically after every finalize call in in the test of whatever migration was currently the most recent.
There was a problem hiding this comment.
Will move it to the class declaration.
As for what is tested I think that as long as one sample for a certain kind works in this migration that should be sufficient. If I would test the complete data set this could be done in all tests but needed much more development time without giving much more security that everything works well.
In future migration tests the previous MO should be the standard again. For that we can have a similar behaviour where the data is checked directly by pulling it through the database adapter or so.
* do some handling of migration tables * use new migration states * test improvements (f.e. thread syncing, sequences) * update meta
|
Looked into the relDB branch: |
* requested changes * fix setting to running as first action * test fixes * docs * replace trigger * hard coded ORIGIN_COLLECTIONS * unskip initial import tests * update meta
Co-authored-by: luisa-beerboom <[email protected]> Co-authored-by: peb-adr <[email protected]>
Co-authored-by: luisa-beerboom <[email protected]>
|
Reactivated the initial import tests. Didn't do that for the meeting import, because that has some other implications. Probably better in its own PR. |
5 participants
Continuation of #2520