Skip to content

Conversation

@Sh099078
Copy link
Contributor

@Sh099078 Sh099078 commented Nov 21, 2025

When testing the Diesel migrations, I noticed that several down.sql were broken for various reasons (empty scripts, tables deleted in the wrong order, migrations that delete or alter objects in their up script and do not revert the changes correctly in the down script).

The PR fixes those migrations reverts.

To check the broken migrations: run diesel migration redo --all with an up-to-date database on the dev branch. The command will fail during migration reverts. The same command should work on this branch.

Note: the problem was often in a different migration than the one that broke. Some migration reverts were valid and coherent with their up.sql but broke due to the revert of other migrations that did not correctly reset the database schema to its prior state on revert. It happens quite a lot with the migrations generated automatically with the command make-migration, as they tend to delete and re-create the objects they manipulate in their up.sql and delete the same objects in the down.sql no matter if the objects existed before the migration or not. Those generated migration scripts work fine if the said objects didn't exist before but if they did, their revert leave the schema in a different state compared to before applying them, which then causes the revert of earlier migrations to crash.


Example:

That migration has coherent up and down scripts:

-- 2023-12-18-145922_search_op_rename_table/up.sql
ALTER TRIGGER search_operationalpoint__ins_trig ON infra_object_operational_point
RENAME TO search_operational_point__ins_trig;
-- [...]
-- 2023-12-18-145922_search_op_rename_table/down.sql
ALTER TRIGGER search_operational_point__ins_trig ON infra_object_operational_point
RENAME TO search_operationalpoint__ins_trig;

But reverting it breaks because of the following migration:

-- 2023-12-19-122428_search_op_ci_code/up.sql

-- DO NOT EDIT THIS FILE MANUALLY!
-- To change the migration's content, use `editoast search make-migration`.
-- To add custom SQL code, check out `#[derive(Search)]` attributes `prepend_sql` and `append_sql`.

--[...]
CREATE OR REPLACE TRIGGER search_operational_point__ins_trig
AFTER INSERT ON "infra_object_operational_point"
FOR EACH ROW EXECUTE FUNCTION search_operational_point__ins_trig_fun();
--[...]
-- 2023-12-19-122428_search_op_ci_code/down.sql

-- DO NOT EDIT THIS FILE MANUALLY!

-- [...]
DROP TRIGGER IF EXISTS search_operational_point__ins_trig ON "infra_object_operational_point";
-- [...]

There are several migrations that break in the same manner because of deleted tables / functions / triggers in later migrations (including the revert of init.sql 😅)

The cleanest fix would be to modify the make-migration command and re-generate those migration scripts correctly. We could add a way to revert the object changes to their prior state instead of deleting them in the generated scripts. However, that sort of fix seems much more complex and time consuming than manually editing the faulty scripts and the end result would be the same. I chose not to go down that road and focus in this pull request on making the Diesel migrations reverts work again in a simple and fast manner.

For now, I directly edited the faulty generated migration scripts and ignored their warning message -- DO NOT EDIT THIS FILE MANUALLY, as it is their generation that is broken and it is not being fixed in this pull request. I could add a note in the relevant scripts to mention the manual fix, give some context and maybe add a TODO for a cleaner fix ?


Note: there might be other issues with the migrations reverts. This pull request only fixes the obvious ones, i.e. the ones that lead diesel migration redo --all to crash.

forgot to implement the revert script
The `make-migration` cli command generates up and down sql migration
scripts that delete their related triggers and functions.

If causes compatibility issues if prior migrations manipulate the same
objects.

The migration that is being fixed in the current commit deletes triggers
that its up.sql script re-created. Those triggers already existed prior
to the migration though, so its down.sql script should revert them to their
prior state instead of deleting them.

This commit is a quick fix on the generated down.sql script. It is a
fast way to make migration reverts work again but it doesn't fix the root
cause of the issue: `make-migration` should be updated so that we can
revert the PostgreSQL objects its script manipulate instead of deleting
them in the generated down.sql scripts.
@github-actions github-actions bot added the area:editoast Work on Editoast Service label Nov 21, 2025
@Sh099078 Sh099078 marked this pull request as ready for review November 21, 2025 10:39
@Sh099078 Sh099078 requested a review from a team as a code owner November 21, 2025 10:39
@Sh099078 Sh099078 self-assigned this Nov 21, 2025
@Sh099078 Sh099078 moved this to Awaiting merge in Board PI 17 Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:editoast Work on Editoast Service

Projects

Status: Awaiting merge

Development

Successfully merging this pull request may close these issues.

2 participants