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

Fix setting an object with an embedded object property to null #6295

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

elle-j
Copy link
Contributor

@elle-j elle-j commented Dec 1, 2023

What, How & Why?

Updating an object's property whose value was an embedded object did not call into Core to update the underlying object when the new value was null or undefined.

This closes #6280

☑️ ToDos

  • 📝 Changelog entry
  • 🚦 Tests

Copy link

coveralls-official bot commented Dec 1, 2023

Coverage Status

coverage: 86.193% (+0.01%) from 86.182%
when pulling 38656a9 on lj/embedded-null-value
into 7c92ac8 on main.

Comment on lines 104 to 111
// Asking for the toBinding will create the object and link it to the parent in one operation.
// Thus, no need to actually set the value on the `obj` unless it's an optional null value.
const bindingValue = toBinding(value, { createObj: () => [obj.createAndSetLinkedObject(columnKey), true] });
// No need to destructure `optional` and check that it's `true` in this condition
// as trying to set a non-optional value to null will fail at an earlier stage.
if (bindingValue === null) {
obj.setAny(columnKey, bindingValue);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The call to toBinding() will in turn call nullPassthrough(), resulting in toBinding() returning null without calling into Core. For other non-null and non-undefined values, the embedded object will be created during that call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's a good catch!

Comment on lines 104 to 111
// Asking for the toBinding will create the object and link it to the parent in one operation.
// Thus, no need to actually set the value on the `obj` unless it's an optional null value.
const bindingValue = toBinding(value, { createObj: () => [obj.createAndSetLinkedObject(columnKey), true] });
// No need to destructure `optional` and check that it's `true` in this condition
// as trying to set a non-optional value to null will fail at an earlier stage.
if (bindingValue === null) {
obj.setAny(columnKey, bindingValue);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's a good catch!

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.

LGTM!

@elle-j elle-j merged commit 3fa8b6b into main Dec 5, 2023
34 checks passed
@elle-j elle-j deleted the lj/embedded-null-value branch December 5, 2023 09:00
bimusiek pushed a commit to bimusiek/realm-js that referenced this pull request Mar 14, 2024
…lm#6295)

* Update DB obj when embedded prop is set to null.

* Add tests.

* Add CHANGELOG entry.

* Update inline comment.

* Add tests using 'UpdateMode'.

* Extract common logic to separate function to make the tests more focused.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nullable embedded schema behaves differently on Realm 12 and 11
4 participants