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

Add subscription management functions #6029

Merged
merged 6 commits into from
Dec 14, 2018
Merged

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Dec 3, 2018

This adds -[RLMRealm subscriptions] which returns a RLMResults of subscriptions and -[RLMRealm subscriptionWithName:] to look up a specific subscription, along with Swift wrappers for these. Rather than having a special thing using LIKE syntax as JS has, the RLMResults is simply filterable like any other collection.

The implementation of this is made awkward by that we already have a RLMSyncSubscription type that's not an RLMObject and doesn't behave like an RLMObject. Fortunately, obj-c is sufficiently dynamic that we can sometimes return the existing objectstore Subscription-backed type and sometimes return a new RLMObjectBase-backed type and have them behave (hopefully) identically.

Closes #6010.

This adds `-[RLMRealm subscriptions]` which returns a `RLMResults` of
subscriptions and `-[RLMRealm subscriptionWithName:]` to look up a specific
subscription, along with Swift wrappers for these. Rather than having a special
thing using LIKE syntax as JS has, the `RLMResults` is simply filterable like
any other collection.

The implementation of this is made awkward by that we already have a
RLMSyncSubscription type that's not an RLMObject and doesn't behave like an
RLMObject. Fortunately, obj-c is sufficiently dynamic that we can sometimes
return the existing objectstore `Subscription`-backed type and sometimes return
a new `RLMObjectBase`-backed type and have them behave (hopefully) identically.
@tgoyne tgoyne force-pushed the tg/subscription-management branch from 33aa0fb to fa02e20 Compare December 3, 2018 23:14
Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Some comments to the implemented semantics. The tests look fine as well, but a lot of the implementation went over my head.

}

- (void)unsubscribe {
__builtin_unreachable();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this? Any reason this could not be implemented?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just an abstract class that's never instantiated. There isn't actually any reason for it to be, though (initially RLMSyncSubscriptionObject was a subclass of it, but that didn't work at all) and I think I can just re-merged RLMSyncSubscription and RLMNewSyncSubscription.

}

- (NSString *)name {
return _row.is_attached() ? RLMStringDataToNSString(_row.get_string(_row.get_column_index("name"))) : nil;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are managed RealmObjects normally allowed to be accessed when the Realm is closed? In Java this would throw an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this class is specifically trying to act like the pre-existing RLMSyncSubscription type, which isn't a managed object.


- (void)unsubscribe {
if (_row) {
partial_sync::unsubscribe(Object(_realm->_realm, *_info->objectSchema, _row));
Copy link
Contributor

Choose a reason for hiding this comment

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

In Java we implemented this as a normal managed operation, i.e. it requires a write transaction and it basically just an alias for deleting the object. I'm not sure I can what semantics make the most sense for the Cocoa binding though.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is something that's already part of the released API and not a new thing. I don't think the semantics we chose are great, but it's not bad enough to justify a breaking change to them.

@throw RLMException(@"-[RLMRealm subscriptions] can only be called on a Realm using query-based sync");
}
// The server automatically adds a few subscriptions for the permissions
// types which we want to hide. They're just an implementation detail and
Copy link
Contributor

Choose a reason for hiding this comment

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

In Java we actually just exposes them. Mostly because these methods might also be used for debugging and not showing all subscriptions can also make it hard to reason about the system, especially if the API's are used by us internally. I do agree that if a user by accident deletes them they will probably be very surprised, but IMO the chance is pretty slim and will probably be detected pretty quick.

Copy link
Member Author

Choose a reason for hiding this comment

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

My concern (other than confusion) is that realm.subscriptions().map { $0.unsubscribe() } is an easy one-liner to remove all your subscriptions that'll appear to work fine and won't even cause any obvious problems if you aren't using object-level permissions, but has actually left your realm in a broken state that may cause not-obviously-related problems down the road.

@tgoyne tgoyne merged commit 7be59e9 into master Dec 14, 2018
@tgoyne tgoyne deleted the tg/subscription-management branch December 14, 2018 20:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better subscription management
2 participants