Astronomy sets all missing fields as modified when saving specific fields

[arggh]

I had a feeling this had been reported and fixed before, but I couldn't find any relevant issues. I noticed today that isModified inside event hooks returns true for all document's fields missing, even though the fields option was used while saving, specifying only a single field to be saved.

What happens:

A field that is not being saved gets marked as modified.

How it should work (IMHO):

If a field is not being saved should not be marked as modified (inside an onUpdate hook).

Example

Let's say I have an event hook like so:

...
beforeUpdate: [
  function preventNameChanges (e) {
    if (!e.trusted) {
      const doc = e.currentTarget;
      if (doc.isModified('name')) {
        throw new Meteor.Error(
          'not-allowed',
          'Name changes not allowed from client.'
        );
      }
    }
  }
],
...

...and then, on the client side, I have this code running:

const doc = Doc.findOne({ _id: id }, { fields: { date: 1 } });
doc.date = new Date();
doc.save({ fields: ['date'] });

...that Name changes not allowed from client-error will be thrown, because doc.isModified('name') will return true because it's missing entirely, even though I'm only trying to save field date.

[lukejagodzinski]

@arggh could you create minimal reproduction repository? It will be easier for me to test and fix

[arggh]

Here you go: https://github.com/arggh/astronomy-issue-694

Just run it with meteor, open browser at localhost:3000 and click the only button on page to trigger the error thrown.

[lukejagodzinski]

Yes that might be an issue. Overall, the isModified method is not designed to work properly in such cases. I'm working on the workaround for that. After it's implemented it will be possible to do something like this:

if (e.fields && e.fields.includes("name") && doc.isModified("name")) {
}

Where e is an event object in the beforeUpdate event. It will also work with any other event.

[lukejagodzinski]

@arggh ok fixed in v 2.6.3 the way I described it above. Hope it fits your needs

[arggh]

Great! Ideally it would have just worked without the extra checks, but I understand that it would probably be a breaking change in how isModified works.

This will work and it's clear what the intention is, so it's all good. I think I'll wrap those checks inside a helper, so I can just call something like this:

if (isReallyModified(e, 'name')) {
...
}

Thanks @lukejagodzinski !

[lukejagodzinski]

Yep, to make it without extra checks would require a lot of rewriting and it would be a breaking change

[arggh]

Just a quick update, I have an Astronomy behavior that logs any changes to my documents. Here's the relevant parts I had to change to use the newly provided fields feature:

...
  const definition = {
      fields: {},
      events: {
        beforeUpdate: (e) => {
          if (Meteor.isServer && this.enabled('update', e)) {
            const doc = e.currentTarget;
-           const fieldsToUpdate = doc.getModified();
+           const modifiedOrMissingFields = doc.getModified();
+           const specifiedFields = e.fields;
+           const fieldsToUpdate = specifiedFields ?
+             modifiedOrMissingFields.filter(field => specifiedFields.includes(field)) :
+             modifiedOrMissingFields;
            const msg = this.options.updatedMessage(this.getClassName(e), fieldsToUpdate);
            this.log(msg, {
              detail: fieldsToUpdate,
              doc: doc
            });
          }
        },
        afterInsert: (e) => {
...

The new behavior adds a bit of overhead, but it works 👍

[arggh]

Maybe it would make sense to combine those 5 lines of code into a single function on the document? doc.getModified4Realz() or doc.getUpdated()?

[lukejagodzinski]

Maybe I could add an option to the getModified method allowing passing the list of fields? I think it would be much easier as documents are not aware of the context in which they're being invoked. So document in the beforeUpdate event doesn't know that it should treat the getModified method differently and I just don't want to override this behavior as it would not be backward compatible.

[arggh]

I think that would be even better than adding another method. Something like

doc.getModified({ onlyFieldsThatWillBeSaved: true });

...but with a better name for the option I guess!

[lukejagodzinski]

Implemented in 2.7.0. Here is description of changes: https://github.com/jagi/meteor-astronomy/releases/tag/2.7.0

[arggh]

Great, thank you!