Skip to content
This repository has been archived by the owner on Mar 28, 2019. It is now read-only.

Total-Records should give the total number of records. #43

Merged
merged 2 commits into from Oct 13, 2015

Conversation

Natim
Copy link
Contributor

@Natim Natim commented Oct 9, 2015

Today Total-Record only contains the number of records in the current request.
Kinto protocol defines it to be the total number of records for the collection.

We should use the syncclient.info_collection() methods in order to return the right number of records. We may want to cache this value when used with pagination.

@Natim
Copy link
Contributor Author

Natim commented Oct 9, 2015

The right endpoint is get_collection_counts rather than info_collection but that's the same idea.

@Natim
Copy link
Contributor Author

Natim commented Oct 9, 2015

@rfk is it something that we should fix in syncserver instead?

@Natim
Copy link
Contributor Author

Natim commented Oct 9, 2015

Actually the documentation describes the implemented feature:

This header may be sent back with multi-record responses, to indicate the total number of records included in the response.

So I guess it is not a Sync Server bug.

@Natim
Copy link
Contributor Author

Natim commented Oct 9, 2015

@michielbdejong Can I remove this feature from syncto until we actually get a use case?
The scope of Syncto is to let us use Kinto.js to query Sync and Kinto.js doesn't use the Total-Records header.

@Natim
Copy link
Contributor Author

Natim commented Oct 9, 2015

Here is a proposal to remove Total-Records when paginating and leave it when requesting all records.

r? @leplatrem @michielbdejong

@Natim Natim self-assigned this Oct 9, 2015
@Natim Natim force-pushed the do-not-support-total-records-with-pagination branch from 80e4f9a to 0f58946 Compare October 9, 2015 15:14
@michielbdejong
Copy link
Contributor

+1 but if next year Kinto.js starts relying on this header, we should somehow remember that Syncto does not implement the entire Kinto API. We will have integration tests that would start failing, but maybe we should also update "using the Kinto API" on http://syncto.readthedocs.org/en/latest/ to "using a subset of the Kinto API", so that this is clear.

@Natim
Copy link
Contributor Author

Natim commented Oct 12, 2015

Good point thanks.

@Natim
Copy link
Contributor Author

Natim commented Oct 13, 2015

@leplatrem I have created #51 to track a potential follow up on this.

leplatrem added a commit that referenced this pull request Oct 13, 2015
…ords-with-pagination

Total-Records should give the total number of records.
@leplatrem leplatrem merged commit 081ecb2 into master Oct 13, 2015
@Natim Natim deleted the do-not-support-total-records-with-pagination branch October 13, 2015 12:35
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f0d9de2 on do-not-support-total-records-with-pagination into 46f4d77 on master.

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.

None yet

4 participants