no Total-Records header in GET requests. only in HEAD requests.

[peterbe]

Fixes #1624

• Add documentation.
• Add tests.
• Add a changelog entry.
• Add your name in the contributors file.
• If you changed the HTTP API, update the API_VERSION constant and add an API changelog entry in the docs
• If you added a new configuration setting, update the kinto.tpl file with it.
• A bunch of storage tests use the now deprecated internal API of storage.get_all when in fact it should be using storage.list_all or storage.count_all.

[leplatrem]

@peterbe, don't worry about the conflicts, I can do it and try to push a merge on your branch.

[leplatrem]

I have one major question though: what do we do about our clients:

https://github.com/Kinto/kinto-http.js/blob/27737b9e62df2aff6adc32523708cd7e66131282/src/base.js#L540 --> Nan
https://github.com/Kinto/elm-kinto/blob/d5b5337166f23d130322858ace30c0a80abdca28/src/Kinto.elm#L299-L301 --> 0

Maybe we should first release of kinto-http.js where the totalRecords is obtained via a HEAD?
(I don't know, truely asking..)

[peterbe]

Haven't dug in yet but the base.js you pointed to is inside a function called paginatedList and the doc string...

   * Fetch some pages from a paginated list, following the `next-page`
   * header automatically until we have fetched the requested number
   * of pages. Return a response with a `.next()` method that can be
   * called to fetch more results.

So, if the idea is that you use...

for (const page in client.paginatedList(args)) {
  ...
}

...then we're fairly certain the total records is not needed or used. The pagination is done using Next-Page.

Perhaps my brain is a bit end-of-day fried and there's a lot of mind-bending async in that code but it looks like the totalRecords isn't actually used.

[glasserc]

Yeah, my hypothesis is that nobody actually uses the totalRecords field on the paginatedList result, but just pages the results. I think automatically including a call to getTotalRecords (on each page...) would be a prohibitive performance penalty and would rather not do that.

[leplatrem]

Appart from the count_only in the model get_objects(), it looks good to me!

GG! 🙌

Also, this is not a minor change from the client perspective. The totalRecords is exposed in the result of listRecords() in kinto-http.js for example. I think we should be very explicit about the change in the API changelog. If we say we follow semver, we should bump a major for the API version, and well....I'm not sure we're ready for that. So, at least we should be very explicit!

[leplatrem]

We should probably have a _details-head-list.rst where we mention that the header can be obtained with HEAD

[glasserc]

At a glance it looks OK. I'll defer to Mat for review on this. This PR badly needs a rebase :)

[peterbe]

This PR badly needs a rebase

Do you mean a squash? Isn't that done with the green button when it's fully approved?

Or do you mean it needs the latest and greatest of master?

It's hard to remember what different projects have as best practices with active PRs and the landing protocol.

[Natim]

Usually in the Kinto org we merge without squashing to keep the history and pace of the pull-request. Here we just need to fix tests I guess

[peterbe]

I can rebase this to get rid of unimportant small commits but keep stuff like the master merge and various others. But I don't think we're done yet :)

[Natim]

No don't worry about that.

[glasserc]

Do you mean a squash?

No, I meant a rebase, meaning moving these comments onto master so that we get fewer wacky diffs like this:

[leplatrem]

The wacky diffs come from some mistakes I made during the painful merge I guess.

[leplatrem]

@peterbe, I merged and repaired the merge :) Tests now pass.

I believe what remains is:

• be extremely clear about the removal of the header in GET in changelog
• Maybe we could have a server setting to maintain the header if set to True (default: false) for situations where clients rely on this info
• document that the header can be obtained with HEAD
• figure out a way to nicely document that we Total-Records is now deprecated in favor of Total-Objects because of Rename core notions #710
• improve the models code (eg. two methods instead of count_only param)
• squash could be a good idea (didn't want to force push on your branch ;))
• kinto/core/storage/testing.py still uses objects, total_count = self.storage.get_all(params) and that causes deprecation warnings.

[peterbe]

@leplatrem It's a bit of a mess but check out the latest couple of commits related to documentation and whether it solves the figure out a way to nicely document that we Total-Records is now deprecated in favor of Total-Objects because of checkbox above.

Also, see the latest commit regarding how I broken up model.get_objects so we no longer need this count_only=boolean stuff.

[leplatrem]

Niiice!

Thanks for the hard work!

I made a few comment, the main one is about having a deprecation decorator for a method that was just introduced! Otherwise, it looks like close to the finish line :)

[glasserc]

Just some drive-by comments..

[leplatrem]

🎉 🎉

I let you rebase/squash/do whatever you want before pressing the merge button ;)

[leplatrem]

in the resource?

[peterbe]

I let you rebase/squash/do whatever you want before pressing the merge button

I don't think there are any commits that are worthy to have to be squashed. If I do try to selectively squash some of them, I will have to go through the same merge conflicts you went through, no?

[leplatrem]

Yeah, I'm ok with whatever ;)