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.

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

Conversation

elle-j
Copy link
Contributor

@elle-j elle-j commented Jan 5, 2024

What, How & Why?

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.

This PR updates it so that the individual operation is automatically rolled back.

This closes #2638.

It also sets up somewhat related tests (currently .skiped) that can be enabled once #6355 is fixed.

☑️ ToDos

  • 📝 Changelog entry
  • 🚦 Tests

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.

Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

I just have a small suggestion 👍 But this can merge as is if you want to.

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

Great fix!

@elle-j
Copy link
Contributor Author

elle-j commented Jan 8, 2024

As per our team discussion, to prevent the extra oplog changes with the rollback, but still make realm.create() only throw or create an object, I'll rework the PR in tandem with #6359 and #6355.

@elle-j elle-j marked this pull request as draft January 9, 2024 08:51
@kneth kneth removed their request for review March 12, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating objects while in a manual transaction, should either create an object or throw
3 participants