fix: watch attributes only run getters/setters for the method they define

CHANGELOG.md

@@ -664,4 +664,8 @@ All notable changes to this project will be documented in this file. Breaking ch
 
 ## [3.9.0]
 ### Added
-- Added a new `returnOnConditionCheckFailure` execution option for write operations. Instead of throwing when a condition expression fails, the call resolves with `{ rejected: true }`. Pass `true` to get just the rejection flag, or `"all_old"` to also receive the existing item under `data`. The `"all_old"` mode requires AWS SDK v3, or AWS SDK v2 >= `2.1408.0`.
+- Added a new `returnOnConditionCheckFailure` execution option for write operations. Instead of throwing when a condition expression fails, the call resolves with `{ rejected: true }`. Pass `true` to get just the rejection flag, or `"all_old"` to also receive the existing item under `data`. The `"all_old"` mode requires AWS SDK v3, or AWS SDK v2 >= `2.1408.0`.
+
+## [3.9.1]
+### Fixed
+- [Issue #578](https://github.com/tywalch/electrodb/issues/578); Watchers no longer do unnecessary work for methods they don't define: a watcher only runs on `get` if it has a getter, and on `set` if it has a setter. This also fixes a setter-only `watch` attribute (e.g. an `updatedAt` timestamp) being returned as `undefined` on reads when absent from the item.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "electrodb",
-  "version": "3.9.0",
+  "version": "3.9.1",
   "description": "A library to more easily create and interact with multiple entities and heretical relationships in dynamodb",
   "main": "index.js",
   "scripts": {

src/schema.js

@@ -147,9 +147,17 @@ class Attribute {
     this.parent = { parentType: this.type, parentPath: this.path };
     this.get = this._makeGet(definition.get);
     this.set = this._makeSet(definition.set);
+    this._hasMutationHandler = {
+      [AttributeMutationMethods.get]: typeof definition.get === "function",
+      [AttributeMutationMethods.set]: typeof definition.set === "function",
+    };
     this.getClient = definition.getClient;
   }
 
+  hasMutationHandler(method) {
+    return !!this._hasMutationHandler[method];
+  }
+
   static buildChildAttributes(type, definition, parent) {
     let items;
     let properties;
@@ -1601,28 +1609,38 @@ class Schema {
   _validateProperties() {}
 
   _formatWatchTranslations(attributes) {
-    let watchersToAttributes = {};
-    let attributesToWatchers = {};
-    let watchAllAttributes = {};
-    let hasWatchers = false;
-    for (let name of Object.keys(attributes)) {
-      if (attributes[name].isWatcher()) {
-        hasWatchers = true;
-        watchersToAttributes[name] = attributes[name].watching;
-      } else if (attributes[name].watchAll) {
-        hasWatchers = true;
-        watchAllAttributes[name] = name;
-      } else {
-        attributesToWatchers[name] = attributesToWatchers[name] || {};
-        attributesToWatchers[name] = attributes[name].watchedBy;
+    const translations = {};
+    for (let method of Object.values(AttributeMutationMethods)) {
+      let watchersToAttributes = {};
+      let attributesToWatchers = {};
+      let watchAllAttributes = {};
+      let hasWatchers = false;
+      for (let name of Object.keys(attributes)) {
+        const attribute = attributes[name];
+        const hasMutationHandler = attribute.hasMutationHandler(method);
+        if (attribute.isWatcher() && hasMutationHandler) {
+          hasWatchers = true;
+          watchersToAttributes[name] = attribute.watching;
+        } else if (attribute.watchAll && hasMutationHandler) {
+          hasWatchers = true;
+          watchAllAttributes[name] = name;
+        } else {
+          attributesToWatchers[name] = {};
+          for (let watcher of Object.keys(attribute.watchedBy)) {
+            if (attributes[watcher] && attributes[watcher].hasMutationHandler(method)) {
+              attributesToWatchers[name][watcher] = watcher;
+            }
+          }
+        }
       }
+      translations[method] = {
+        hasWatchers,
+        watchAllAttributes,
+        watchersToAttributes,
+        attributesToWatchers,
+      };
     }
-    return {
-      hasWatchers,
-      watchAllAttributes,
-      watchersToAttributes,
-      attributesToWatchers,
-    };
+    return translations;
   }
 
   getAttribute(path) {
@@ -1667,21 +1685,21 @@ class Schema {
 
   _fulfillAttributeMutationMethod(method, payload) {
     let watchersToTrigger = {};
+    const translation = this.translationForWatching[method];
     // include: payload               | We want to hit the getters/setters for any attributes coming in to be changed
     // avoid: watchersToAttributes    | We want to avoid anything that is a watcher, even if it was included
     let avoid = {
-      ...this.translationForWatching.watchersToAttributes,
-      ...this.translationForWatching.watchAllAttributes,
+      ...translation.watchersToAttributes,
+      ...translation.watchAllAttributes,
     };
     let data = this._applyAttributeMutation(method, payload, avoid, payload);
     // `data` here will include all the original payload values, but with the mutations applied to on non-watchers
-    if (!this.translationForWatching.hasWatchers) {
+    if (!translation.hasWatchers) {
       // exit early, why not
       return data;
     }
     for (let attribute of Object.keys(data)) {
-      let watchers =
-        this.translationForWatching.attributesToWatchers[attribute];
+      let watchers = translation.attributesToWatchers[attribute];
       // Any of the attributes on data have a watcher?
       if (watchers !== undefined) {
         watchersToTrigger = { ...watchersToTrigger, ...watchers };
@@ -1693,12 +1711,12 @@ class Schema {
     let include = {
       ...data,
       ...watchersToTrigger,
-      ...this.translationForWatching.watchAllAttributes,
+      ...translation.watchAllAttributes,
     };
     return this._applyAttributeMutation(
       method,
       include,
-      this.translationForWatching.attributesToWatchers,
+      translation.attributesToWatchers,
       data,
     );
   }

test/offline.entity.spec.js

@@ -9011,5 +9011,166 @@ describe("Entity", () => {
       }, { ignoreOwnership: false });
       expect(result.data).to.deep.equal({ id: "123", name: "test", description: "desc" });
     });
+
+    describe("with watch attributes", () => {
+      const makeWatcherEntity = (updatedAt) =>
+        new Entity(
+          {
+            model: {
+              entity: "watcherItem",
+              service: "Playground",
+              version: "1",
+            },
+            attributes: {
+              id: { type: "string", required: true },
+              name: { type: "string", required: true },
+              updatedAt,
+            },
+            indexes: {
+              byId: {
+                pk: { field: "pk", composite: ["id"] },
+                sk: { field: "sk", composite: ["id"] },
+              },
+            },
+          },
+          { table: "electro" },
+        );
+
+      const pk = "$playground#id_123";
+      const sk = "$watcheritem_1#id_123";
+
+      it("should not add a watch-all attribute that only defines a setter when it is absent from the item", () => {
+        const entity = makeWatcherEntity({
+          type: "number",
+          watch: "*",
+          set: () => 1234567890,
+          readOnly: true,
+        });
+
+        const result = entity.parse({
+          Item: { pk, sk, id: "123", name: "test" },
+        });
+
+        expect(result.data).to.deep.equal({ id: "123", name: "test" });
+        expect(result.data).to.not.have.property("updatedAt");
+      });
+
+      it("should still return a setter-only watch-all attribute when it is present on the item", () => {
+        const entity = makeWatcherEntity({
+          type: "number",
+          watch: "*",
+          set: () => 1234567890,
+        });
+
+        const result = entity.parse({
+          Item: { pk, sk, id: "123", name: "test", updatedAt: 999 },
+        });
+
+        expect(result.data).to.deep.equal({
+          id: "123",
+          name: "test",
+          updatedAt: 999,
+        });
+      });
+
+      it("should still generate a setter-only watch-all attribute on the write path", () => {
+        const entity = makeWatcherEntity({
+          type: "number",
+          watch: "*",
+          set: () => 1234567890,
+          readOnly: true,
+        });
+
+        const params = entity.create({ id: "123", name: "test" }).params();
+
+        expect(params.Item.updatedAt).to.equal(1234567890);
+      });
+
+      it("should still compute a getter-only watch-all attribute on read when it is absent from the item", () => {
+        const entity = makeWatcherEntity({
+          type: "string",
+          watch: "*",
+          get: (_value, { id }) => `computed-${id}`,
+        });
+
+        const result = entity.parse({
+          Item: { pk, sk, id: "123", name: "test" },
+        });
+
+        expect(result.data).to.deep.equal({
+          id: "123",
+          name: "test",
+          updatedAt: "computed-123",
+        });
+      });
+
+      it("should run the getter of a watch-all attribute that defines both a getter and a setter on read", () => {
+        const entity = makeWatcherEntity({
+          type: "string",
+          watch: "*",
+          get: (_value, { id }) => `computed-${id}`,
+          set: () => "set-value",
+        });
+
+        const result = entity.parse({
+          Item: { pk, sk, id: "123", name: "test" },
+        });
+
+        expect(result.data).to.deep.equal({
+          id: "123",
+          name: "test",
+          updatedAt: "computed-123",
+        });
+      });
+
+      it("should still trigger a getter on a named (non-watch-all) watcher on read", () => {
+        const entity = makeWatcherEntity({
+          type: "string",
+          watch: ["name"],
+          get: (_value, { name }) => `named-${name}`,
+        });
+
+        const result = entity.parse({
+          Item: { pk, sk, id: "123", name: "test" },
+        });
+
+        expect(result.data).to.deep.equal({
+          id: "123",
+          name: "test",
+          updatedAt: "named-test",
+        });
+      });
+
+      it("should not add a named (non-watch-all) setter-only watcher when it is absent from the item", () => {
+        const entity = makeWatcherEntity({
+          type: "string",
+          watch: ["name"],
+          set: () => "set-value",
+        });
+
+        const result = entity.parse({
+          Item: { pk, sk, id: "123", name: "test" },
+        });
+
+        expect(result.data).to.deep.equal({ id: "123", name: "test" });
+        expect(result.data).to.not.have.property("updatedAt");
+      });
+
+      it("should exclude a setter-only watch-all attribute when specific attributes are requested", () => {
+        const entity = makeWatcherEntity({
+          type: "number",
+          watch: "*",
+          set: () => 1234567890,
+        });
+
+        const result = entity.parse(
+          { Item: { pk, sk, id: "123", name: "test", updatedAt: 999 } },
+          { attributes: ["id"] },
+        );
+
+        expect(result.data).to.deep.equal({ id: "123" });
+        expect(result.data).to.not.have.property("updatedAt");
+      });
+    });
   });
 });