Roll back invalid object creations not handled by realm.write()

CHANGELOG.md

@@ -7,8 +7,10 @@
 * None
 
 ### Fixed
-* <How to hit and notice issue? what was the impact?> ([#????](https://github.com/realm/realm-js/issues/????), since v?.?.?)
-* None
+* Trying to create an invalid object (e.g. missing required properties) via `realm.create()` or a `Realm.Object` constructor would both throw and create the object (with default values for required properties), causing the object creation to not be rolled back in the following cases:
+  * A) During a manual transaction if the user does not explicitly catch the exception and cancel the transaction.
+  * B) If the user catches the exception within the callback passed to `realm.write()` and does not rethrow it.
+  * Fix: The operation is now automatically rolled back. ([#2638](https://github.com/realm/realm-js/issues/2638))
 
 ### Compatibility
 * React Native >= v0.71.4
@@ -23,7 +25,7 @@
 ## 12.5.1 (2024-01-03)
 
 ### Fixed
-* Accessing the `providerType` on a `UserIdentity` via `User.identities` always yielded `undefined`. Thanks to [@joelowry96](https://github.com/joelowry96) for pinpointing the fix.
+* Accessing the `providerType` on a `UserIdentity` via `User.identities` always yielded `undefined`. Thanks to [@joelowry96](https://github.com/joelowry96) for pinpointing the fix. ([#6248](https://github.com/realm/realm-js/issues/6248))
 * Bad performance of initial Sync download involving many backlinks. ([realm/realm-core#7217](https://github.com/realm/realm-core/issues/7217), since v10.0.0)
 
 ### Compatibility

integration-tests/tests/src/schemas/person-and-dogs.ts

@@ -76,3 +76,31 @@ export class Dog extends Realm.Object<Dog> {
 
   static schema: Realm.ObjectSchema = DogSchema;
 }
+
+export interface IPersonWithEmbedded {
+  name: string;
+  age: number;
+  address?: IEmbeddedAddress;
+}
+
+export const PersonWithEmbeddedSchema: Realm.ObjectSchema = {
+  name: "PersonWithEmbedded",
+  primaryKey: "name",
+  properties: {
+    age: "int",
+    name: "string",
+    address: "EmbeddedAddress?",
+  },
+};
+
+export interface IEmbeddedAddress {
+  street: string;
+}
+
+export const EmbeddedAddressSchema: Realm.ObjectSchema = {
+  name: "EmbeddedAddress",
+  embedded: true,
+  properties: {
+    street: "string",
+  },
+};

integration-tests/tests/src/tests/transaction.ts

@@ -16,12 +16,18 @@
 //
 ////////////////////////////////////////////////////////////////////////////
 import { expect } from "chai";
-import { openRealmBeforeEach } from "../hooks";
+import { UpdateMode } from "realm";
 
-import { PersonSchema } from "../schemas/person-and-dogs";
+import { openRealmBeforeEach } from "../hooks";
+import {
+  EmbeddedAddressSchema,
+  IPersonWithEmbedded,
+  PersonSchema,
+  PersonWithEmbeddedSchema,
+} from "../schemas/person-and-dogs";
 
 describe("Realm transactions", () => {
-  openRealmBeforeEach({ schema: [PersonSchema] });
+  openRealmBeforeEach({ schema: [PersonSchema, PersonWithEmbeddedSchema, EmbeddedAddressSchema] });
 
   describe("Manual transactions", () => {
     it("can perform a manual transaction", function (this: RealmContext) {
@@ -69,8 +75,7 @@ describe("Realm transactions", () => {
         realm.commitTransaction(); // We don't expect this to be called
       }).throws("Expected value to be a number or bigint, got a string");
 
-      // TODO: Fix 👇 ... its a bit surprising that an object gets created at all
-      expect(persons.length).equals(1);
+      expect(persons.length).equals(0);
       expect(realm.isInTransaction).to.be.true;
 
       realm.cancelTransaction();
@@ -85,4 +90,125 @@ describe("Realm transactions", () => {
       expect(!this.realm.isInTransaction).to.be.true;
     });
   });
+
+  describe("Transactions via realm.write()", () => {
+    it("`realm.create()` does not create an object if it throws", function (this: Mocha.Context & RealmContext) {
+      this.realm.write(() => {
+        // It is important to catch the exception within `realm.write()` in order to test
+        // that the object creation path does not create the object (rather than being due
+        // to `realm.write()` cancelling the transaction).
+        expect(() => {
+          const invalidPerson = { name: "Amy" };
+          this.realm.create(PersonWithEmbeddedSchema.name, invalidPerson);
+        }).to.throw("Missing value for property 'age'");
+      });
+      expect(this.realm.objects(PersonWithEmbeddedSchema.name).length).equals(0);
+    });
+
+    it("`realm.create()` does not create an object if having an invalid embedded object", function (this: Mocha.Context &
+      RealmContext) {
+      this.realm.write(() => {
+        expect(() => {
+          const invalidEmbeddedAddress = {};
+          this.realm.create(PersonWithEmbeddedSchema.name, {
+            name: "Amy",
+            age: 30,
+            address: invalidEmbeddedAddress,
+          });
+        }).to.throw("Missing value for property 'street'");
+      });
+      expect(this.realm.objects(PersonWithEmbeddedSchema.name).length).equals(0);
+    });
+
+    it("commits successful operations if exceptions from failed ones are caught", function (this: Mocha.Context &
+      RealmContext) {
+      this.realm.write(() => {
+        this.realm.create(PersonWithEmbeddedSchema.name, { name: "John", age: 30 });
+        expect(() => {
+          const invalidPerson = { name: "Amy" };
+          this.realm.create(PersonWithEmbeddedSchema.name, invalidPerson);
+        }).to.throw("Missing value for property 'age'");
+      });
+      const objects = this.realm.objects<IPersonWithEmbedded>(PersonWithEmbeddedSchema.name);
+      expect(objects.length).equals(1);
+      expect(objects[0].name).equals("John");
+    });
+
+    it("does not commit the transaction if any operation that throws is not caught", function (this: Mocha.Context &
+      RealmContext) {
+      expect(() => {
+        this.realm.write(() => {
+          this.realm.create(PersonWithEmbeddedSchema.name, { name: "John", age: 30 });
+          // Don't catch any exceptions within `realm.write()`.
+          const invalidPerson = { name: "Amy" };
+          this.realm.create(PersonWithEmbeddedSchema.name, invalidPerson);
+        });
+      }).to.throw("Missing value for property 'age'");
+      expect(this.realm.objects(PersonWithEmbeddedSchema.name).length).equals(0);
+    });
+
+    // TODO: Enable when fixing this issue: https://github.com/realm/realm-js/issues/6355
+    it.skip("does not modify an embedded object if resetting it to an invalid one via a setter", function (this: Mocha.Context &
+      RealmContext) {
+      const amy = this.realm.write(() => {
+        return this.realm.create(PersonWithEmbeddedSchema.name, {
+          name: "Amy",
+          age: 30,
+          address: { street: "Broadway" },
+        });
+      });
+      expect(this.realm.objects(PersonWithEmbeddedSchema.name).length).equals(1);
+      expect(amy.address?.street).equals("Broadway");
+
+      this.realm.write(() => {
+        // It is important to catch the exception within `realm.write()` in order to test
+        // that the object creation path does not modify the object (rather than being due
+        // to `realm.write()` cancelling the transaction).
+        expect(() => {
+          const invalidEmbeddedAddress = {};
+          // @ts-expect-error Testing setting invalid type.
+          amy.address = invalidEmbeddedAddress;
+        }).to.throw("Missing value for property 'street'");
+      });
+      const objects = this.realm.objects<IPersonWithEmbedded>(PersonWithEmbeddedSchema.name);
+      expect(objects.length).equals(1);
+      expect(objects[0].address).to.not.be.null;
+      expect(objects[0].address?.street).equals("Broadway");
+    });
+
+    // TODO: Enable when fixing this issue: https://github.com/realm/realm-js/issues/6355
+    it.skip("does not modify an embedded object if resetting it to an invalid one via UpdateMode", function (this: Mocha.Context &
+      RealmContext) {
+      const amy = this.realm.write(() => {
+        return this.realm.create(PersonWithEmbeddedSchema.name, {
+          name: "Amy",
+          age: 30,
+          address: { street: "Broadway" },
+        });
+      });
+      expect(this.realm.objects(PersonWithEmbeddedSchema.name).length).equals(1);
+      expect(amy.address?.street).equals("Broadway");
+
+      this.realm.write(() => {
+        // It is important to catch the exception within `realm.write()` in order to test
+        // that the object creation path does not modify the object (rather than being due
+        // to `realm.write()` cancelling the transaction).
+        expect(() => {
+          const invalidEmbeddedAddress = {};
+          this.realm.create(
+            PersonWithEmbeddedSchema.name,
+            {
+              name: "Amy",
+              address: invalidEmbeddedAddress,
+            },
+            UpdateMode.Modified,
+          );
+        }).to.throw("Missing value for property 'street'");
+      });
+      const objects = this.realm.objects<IPersonWithEmbedded>(PersonWithEmbeddedSchema.name);
+      expect(objects.length).equals(1);
+      expect(objects[0].address).to.not.be.null;
+      expect(objects[0].address?.street).equals("Broadway");
+    });
+  });
 });

packages/realm/src/Object.ts

@@ -66,7 +66,7 @@
 }
 
 /** @internal */
-export type ObjCreator = () => [binding.Obj, boolean];
+export type ObjCreator = () => [binding.Obj, boolean, binding.TableRef?];
 
 type CreationContext = {
   helpers: ClassHelpers;
@@ -141,7 +141,7 @@
  * }
  * ```
  * @see {@link ObjectSchema}
  * @typeParam `T` - The type of this class (e.g. if your class is `Person`,
  * `T` should also be `Person` - this duplication is required due to how
  * TypeScript works)
  * @typeParam `RequiredProperties` - The names of any properties of this
@@ -213,27 +213,27 @@
     } = context;
 
     // Create the underlying object
-    const [obj, created] = createObj ? createObj() : this.createObj(realm, values, mode, context);
-    const result = wrapObject(obj);
-    assert(result);
-    // Persist any values provided
-    // TODO: Consider using the property helpers directly to improve performance
-    for (const property of persistedProperties) {
-      const propertyName = property.publicName || property.name;
-      const { default: defaultValue } = properties.get(propertyName);
-      if (property.isPrimary) {
-        continue; // Skip setting this, as we already provided it on object creation
-      }
-      const propertyValue = values[propertyName];
-      if (typeof propertyValue !== "undefined") {
-        if (mode !== UpdateMode.Modified || result[propertyName] !== propertyValue) {
-          // This will call into the property setter in PropertyHelpers.ts.
-          // (E.g. the setter for [binding.PropertyType.Array] in the case of lists.)
-          result[propertyName] = propertyValue;
+    const [obj, created, table] = createObj ? createObj() : this.createObj(realm, values, mode, context);
+    try {
+      const result = wrapObject(obj);
+      assert(result);
+      // Persist any values provided
+      // TODO: Consider using the property helpers directly to improve performance
+      for (const property of persistedProperties) {
+        const propertyName = property.publicName || property.name;
+        const { default: defaultValue } = properties.get(propertyName);
+        if (property.isPrimary) {
+          continue; // Skip setting this, as we already provided it on object creation
         }
-      } else {
-        if (created) {
-          if (typeof defaultValue !== "undefined") {
+        const propertyValue = values[propertyName];
+        if (propertyValue !== undefined) {
+          if (mode !== UpdateMode.Modified || result[propertyName] !== propertyValue) {
+            // This will call into the property setter in PropertyHelpers.ts.
+            // (E.g. the setter for [binding.PropertyType.Array] in the case of lists.)
+            result[propertyName] = propertyValue;
+          }
+        } else if (created) {
+          if (defaultValue !== undefined) {
             result[propertyName] = typeof defaultValue === "function" ? defaultValue() : defaultValue;
           } else if (
             !(property.type & binding.PropertyType.Collection) &&
@@ -243,8 +243,24 @@
           }
         }
       }
+      return result as RealmObject;
+    } catch (err) {
+      // Currently, `table` will only be defined for non-embedded objects. Invalid embedded
+      // objects created on a parent as part of `realm.create()` will still be removed through
+      // cascaded delete in Core. However, an invalid embedded object created by only setting
+      // a property (not through `realm.create()`) will not enter this if-block and be removed.
+      // We could remove the check for `table` then get it via `binding.Helpers.getTable(..)`,
+      // but removing the embedded object in this case would cause the parent's embedded object
+      // field to be set to `null` (overwriting the previous value). Not removing the object
+      // (as in the current implementation) instead causes Core to overwrite the values of
+      // the embedded object with default values. That is the same behavior as before this
+      // commit. Ideally, the valid embedded object should remain unchanged, see issue:
+      // https://github.com/realm/realm-js/issues/6355
+      if (created && table) {
+        table.removeObject(obj.key);
+      }
+      throw err;
     }
-    return result as RealmObject;
   }
 
   /**
@@ -256,7 +272,7 @@
     values: DefaultObject,
     mode: UpdateMode,
     context: CreationContext,
-  ): [binding.Obj, boolean] {
+  ): [binding.Obj, boolean, binding.TableRef] {
     const {
       helpers: {
         objectSchema: { name, tableKey, primaryKey },
@@ -283,16 +299,15 @@
           : primaryKeyHelpers.default,
       );
 
-      const result = binding.Helpers.getOrCreateObjectWithPrimaryKey(table, pk);
-      const [, created] = result;
+      const [obj, created] = binding.Helpers.getOrCreateObjectWithPrimaryKey(table, pk);
       if (mode === UpdateMode.Never && !created) {
         throw new Error(
           `Attempting to create an object of type '${name}' with an existing primary key value '${primaryKeyValue}'.`,
         );
       }
-      return result;
+      return [obj, created, table];
     } else {
-      return [table.createObject(), true];
+      return [table.createObject(), true, table];
     }
   }
 

packages/realm/src/Realm.ts

@@ -847,7 +847,6 @@
   }
 
   // TODO: Support embedded objects
-  // TODO: Rollback by deleting the object if any property assignment fails (fixing #2638)
   /**
    * Create a new {@link RealmObject} of the given type and with the specified properties. For objects marked asymmetric,
    * `undefined` is returned. The API for asymmetric objects is subject to changes in the future.
@@ -906,7 +905,7 @@
    * Deletes the provided Realm object, or each one inside the provided collection.
    * @param subject - The Realm object to delete, or a collection containing multiple Realm objects to delete.
    */
   delete(subject: AnyRealmObject | AnyRealmObject[] | AnyList | AnyResults | any): void {
     assert.inTransaction(this, "Can only delete objects within a transaction.");
     assert.object(subject, "subject");
     if (subject instanceof RealmObject) {
@@ -939,7 +938,7 @@
 
   /**
    * Deletes a Realm model, including all of its objects.
    * If called outside a migration function, {@link schema} and {@link schemaVersion} are updated.
    * @param name - The model name.
    */
   deleteModel(name: string): void {
@@ -1102,7 +1101,7 @@
    * Remove the listener {@link callback} for the specified event {@link eventName}.
    * @param eventName - The event name.
    * @param callback - Function that was previously added as a listener for this event through the {@link addListener} method.
    * @throws an {@link Error} If an invalid event {@link eventName} is supplied, if Realm is closed or if {@link callback} is not a function.
    */
   removeListener(eventName: RealmEventName, callback: RealmListenerCallback): void {
     assert.open(this);
@@ -1146,12 +1145,15 @@
   }
 
   /**
    * Synchronously call the provided {@link callback} inside a write transaction. If an exception happens inside a transaction,
    * you’ll lose the changes in that transaction, but the Realm itself won’t be affected (or corrupted).
    * More precisely, {@link beginTransaction} and {@link commitTransaction} will be called
    * automatically. If any exception is thrown during the transaction {@link cancelTransaction} will
    * be called instead of {@link commitTransaction} and the exception will be re-thrown to the caller of {@link write}.
    *
+   * Note that if you choose to catch an exception within the callback and not rethrow it, the transaction
+   * will not be cancelled. However, the individual operation that failed will be rolled back automatically.
+   *
    * Nested transactions (calling {@link write} within {@link write}) is not possible.
    * @param callback - Function to be called inside a write transaction.
    * @returns Returned value from the callback.