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

Allow DataModel.migrateData to be called recursively for models which are not direct children of the root model schema. #8763

Closed
Fyorl opened this issue Jan 20, 2023 · 1 comment
Assignees
Labels
api Issues related to the API used by Mod Devs bug Functionality which is not working as intended data-models Issues related to data models and schema changes

Comments

@Fyorl
Copy link
Contributor

Fyorl commented Jan 20, 2023

Implemented Solution

Data model migration (via the DataModel.migrateData method) is now fully recursive allowing for models which are not direct descendants of the root DataModel to also be migrated.

Example Usage

fields = foundry.data.fields;

class InnerModel extends foundry.abstract.DataModel {
  static defineSchema() {
    return {
      foo: new fields.StringField()
    }
  }

  static migrateData(data) {
    data.foo = "baz";
  }
}

class OuterModel extends foundry.abstract.DataModel {
  static defineSchema() {
    return {
      inner: new fields.ArrayField(new fields.EmbeddedDataField(InnerModel))
    }
  }
}

om = new OuterModel({inner: [{foo: "bar"}]})
om.inner[0].foo; // baz

Original Report

Originally reported by Mana#4176 https://discord.com/channels/170995199584108546/956306859491471420/1065042256559624202

ALL MODULES DISABLED? true
OS, Hosting, Browser (if applicable): any
Short Description of bug: DataModel#migrateData() is not run for EmbeddedDataField contents.
Simple steps to reproduce the bug:
Implement DataModel with noisy migrateData in EmbeddedDataField.
You'll notice the migrateData is never run.

Update: This may be a problem only when the EmbededDataField is inside another data field, like ArrayField or SchemaField.
Update 2: Minimal example fails to reproduce the issue (when used in dev tools console without hooking into system models).

@Fyorl Fyorl added bug Functionality which is not working as intended api Issues related to the API used by Mod Devs data-models Issues related to data models and schema changes labels Jan 20, 2023
@Fyorl Fyorl added this to the Version 11 - Prototype 2 milestone Jan 20, 2023
@mkahvi
Copy link

mkahvi commented Jan 20, 2023

Here's some work in progress stuff that this was noticed with...

Starting point is an item model referring to another datamodel in array field:

actions: new fields.ArrayField(new fields.EmbeddedDataField(ActionModel), { required: false }),

Ref: https://gitlab.com/mkahvi/foundryvtt-pathfinder1/-/blob/54c14b0627663ce6b1f488bccaa76f5264f79147/module/data-model/item/consumable-model.mjs#L42

That datamodel then has schemafield for another datamodel:

      damage: new fields.SchemaField(
        {
          parts: new fields.ArrayField(new fields.EmbeddedDataField(DamageModel), { required: false }),
          critParts: new fields.ArrayField(new fields.EmbeddedDataField(DamageModel), { required: false }),
          nonCritParts: new fields.ArrayField(new fields.EmbeddedDataField(DamageModel), { required: false }),
        },
        { required: false }
      ),

Ref: https://gitlab.com/mkahvi/foundryvtt-pathfinder1/-/blob/54c14b0627663ce6b1f488bccaa76f5264f79147/module/data-model/common/action-model.mjs#L87-94

Which then refers to DamageModel:
Ref: https://gitlab.com/mkahvi/foundryvtt-pathfinder1/-/blob/datamodel/module/data-model/common/damage-model.mjs

Adding migrateData to the DamageModel with console logging, that logging never fired. It did not in fact even trigger if the actionmodel had the migrateData.

However recreating all this in minimal datamodel and experimenting in dev tools console alone did not produce the same results.

@aaclayton aaclayton self-assigned this Mar 22, 2023
@aaclayton aaclayton changed the title DataModel.migrateData is not run on nested EmbeddedDataFields Allow DataModel.migrateData to be called recursively for models which are not direct children of the root model schema. Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues related to the API used by Mod Devs bug Functionality which is not working as intended data-models Issues related to data models and schema changes
Projects
Status: Done
Development

No branches or pull requests

3 participants