foundryvtt / pf2e

A community contributed game system for Pathfinder Second Edition.
https://foundryvtt.com/packages/pf2e
Apache License 2.0
423 stars 355 forks source link

System migration compatibility with `DataModel`s #15545

Open stwlam opened 4 months ago

stwlam commented 4 months ago

System migrations are currently asynchronous and called from a "ready" hook. In order for full adoption of actor and item TypeDataModels the system migration framework needs to be adapted to it. The current hazy plan is:

  1. TypeDataField#migrateSource overrides should check the document's schema version: if lower than a certain cutoff point, data-model migrations are skipped.
  2. System asynchronous migrations operate on the document, catching it up to the schema version cutoff.
  3. The document's TypeDataField is notified to recheck and run migrations for schema versions past the cutoff point, which are exclusively synchronous.
  4. A no-diff update is made for each migrated document, synchronizing the data maintained by the server and what is found in #_source.

Questions/challenges:

CarlosFdez commented 4 months ago

I asked openly in the discord, but would #2 work if a data model exists for the document? @In3luki was under the impression that it will not because the source data will be altered by the data model, and that v12 has not changed in this regard. There was no clarification otherwise in dev-general.

EDIT: Clarified separately. By not supering up, we can avoid the butchering.

CarlosFdez commented 4 months ago

Though we may wish to avoid it in the future, there may be a very strong perceived need for later asynchronous migrations. Should this be anticipated as inevitable and somehow accommodated?

This can only be addressed in core, but one advantage of sync migrations and duplicating the data we're overwriting to make that work is that we don't have to be afraid of future updates to the source data. This thing we did was inherently unsafe and its good that we have to avoid it.

One potential compromise that occurred to me though was flagging an item as "needs refresh". This could skip all later migrations and then asynchronously update the item afterwards. Skipping later migrations would avoid the problem scenario of updating an item to its most recent version, then running a migration that assumes an older version, which is a potential problem of our current structure.