foundryvtt icon indicating copy to clipboard operation
foundryvtt copied to clipboard

Passing a BaseFolder instance in Actor#update fails validation

Open Eranziel opened this issue 3 years ago • 4 comments

What happened?

Attempting to update an Actor's folder by passing a BaseFolder instance fails validation. The validation for the ForeignDocumentField complains that it must be given a BaseFolder instance, but the instance is flattened to a simple Object earlier in the call stack.

In foundry.js line 12651:

      // Retrieve the change object
      let changes;
      if ( update instanceof foundry.abstract.DataModel ) changes = update.toObject();
      else changes = foundry.utils.expandObject(update);  // ED: Here the BaseFolder passed in update is flattened to an Object.
      changes = cls.migrateData(changes);

      // other stuff not relevant

      // Clean and validate the proposed changes
      try {
        doc.validate({changes, clean: true, strict: true, fallback: false});  // ED: Object instead of BaseFolder is passed to validate
      } catch(err) {
        ui.notifications.error(err.message.split("] ").pop());
        Hooks.onError("ClientDatabaseBackend#_preUpdateDocumentArray", err, {id: doc.id, log: "error"});
        continue;
      }

In ForeignDocumentField:

    _cast(value) {
      if ( typeof value === "string" ) return value;
      if ( (value instanceof this.model) ) return value._id;
      // This is where the error is thrown. On the previous line, value is not an instance of BaseFolder.
      throw new Error(`The value provided to a ForeignDocumentField must be a ${this.model.name} instance.`);
    }

What ways of accessing Foundry can you encounter this issue in?

  • [X] Native App (Electron)
  • [X] Chrome
  • [ ] Firefox
  • [ ] Safari
  • [ ] Other

Reproduction Steps

The following works in Simple Worldbuilding, Dnd5e, and the dev branch of Lancer. I'm confident the bug exists in Foundry core and is not system specific.

  • Create a folder
  • Create an Actor inside that folder
  • In console:
    let a = game.actors.get(id);
    a.update({"folder": a.folder});
    

What core version are you reporting this for?

10.288

Relevant log output

let a = game.actors.get("qJm9W1wrH43iKqta");
a.update({"folder": a.folder})
foundry.js:58721 The value provided to a ForeignDocumentField must be a BaseFolder instance.
fetch @ foundry.js:58721
notify @ foundry.js:58648
error @ foundry.js:58684
_preUpdateDocumentArray @ foundry.js:12669
_updateDocuments @ foundry.js:12583
update @ commons.js:6771
await in update (async)
updateDocuments @ commons.js:6128
update @ commons.js:6225
update @ lancer-actor.ts:1215
(anonymous) @ VM469:1
foundry.js:747 Error: The value provided to a ForeignDocumentField must be a BaseFolder instance.
    at ForeignDocumentField._cast (commons.js:4723:13)
    at ForeignDocumentField.clean (commons.js:3757:20)
    at ForeignDocumentField.clean (commons.js:4299:20)
    at SchemaField._cleanType (commons.js:4028:28)
    at SchemaField.clean (commons.js:3760:19)
    at LancerActor.cleanData (commons.js:5368:26)
    at LancerActor.validate (commons.js:5466:48)
    at ClientDatabaseBackend._preUpdateDocumentArray (foundry.js:12667:13)
    at ClientDatabaseBackend._updateDocuments (foundry.js:12583:33)
    at ClientDatabaseBackend.update (commons.js:6771:24)
onError @ foundry.js:747
_preUpdateDocumentArray @ foundry.js:12670
_updateDocuments @ foundry.js:12583
update @ commons.js:6771
await in update (async)
updateDocuments @ commons.js:6128
update @ commons.js:6225
update @ lancer-actor.ts:1215
(anonymous) @ VM469:1
Promise {<fulfilled>: undefined}

Bug Checklist

  • [X] The issue occurs while all Modules are disabled

Eranziel avatar Oct 28 '22 04:10 Eranziel

Attempting similar updates with an Item or Journal produces the same error. Presumably any other document which can be organized in folders as well, but I haven't exhaustively tested.

Eranziel avatar Oct 28 '22 04:10 Eranziel

Yes, you must pass the folder ID directly in the update, for the same reasons that you cannot update SetFields with actual Sets and must instead provide an array, see here: #7706

Fyorl avatar Oct 28 '22 11:10 Fyorl

Should the error message be amended then? It seems confusing that it "must be a Base folder" when in fact it must not be a Base folder. (I'm mostly just confused about when would that error be true? As to my knowledge _cast is only used for fixing incoming .update data...)

whitespine avatar Oct 28 '22 16:10 whitespine

The error message is correct with regards to ForeignDocumentField validation, it's just unfortunately misleading in this case. Even though the caller is passing a BaseFolder as part of update, what arrives at ForeignDocumentField#_cast is not a BaseFolder (through no fault of the caller, it is clobbered as part of the update process). DataModels can be used and validated entirely independently of CRUD operations, they are not coupled to that particular workflow.

Fyorl avatar Oct 28 '22 16:10 Fyorl