opened
cmelchior
username_0
We already have `freeze` API's for all classes, but the reverse API `thaw()` has only been implemented directly in the  Cocoa SDK (https://github.com/realm/realm-cocoa/pull/6975).

The Kotlin SDK needs similar mechanisms for it to work, so this PR adds `freeze/thaw` to SharedRealm, Results, List and Object in both ObjectStore and the C-API.

While the `thaw()` API more or less directly mirrors the `import_copy()` API in Core, having it exposed in the higher-level semantics of ObjectStore makes it much easier to reason about and use, as well as allows for future optimizations.

TODO:
- [x] Understand why some methods in the C-API return `bool` and others have `out_<param>`. For now I followed the pattern already existing for `freeze()`
- [x] Add unit tests for ObjectStore and the C-API for the new methods

Regarding error reporting: All added functions returns pointers that follows the existing scheme in the C-API (AFIK)
Regarding unit tests: Added tests for the C-API. Don't know if we need explicit test for the ObjectStore API.
Add Freeze/Thaw API's to Realm/Results/List/Object.
issue
created
rorbech
username_1
@simonask I addressed your comments by adding some comments and aligning the asserts. However, I guess introducing asserts for the existing `freeze` on objects and results are technically breaking, though I don't think SDK will ever call these with a non-frozen realm. Is it safe enough to add?

Alternatively we could have done a generic `import_copy_of` so it was up to the SDK to keep track of frozen/life-state of the realms themselves and reasoning about the resulting object from that, but it would introduce another ffi-call if we would like to do similar asserts in the SDK.
comment
created
rorbech
username_1
I removed the asserts that caused a breaking API. Don't know if we should also remove the assert in the thawing methods too to make it symmetrical? 🤔

Only lacking thing is that it would be nice with a specific error/error code if we are trying to thaw an object that has been deleted. In Kotlin we are offering a `findLatest(object)` that we would like to return null in case the object is no longer needed. Our current implementation catches C-API errors and returns null, but we should only do this in the specific situation where the object is just not longer valid.  Given the latest discussion about error codes, this is maybe not for this PR.
comment
created
nirinchev
username_2
Okay, I saw your comment that thawing a deleted object will throw an error. I think this is wrong. That is not an unexpected/erroneous situation and we should not use exceptions to relay such information. I would propose that we align the C API to that of the SDK and rework these functions as `try_freeze` and `try_thaw` or whatever follows the C API conventions.
comment
created
rorbech
username_1
Simon was the one suggesting the specific error code when the object was deleted. Returning a null pointer is a bit problematic that this conventionally used to indicate the the api call might have caused an error that should be inspected through `realm_get_last_error`. Probably need somebody with more insight than me to decide on that.
comment
created
nirinchev
username_2
I don't see any SDK throwing an error from thaw, which makes it silly to have an API discrepancy between the C API and the SDK API. I believe the solution here would be to change the signature of `freeze/thaw` to something like:

```cpp
RLM_API bool realm_object_freeze(const realm_object_t* live_object, const realm_t* frozen_realm, realm_object_t* out_frozen_object);
```

Where the return value indicates whether the method succeeded or failed and `out_frozen_object` may contain a valid pointer or nullptr.

It's possible I'm missing some context here, but generally, it's a bad practice to use errors to handle expected scenarios.
comment
created
fealebenpae
username_3
I agree with Nikola. I think using error codes to signal the object no longer being there is too close to using exceptions for flow control for my taste. His suggestion about returning bool and using an out parameter seems sound to me.
comment
created
rorbech
username_1
I updated the C-API for the thaw-functions where the object or parent object can have been deleted, added test cases (and fixed the error in the implementation).
comment
created
fealebenpae
username_3
The changes in the C API look good, but I'm wondering whether the C++ `thaw()` methods have the correct behavior. Perhaps their signature should be something like `util::Optional<T> T::thaw(const std::shared_ptr<Realm>&)` to indicate that the object returned might not be valid? Otherwise we need to rely on what I think is implicit semantics on Object, List, and Results.
comment
created
rorbech
username_1
I discussed with @username_4 and just followed the pattern of the existing implementation. I don't see that freeze is returning an optional to signal that the object cannot be frozen in the given realm. Maybe somebody from core could take over as it seems like it requires more insight into the overall behaviour. Or we could just go with the current implementation and then add a follow up issue to align the internals. @username_3?
comment
created
fealebenpae
username_3
Optional-ness is sort of a general question I have about the API, I guess, and it's definitely not just about these new methods in this PR. I am happy to merge this as-is, and revising the API with explicit optionality in the type system is definitely something that can happen at a later point and shouldn't block this work.
comment
created
rorbech
username_1
@username_3 @username_2 I updated the C-API to return optional/null when trying to thaw deleted objects/lists. I don't see freezing deleted objects/list as a similar SDK use cases and also wouldn't be able change the API for chose without being a breaking change.
comment
created
finnschiermer
username_4
I think the freeze methods needs to throw if the object isn't present in the target frozen realm.
comment