Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/add rerun on change #3112

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Tenshock
Copy link

fixes #3111

@Tenshock
Copy link
Author

Tenshock commented Mar 12, 2024

I voluntarily break the commits into more succinct ones in order to discuss about implementation easily

@Tenshock Tenshock marked this pull request as ready for review March 13, 2024 10:32
@abonander
Copy link
Collaborator

I don't think it makes sense to have these at the same level as the others, especially since they're explicitly not versioned. Instead of .onchange.sql, I think it should be a subfolder,

I'd propose migrations/after-migrate/ for less ambiguity (what does "change" mean? it's not as obvious as you might think). This also gives us room to extend it in the future, e.g.:

  • migrations/after-migrate-up/, executed only after upgrading from a previous version.
  • migrations/after-migrate-down/, executed only after downgrading from a previous version.
  • migrations/before-migrate/ and -up and -down variants, for executing scripts before migrating (maybe to kick off a backup or something).

You should also not rely on the filesystem returning entries in any defined order. It can differ between platforms and filesystems. For this reason, I'd recommend using the same filename format and applying scripts by version number just like migrations are.

@Tenshock Tenshock force-pushed the feat/add-rerun-on-change branch 2 times, most recently from 5efa823 to 544538a Compare March 18, 2024 15:31
@Tenshock
Copy link
Author

Tenshock commented Mar 19, 2024

Do we discuss about the choices and implementations here or in the related issue?

About the meaning of "change"

I may have poorly explained the process and the underlying logic in the related issue. As I think about it, a change is a modification of an entity. This leads to the question of what is an entity? An entity is a thing that has a unique identifier.

In the implementation of Simple, ReverseUp and ReverseDown migrations, the identifier of a script is its version, extracted from the file name numeric prefix. And the changes are calculated from its checksum. So, in this case, a change is a checksum change for a given version.

As we agree that OnChange migrations does not have a version as it is irrelevant, the proposed implementation uses the description of the file as its identifier. So, in this case, a change is a checksum change for a given description.

For the design choice between the file suffix .onchange.sql and the sub-folder strategy

I think I see your point.

As I understand it in the lib, I see the migrations folder as de default location where sqlx finds its scripts. It can be overridden. I think this is the user responsibility to organise its scripts as he wants to, I wouldn't base some behaviours based in the script location.

About the logic behind the migrations/[after|before]-migrate-[up|down], if we choose to use the proposed implementation, we can think of an Always migration with a .always.sql suffix and we leave the responsibility to the user to move its scripts to relevant folder and to proceed to a potential workflow as follows:

# Say the user has this structure
.
├── after_migrate
│   ├── clean_logs.always.sql
│   └── update_ref_table.always.sql
├── before_migrate
│   ├── backup_database.always.sql
└── migrations
    ├── 0001_my_simple_script.sql
    └── 0002_another_simple_script.sql

We can perfectly do:

sqlx migrate run --source before_migrate --ignore-missing
sqlx migrate run  # --source migrations
sqlx migrate run --source after_migrate --ignore-missing

The main downfall (if it is really one) with the current capacities of the sqlx CLI is the need of --ignore-missing option to not trigger the validation process as we use different folder but it works perfectly in this use-case. As we do not track versions for OnChange or Always migrations, it is perfectly justified and fine.

Overall, SQLx already provides quite all the mechanisms to do it.

About the order to apply OnChange migrations

There is two orders to think about. The first is, at which time do we apply the OnChange migrations? Before or after the regular ones? I think the answer is explained above with the proposed workflow. Another possibility if all the scripts are in the same folder is to add a parameter like --run-always-first, --run-always-last, or perhaps to implement a weight mechanism for these not versioned scripts.

For the order between the OnChange (and potential Always ) migrations that do not have a version, I see three possibilities:

  • We enforce a sequential or timestamp version like the current behaviour
  • We leave the choice to the user to add or not the possibility to add a version
  • The order is random, like you said, order can differ between platforms and filesystems.

If we enforce a prefix version, internally, the script identifier is still the description, the version only having responsibility to enforce an order.

I hope my explanations are clear, I truly want to enhance this great lib.

By the way, I changed the implementation for the stored internal version for OnChange scripts from the last billion before the i64 max value to negative versions. As it is marked as invalid in the source code for regular migration scripts.

@Tenshock Tenshock force-pushed the feat/add-rerun-on-change branch 4 times, most recently from 802adba to 79b33f8 Compare April 4, 2024 13:29
@Tenshock
Copy link
Author

Tenshock commented Apr 9, 2024

Hello @abonander, any news about the suggestions? :)

The main points are:

  • .onchange.sql scripts vs subfolder strategy
  • scripts execution order

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.

add On Change migrations types
2 participants