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.

Sign up for GitHub

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

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e039a5
Roll back object creation if it throws.
elle-j Jan 5, 2024
e2ffd9a
Add tests.
elle-j Jan 5, 2024
c6c618d
Update docs.
elle-j Jan 5, 2024
c475132
Add CHANGELOG entry.
elle-j Jan 5, 2024
527662c
Update comments.
elle-j Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Roll back object creation if it throws.
  • Loading branch information
@elle-j
elle-j committed Jan 5, 2024
commit 5e039a5d71feea0fcb93c6fcb1ef003c6aedd7f7
69 changes: 42 additions & 27 deletions packages/realm/src/Object.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export enum UpdateMode {
}

/** @internal */
export type ObjCreator = () => [binding.Obj, boolean];
export type ObjCreator = () => [binding.Obj, boolean, binding.TableRef?];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that it's now 3 values, we could choose to return an object instead to clarify the return values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is approaching (if its not already in) the territory that needs an object 👍

In this specific case, we might not need to pass the TableRef here at all, but we could instead pass a "rollback", "revert" or "delete" function, taking no arguments, which will call table.removeObject(obj.key).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the TableRef really optional? Maybe i'm overlooking something, but the creator seems to always return a TableRef.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..but we could instead pass a "rollback", "revert" or "delete" function, taking no arguments..

Interesting, yeah that seems more appropriate given that that's the only reason for passing the table 🙂

Is the TableRef really optional?

The creator defined on the Object class will always return the table, but for embedded objects, a custom createObj() method will be passed as part of the CreationContext. As those parts have not been modified, the table ref needed to be optional in this case.


type CreationContext = {
helpers: ClassHelpers;
Expand Down Expand Up @@ -213,27 +213,27 @@ export class RealmObject<T = DefaultObject, RequiredProperties extends keyof Omi
} = 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) &&
Expand All @@ -243,8 +243,24 @@ export class RealmObject<T = DefaultObject, RequiredProperties extends keyof Omi
}
}
}
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 parent's embedded object
// field to be set to `null` (overriding 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;
}

/**
Expand All @@ -256,7 +272,7 @@ export class RealmObject<T = DefaultObject, RequiredProperties extends keyof Omi
values: DefaultObject,
mode: UpdateMode,
context: CreationContext,
): [binding.Obj, boolean] {
): [binding.Obj, boolean, binding.TableRef] {
const {
helpers: {
objectSchema: { name, tableKey, primaryKey },
Expand All @@ -283,16 +299,15 @@ export class RealmObject<T = DefaultObject, RequiredProperties extends keyof Omi
: 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];
}
}

Expand Down