Implement Batching

[lukasgraf]

Implements batching for

• Plone Site Root children
• Dexterity Folder
• Archetypes Folders
• Collections
• Search Results

Also changes implementation for GET on folderish contexts so catalog queries are used instead of objectValues().

TODO:

• Documentation
• Use Plone's default batch size as the default (turns out Plone doesn't have a configurable default batch sizes. It seems to be hard coded to 25 in most places, so I changed the default in plone.restapi.batching to that as well)
• Rename items_count to total_items
• Refactor tests

Closes #8

[lukasgraf]

@tisto this one is ready for review

[lukasgraf]

@buchi fixed some flaky tests and rebased onto master - please review

[lukasgraf]

@buchi updated to not have HypermediaBatch inherit from Batch - it was only supposed to proxy to a batch.

[buchi]

Still need some time for the review. Will do asap.

@lukasgraf PR needs rebase

[lukasgraf]

@buchi rebased

[tisto]

@lukasgraf if the request is on "/folder/@search?b_size=5&sort_on=path", why is the @id equals "/folder/@search?sort_on=path" (without the "b_size")? Shouldn't this be either the base request or the full request with all params?

[lukasgraf]

@tisto no, this is supposed to be the canonical ID to the base collection without any batching information (loosely based on https://www.w3.org/community/hydra/wiki/Collection_Design#Pagination / https://www.w3.org/community/hydra/wiki/Pagination#PartialCollection). The ID for the current batch is in batching/@id.

I guess the sort_on parameter doesn't really affect the collection's contents, just their ordering. But if you consider other query string parameters for @search (or possibly other endpoints), the query string params can directly affect what resource will be returned, so I'm preserving them and just stripping batching related params for the canonical resource URL.

[tisto]

Right. My feeling is that we should remove the sort_on param from the canonical top-level id as well. Do we have any hypermedia controls for changing the sort order? Have you looked into that?

[lukasgraf]

@tisto I briefly looked into it, but couldn't find any spec for hypermedia controls to specify sort order that are standarized in any sense. Updated HypermediaBatch.canonical_url to also remove sorting related params.

[buchi]

Maybe we should rather call it pagination or paging. Batching is often used in the context of performing bulk operations. Any thoughts?

[buchi]

What about including total_items in batching instead of top-level?

[buchi]

has been renamed to items

[lukasgraf]

Ah, well spotted. I will do one more pass for member -> items.

[lukasgraf]

The term "pagination" is definitely much more common outside the Plone world. Since we're doing a rather low-level API that exposes many of the implementation details already I decided to stick with Plone's naming.

I wouldn't be opposed to naming it "pagination", but calling it that and still using the b_size and b_start parameters would be a bit odd. (And we should keep using those IMHO, because they cause some implicit magic to happen under the hood that we would otherwise need to reimplement).

[lukasgraf]

Re: moving total_items inside the batching info dict: I also considered that. I ended up going with a top-level attribute that was in an example in one of the earlier design documents, but it would make more sense to move it inside the batching info dict. 👍 I'm in favor of moving it, any objections @tisto?

[tisto]

@buch @lukasgraf the rationale behind the top-level total_items is that all items inside "batching" are supposed to be hypermedia controls that the client just exposes 1:1 to the user. If we start adding more items there we will end up with tight coupling between client and server because the client needs to know about the exact structure and names of the API.

[lukasgraf]

@tisto I see - we'll leave it like that then. The client shouldn't be relying on total_items for page calculations anyway, because it can only ever be approximate (resultset may change during pagination).

[tisto]

@lukasgraf I'm wondering if it would make sense to rename "total_items" to "items_total". This would make "items_total" show up right after "items". Does not make any difference for the consumer app but for a developer browsing the API.

[lukasgraf]

We could, I don't have any strong feelings about either one ;) Regarding developer-friendly representations though, we might want to look into using OrderedDicts at some point. If given an ordered dict, json.dumps() will preserve the order of the keys. Structurally the order of keys in a dict still has no meaning of course, but that might make it easier to produce JSON that is layed out in a human-friendly way.

[buchi]

Right now the batching information is included even when the items count doesn't exceed the batch size.
IMO opinion it doesn't make sense to provide batching links to the first or last batch in this case. A js client will unnecessarily have to check if it should display those links. I would propose to return an empty batching object instead or to not include the batching at all. @tisto @lukasgraf opinions?

[lukasgraf]

@buchi sounds good to me. Though if we were to still include the batching object, it probably shouldn't be completely empty, it would still need the @id I believe, otherwise JSON-LD processors would drop it alltogether. So maybe just leave it out entirely?

[lukasgraf]

Can we get a consensus on renaming total_items to items_total and omitting the batching dict if len(resultset) <= batch_size? I don't feel strongly about either point, but I'd like to prevent this PR from stalling and move forward towards a first alpha.

[tisto]

@lukasgraf +1

[lukasgraf]

@tisto @buchi updated:

• Total item count is now called items_total
• Omit batching links object entirely if resultset isn't batched

I also rebased onto master and squashed my commits in a way that some back and forth changes were eliminated.