Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: allow requesting notes list in chunks #720

Merged
merged 1 commit into from
May 27, 2021
Merged

API: allow requesting notes list in chunks #720

merged 1 commit into from
May 27, 2021

Conversation

korelstar
Copy link
Member

@korelstar korelstar commented May 20, 2021

see nextcloud/notes-android#761

  • add new request parameter chunkSize and chunkCursor, send HTTP header X-Notes-Chunk-Cursor
  • documentation
  • tests

@korelstar korelstar added the feature: API Related to the API for 3rd-party apps, i.e. Android or iOS label May 20, 2021
@korelstar korelstar added this to the 4.1.0 milestone May 20, 2021
@korelstar korelstar force-pushed the chunked branch 2 times, most recently from fa50089 to b4e27ca Compare May 22, 2021 12:22
@korelstar
Copy link
Member Author

@stefan-niedermann Would you have a look on this? I just added the documentation. Please check if the docs are understandable and if the functionality is as expected/desired.

@stefan-niedermann
Copy link
Member

Yeah, i think i got it. But i expected to retrieve information about the totalCount when querying the notes list in chunks. Maybe this should he added as a response header when the chunkSize parameter is set?

I planned to show a progress bar (at some point in the future) at least when importing the account the very first time and i have a feeling that this requirement would fit to this PR - information about the total notes count would be necessary though.

What do you think?

Otherwise the documentation is understandable ✅

@korelstar
Copy link
Member Author

korelstar commented May 22, 2021

I'm fine with such an addition. But let's clarify what is exactly needed and what can be provided. If you do a full import, the total notes count is interesting. However, if you combine the parameters chunkSize and chunkCursor with pruneBefore, the total notes count is misleading (since the total number of transferred notes is not equal to the total number of notes). Therefore, I think a "number of remaining notes" would be more general.

Proposal: if the current response is not the last response, the additional HTTP header X-Notes-Chunk-Pending is set to the number of remaining notes.

What do you think about this approach?

@stefan-niedermann
Copy link
Member

Fine with it this definition ✅

One more thing regarding the wording: I am personally used to the term batch in this context instead of chunk - i am fine with keep naming the parameters chunkXXX, just wanted to note it in case you struggled with the naming, too.

@korelstar korelstar marked this pull request as ready for review May 24, 2021 12:22
@korelstar
Copy link
Member Author

Wording is never easy. I think "batch" is more from the client processing perspective. But since REST is about resources and data, I think "chunk" fits better.

I'm finished now. If there is no protest, I will merge it in the next days.

@korelstar korelstar merged commit 476c18f into master May 27, 2021
@korelstar korelstar deleted the chunked branch May 27, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: API Related to the API for 3rd-party apps, i.e. Android or iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants