feat(articles): Server-side paging for list

[mleanos]

Adds capabilities to handle the Articles list paging server-side.

Updated tests to reflect changes.
EDIT: During review of this PR, some concerns were raised. These latest changes are an attempt to address those concerns. Once the implementation is agreed upon, and finalized, I will add tests.

Closes #1048

[simison]

BTW Angular 1.5 has a new lifecycle hook $onInit:
http://angularjs.blogspot.lt/2016/02/angular-150-ennoblement-facilitation.html

You could use that instead.

[simison]

Actually I take my words back, seems like $onInit works only with new modules.

[mleanos]

@ilanbiala I've changed the implementation to scale better. I think it's pretty close to the best we can get, with a feature like this. Or maybe it's not 😳

I've tested this with 500,000 Article documents. The response time from the server to retrieve the specific page increased as I moved up the pages. Eventually, at around the 9000th page (of 20 articles per page & querying all 500,000 Articles) the server request timed-out. Up to that point, as I reached this limit, the response would take somewhere around 8500 ms at around page 8000. The first few pages took around 25 ms, and I didn't see much increase until I started to get up to around page 50 (1000th Article). I know this isn't a detailed benchmark report of the performance. But I think we get the idea of how this will perform.

I'm wondering what our expectations of this feature are. How well can we expect server-side paging to perform? I don't know what any other library might be doing to solve the same issues that we're facing. At some point our users will have to decided if the implementation makes sense for them, and even if their data grew to a point where this became a performance issue, they could adjust the defaults that we've set for the paging feature.

I'm not opposed to looking at using a library that may be solving this. My goal at this point is to determine what we want out of this feature, and what hurdles we must overcome.

Let me know your thoughts. I'm more than happy to keep at this feature, until we're happy. There's no real rush here.

@codydaig @lirantal @simison Thoughts?

[mleanos]

I left this comment here, so there is some context of why the filter is there & what the implications of using it are.

[mleanos]

@codydaig @ilanbiala @lirantal @simison Can we pick this back up since the other bigger PR's have been merged already?

I'd like to figure out if this PR is on the right track. If I need to take a different direction, or look for a library to use then I'm more than happy to do so. At the very least, we should figure out what we want from a feature like this.

[mleanos]

@meanjs/contributors Need more eyes on this to determine if I should continue, or go down a different path.

[lirantal]

Hi @mleanos , I'm overall neutral about it. Since it shows an example of pagination implementation I guess it may contribute to the stack to show users how to do that.

What is the use for the db-collection-stats API routes?

[codydaig]

I like the general flow from what I can tell.

[mleanos]

@lirantal the db-collection-stats can be used to collect statistics on a particular collection. If we want to display the total count of Articles in the database, then we can use this route to get that info. In this current PR there is no use for it, but I left it there in case we decided to use it. Previously, I was showing the count on the list view, so that the user could see the actual count of Articles; since this list is only paging Articles from the past 30 days.

It might be something that would be only used by the Admin area.

[mleanos]

I'm not opposed to merely having this branch up on my fork, for reference on how to do it. I'm not 100% sold on the idea that we need it included in the framework. It does introduce some complexity, and potential performance issues; if users don't mind the limitations of a feature such as this.

[lirantal]

Like I said before - I'm overall neutral. If we do want to merge this feature I'd remove the collection related code as it is not implemented and only adds extra confusion for readers using the framework.

[ilanbiala]

I'd also say that we should merge this in and remove the stats code simply because we aren't using it.

[mleanos]

Conflicts after merging #807. I'll rebase, and resolve the conflicts.

[ilanbiala]

Sounds good.

[mleanos]

@ilanbiala Final review?

[vaucouleur]

@mleanos

Thanks for this PR ! Two quick comments.

This is the current implementation:

var query = Article.find(filter).limit(100).sort('-created');
query.count(function (err, count) {
....
}

The count is indeed useful to get the total number of hits -- but from a performance point of view, is the sort really necessary for the "count query" ? Could the sort be moved to the other query (the one that does the skip and get the results) ?

Also, from a functional point of view this time, in this current implementation, the "count query" will be impacted by the limit, isn't it ? It's perfectly OK to limit the "result set" query but, I guess, the count of the total number items should always be correct. So, may be, the current limit on the "count query" should be removed.

[mleanos]

@vaucouleur Thank for jumping in here, and providing your feedback.

The way the queries are setup in this implementation is to avoid an performance pitfalls with large collections.

The filter query only gets the top 100 Articles from the past 30 days. The reason limit() is used here is to avoid iterating through a large dataset. For instance, if we have 100k Articles created in the last 30 days, we don't want to traverse such a large data set when we perform our skip() & limit() in the next part of the query.

Think of the first query (the one with the filter & limit) as all pages. The second part (with the skip & limit) is used to get the current page.

From my perspective, we need the sort() performed on the first query in order to ensure we get virtually the same top 100 Articles based on our filter, and we'll use this data-set as our pool of Articles that we're paging through.

Using the created data field, and applying a limit of 100 in the filter query is meant as a guide on how to implement pagination server-side. This filter, and limit, seemed appropriate for Articles. However, users of this framework can adjust this to their specific needs.

I combined the suggestion in this SO answer with other suggested methods, for this type of implementation.

[mleanos]

FYI, I just deleted 8 of the duplicate Coveralls notifications in this PR.

[lirantal]

Cool, thanks.
Would be great to test it out and merge.

[mleanos]

I've been testing it. I encourage others to test manually. More importantly, I need a proper review of the implementation. If you see my comments above, there are some points of discussion still unaddressed.

Mainly, the client-side input for filters. What types of filters should we expect (and/or require) from the client-side request to the API? We need to consider different types of filtering: $in/$nin, something similar to like('%xxxxx%') comparison, $eq, etc.

Currently, the mongoose-filtering service does just a simple equality check based on the filter(s) ([{ title: 'somesearchstring }]`) sent with the request to the API. This works for a simple example, and we may just want to leave it at that. We could just expect that our users would extend this service/feature to suit their own needs.

Other than that, it's just a matter of reviewing and getting some LGTM's. If I can see that this implementation is generally accepted, then I will finalize the tests & we should be good to merge.

[hyperreality]

Why specify these here when they are only going to be plugged directly into the find() function?

[mleanos]

The point is that you may have multiple views, or user inputs, that change the filters and/or sorts.

Furthermore, you could have many different clients sending API requests to the back-end with varying filters & sorts.

[hyperreality]

@mleanos Just gave it a spin and it's great. Even into the tens of thousands of articles performance is retained.

For the filters I suggest one that finds any article with a title including the search term, ignoring case. I can't see how we could implement any more advanced filters at this stage. It would also be nice to have a button to flip articles being sorting by ascended and descended creation date.

[mleanos]

@hyperreality I agree that it would be nice to have the search using an "in-string" type of comparison, and to have sorting exposed with a user input. However, my worry is that we'd end up making the UI implementation of this too opinionated. Where our users will most likely have varying requirements for how the UI interacts with this feature. By keeping the example really simple, we can show the usefulness of the feature. While at the same time making it easy for our users to "roll-their-own" UI for it.

Thank for the feedback. If you wanted to test out even more Articles, I left a link (above) to a Gist with a script that seeds many Articles. Once the collection starts to grow substantially, the sorting should be modified to either use an indexed field, or no sort at all (thus, using the default on the server).

[vaucouleur]

@mleanos Michael, I do not understand the need for the "default sort" on _id. It breaks the natural order of the records in mongodb, and according to my benchmark, it does not help from a performance point of view. There are actually 3 possibilities: "asc sort", "desc sort", and "no sort" -- and "no sort" should be the default. The only time I had to use a "default sort" on an id column was in the context of server-side pagination with a relational database (using a relational database, the query plan for complex queries might change depending on the page number, hence sorting becomes a necessity). As far as I know, this is not the case with mongodb, hence a default sort is not required. Am I missing something ?

https://github.com/mleanos/mean/blob/2b3f3bf68e71c7e459757ef104f5d1cf5c738339/modules/articles/server/controllers/articles.server.controller.js#L84

[lirantal]

Too big of a PR for me to dive into, would appreciate if you guys could come to an agreement.

[mleanos]

@vaucouleur The default sort is just an example. Sure, it does break the natural order of the documents. Perhaps it was a bad choice for a default. However, I would like to include a default sort with the Admin list (/api/parameterized-query/articles) on the server-side. This is meant to show the capabilities of our paging-filtering-sorting solution. Would it be better if I changed the default in this method to sort on created, or title? I can also, update the comment to clearly explain more about this. I purposely chose to add the default sort to the list that's being accessed by the Admin area. For the public facing list of Articles, there's no default sorting.

In the case where I was sorting, with 1M Articles in my db, there was no significant differences in performance than when I had 100,000 articles. Just as long as I was sorting on an indexed field. So I wanted to show that as well.

I understand the differences you're talking about with relational databases, and MongoDB, and their query execution plans. However, sorting is a very common use case for this type of feature, so why not show it?

Other than that, do you have feedback on the other aspects of these changes? I think we're close on this one, but there are few things to wrap up. The issue you brought up, being one of them. Thanks!

[vaucouleur]

@mleanos I'm happy to agree with most things since I can always change the code to my liking, including keeping the natural order of the documents when "no sorting" is specified by the user :) So, not a big deal.

I did not see any other issues. It looks good, and I'm sure it will be very useful to many meanjs followers. Thanks again for your good work !

[mleanos]

@hyperreality @codydaig @ilanbiala ?

[lirantal]

@hyperreality @vaucouleur @codydaig @simison
Merging this PR is now or never... can you please give your +1 or any constructive feedback to @mleanos to address standing issues?

[hyperreality]

All my comments were nitpicks, the implementation looks great. I still think it would be best to provide an in-string search out of the box because it is such a common-use case which could be easily adapted or removed as the dev pleases, but I also understand why @mleanos doesn't want to add it in.

[mleanos]

I did not see any other issues. It looks good, and I'm sure it will be very useful to many meanjs followers. Thanks again for your good work !

@vaucouleur Thank you for your feedback, and the benchmark work you put into helping get this PR ready. This implementation wouldn't have been as good without your involvement! I salute you good sir :)

Merging this PR is now or never...

Harshness.. haha. Thanks for putting a fire under our feet lately @lirantal (it's a good thing), leading to our final push for 0.5.0!

[lirantal]

My pleasure :)

[mleanos]

@simison Have you had a chance to review this? You had brought up using the http headers to pass back the pagination info in our Gitter discussion. Is that something you would like to see in this implementation?

I just want to double check with you before I merge, since Trustroots has a much different pattern for server-side Pagination. You probably have good insight into whether or not this would work; I know you're trying to move away from mongoose-paginate.

[coveralls]

Coverage increased (+0.03%) to 73.086% when pulling d344bdc on mleanos:feature/server-side-pagination into 4f3a501 on meanjs:master.