Fix the exception message for refreshing an immutable Realm #5065
Conversation
| @@ -822,6 +822,10 @@ bool Realm::do_refresh() | |||
| return false; | |||
| } | |||
|
|
|||
| if (m_config.immutable()) { | |||
| throw std::logic_error("Can't refresh a read-only Realm."); | |||
There was a problem hiding this comment.
Can we instead return false like we do for frozen Realms?
There was a problem hiding this comment.
Cocoa throws an exception if the user explicitly calls refresh on an immutable Realm because it's almost certainly a sign that the developer is trying to do something that won't/can't work and we want to let them know. It makes it awkward if they write a function which accepts an arbitrary Realm and they want to call refresh() on it, but that's a terrible idea so making it awkward isn't necessarily a bad thing.
There was a problem hiding this comment.
Makes sense, but isn't it the case for frozen realms as well? I guess in my mind those two are very similar and it just triggers my OCD a little that we have different behavior here.
There was a problem hiding this comment.
Allowing calling refresh() on frozen Realms does seem like a mistake to me.
Fixes #5061
It may be debatable if calling refresh on an immutable Realm should throw or silently do nothing. We could easily make it a no-op, but it seems that the historical behaviour has been to throw an exception so I'll bring that back to not break other SDKs who may be relying on this.
Tracking down the history led to #4688 being where the first refresh was allowed and did nothing. This was because the first refresh of an immutable Realm had no associated transaction and so populated one there. The second call to refresh, now having a transaction actually tried to advance the version and the DB object throws because it has been configured without history.
☑️ ToDos