Some design decisions

[ecordell]

I just wanted to put my thoughts here for discussion. There are a couple of conflicting designs right now and I think they should be unified.

withSyncMode: andClass: There are several methods suffixed with WithSyncMode:(RKSyncMode)syncMode andClass:(Class)objectClass. And not just front-facing API methods, there are many internal methods with this structure so that the syncMode and class can be passed around.

However, since we don't allow multiple sync calls at the same time (at least not using just one syncManager), couldn't we nix the internal withSyncModeAndClass methods in favor of global class variables, _syncMode, and _objectClass?

syncMode, syncStrategy, syncDirection I added the syncMode property to RKManagedObjectMapping. @makdad, you added _strategies and _directions dictionaries to RKSyncManager. Both of these ideas associate syncing information with an object class.

These are both good solutions, but I think we should pick one or the other: either have syncMode, syncStrategy, and syncDirection property on RKManagedObjectMapping, or have _modes, _strategies, and _directions dictionaries in RKSyncManager.

The only advantage I can think that one has over the other is that associating them with a mapping instead of a class means that we could conceivably have multiple mappings with different syncOptions for the same class. Is that useful?

Overall I think this is getting close to ready for primetime. @makdad has started writing tests, so we can work to flesh them out and get some really good coverage. Over the next few weeks I'll write a good example app to include with RestKit that highlights all of the different features and use cases. (I have one that I'm using locally but it's fairly old, iOS4, and not clear/complex enough to be a good example).

There is still the issue of "batching more" to further reduce network requests, and having support for a single sync resource path. However, right now this does enough that I think those could be worked on later, as updates.

It might be a good time to see if @blakewatters has any opinions? I know @grgcombs has expressed interest as well.

[makdad]

@ecordell Inre: where syncMode should live: after I finished putting that in there (into RKSyncManager), I was thinking, "gee, this could go on the mapping as well". Actually, I think it works better on the mapping in retrospect.

If it's a per-class thing, it becomes global for the whole app. There is PROBABLY some case where someone wants objects to sync in a certain way for one case, and another way for another case, and might want to use 2 different mappings to represent that (for example, talking to 2 different servers). So yes, it was nagging at me to change that, I agree with that direction -- move strategy & direction over to the mapping. The mapping is more like a configuration for how RK should treat objects... so I like keeping it centralized there.

Inre: the internal methods with syncMode:class:. Personally, I like it how it is. Even though we could refactor those out as class variables as you suggest, I'm a big fan of functional programming -- even inside the same class, everything a method "needs" to do its job is passed in, as opposed to referenced via the class. I find this often promotes code reuse, and (albeit noisily) identifies exactly what information you "need" to do a task. It often helps me question my own design decisions when it feels "funny" to be passing around so much. It usually tells me I have a method that does "too much". So maybe that's what you're feeling -- some of the main sync methods do do a lot (queue management, delegate calls, the actual syncing).

Finally, question about the block implementation. That totally makes sense to me, and since my company is iOS5 (not even 4!) plus, I am happy to rely on blocks to be smart. However, I've noted other places in the RK library where there are block and non-block options. We'd basically be saying that you need blocks support to use RKSyncManager. I'm absolutely fine with that decision. Is Blake, though? :)

[ecordell]

@makdad Just took care of the stuff in here.

And I think that, yes, we should require blocks for this. A non-block version would, at least at first thought, require much more structure (think queues and ids and nonsense). It's doable but I think is too time consuming, and iOS has awesome version adoption rates.

Here's what's left on my todo list

[makdad]

@ecordell I'm fine with blocks-only -- agreed totally on the complexity required otherwise.

I'm on holiday until early next week but then I'll have a go at some of these remaining items.

[makdad]

@ecordell I had another think about everything on the todo list -- inre: the separate MOC, do you mean for the queue items, so that way we are not constantly "checking" to see if our updated/saved/deleted item was a queue item itself? I assume the actual items being synced need to stay on the main MOC.

[ecordell]

@makdad Ha, yeah. I think that should've said "Investigate using a separate MOC". Someone mentioned this somewhere in the pull request; being able to discard the changes made during a sync.

But as you say we need to watch the main MOC for changes, so if you want a temporary one you should make it outside of the syncManager entirely. I think that can be scratched off the list.

Also, I have an idea for queueing requests that I think would be an improvement, what do you think?

RKSyncManager has two queues: a pushRequestQueue and a pullRequestQueue. (These would be RKRequestQueues). Any time a post/put/delete request needs to be made, we load it into the pushRequestQueue, and any time a get request needs to be made, we put it in the pullRequestQueue.

The catch would be that if there's anything in pushRequestQueue, pullRequestQueue would be suspended. Basically we need to send all changes in local data to the server before pulling anything back, so that the server has a chance to register everything (this is especially important when using a managedObjectCache to clean up items. If a pull is made before a new object on the device has been pushed, the new object will be deleted).

This seems sound to me, but I'm not sure if I'm missing some use cases. Since I know you intend to use this for a slightly different purpose than myself, I thought you might have some good input here.

The current solution (which I just added yesterday) adds everything to a single requestQueue but only allows 1 concurrent request at a time - definitely not ideal.

[ecordell]

Update: fixed this. There's now a concurrent dispatch queue (finally got myself up to speed on GCD).

If in batching mode: Push requests (post, put, delete) requests are queued up so that they can run concurrently. We're guaranteed that the requests don't step on each other's toes, so to speak, precisely because of the batching. (i.e. we won't be sending a delete before a put, because batching inspects the queue ahead of time to make sure the put request isn't even there) Pull requests (get) are barrier items - they wait until everything above them goes, then the execute alone, and then anything after them is concurrent again.

If in proxy-only mode: It's still technically a concurrent queue, but everything is loaded as a barrier item. Functions like a normal fifo, one-at-a-time queue. Because here, order is important even for push requests.

@makdad, I think this is the final stretch. In the future I'd like to get collection routing and single-url-posting as options, but those would be best served as updates. I'll be writing docs/tests/examples over the next couple of weeks, I'd love to get your opinions on recent work and docs and such.

[makdad]

@ecordell I've had another look at the code. GCD seems like the way to go, but unfortunately we've got really long methods again - that's going to make it a bit more difficult to unit test. I think we'll need to do some more extraction and bring the average down a bit (will improve readability as well).

I'm on deadline for a different part of my project that uses RK, so there's no pressing need for me to work on this right now unfortunately (well, what I should say is that there IS a pressing need for me to work elsewhere). It will be August before I'm back on the iOS side ... :(

[ecordell]

Understandable, I only work on this when I have time to (though now I have a project that really needs the syncing, so I'll be testing this branch in production soon).

I know the methods got a lot longer, but mostly it's just the one sendObjects method, and that has pretty obvious ways to split it up. I figured it would be easier to split up during testing.

Anyway, if you get a chance I'd like to make sure I didn't break anything for your use cases :) I'll be writing unit tests soon, so that should catch regressions, but it's always good to get live feedback.