Graphback: No pagination for delta sync

[wtrocki]

Delta sync do not allow to paginate thus it can return large number of results that will overload client. Pagination should be enabled

[wtrocki]

@ssd71 We will not make it for 14 so this is 15 target

[ssd71]

A few questions:

• Are we going to do cursor based pagination here or page based? I feel like page based pagination will fail here suppose if we have data that is being updated between client fetching two pages, client will probably miss some updates if we only use limit and skip
• If we want to do cursor based pagination e.g something like lastSync and limit: some documents with same timestamps will be either missed (if we use simply _lastUpdatedAt:{ gt: lastSync } ) or repeated (if we use _lastUpdatedAt:{ ge: lastSync }

[machi1990]

• If we want to do cursor based pagination e.g something like lastSync and limit: some documents with same timestamps will be either missed (if we use simply _lastUpdatedAt:{ gt: lastSync } ) or repeated (if we use _lastUpdatedAt:{ ge: lastSync }

I believe we have this kind of pagination implemented by findXXX query. Maybe we can go with something similar:
https://github.com/aerogear/graphback/blob/ce7236aa84cf053d0e9090115ab5b2b95d3276e4/packages/graphback-core/src/runtime/CRUDService.ts#L96-L117

Maybe this kind of logic can be implemented for the DataSyncCRUDService#sync method:
https://github.com/aerogear/graphback/blob/ce7236aa84cf053d0e9090115ab5b2b95d3276e4/packages/graphback-datasync/src/services/DataSyncCRUDService.ts#L21-L28

WDYT?

Also let's get @wtrocki @craicoverflow opinion on this.

[wtrocki]

Happy with what was suggested

[ssd71]

@machi1990 Sorry, but just to make sure we are on the same page: you are proposing that we use lastSync and limit as parameters to paginate a sync query (instead of offset and limit) or are you proposing that we use offset and limit themselves?

[ssd71]

I found this article about paginating data from mongodb and I think it sums up what I think about this pretty well, also there's this package that does whatever the article mentioned wdyt

[machi1990]

@machi1990 Sorry, but just to make sure we are on the same page: you are proposing that we use lastSync and limit as parameters to paginate a sync query (instead of offset and limit) or are you proposing that we use offset and limit themselves?

Good question, when we look at it, lastSync is like offset, so what I was proposing is using lastSync, limit and count.

[ssd71]

Thanks! I'll get started on that, but I hope we're also clear on the limitations with that approach :D

[machi1990]

Thanks! I'll get started on that, but I hope we're also clear on the limitations with that approach :D

@ssd1, From top of my head, for the same value of lastSync, I am seeing count being non-static i.e changing all the time, if new data comes in or some modification happens. It's a approach to get started, but maybe not real suitable for real time data. Do you see any other limitations? Do you think we should go this way for a starter and document the limitation or investigate on a far more suitable solution?

[ssd71]

@ssd1, From top of my head, for the same value of lastSync, I am seeing count being non-static i.e changing all the time, if new data comes in or some modification happens. It's a approach to get started, but maybe not real suitable for real time data.

I understand. I was thinking that we were targeting real-time data :P, aren't we?

Do you see any other limitations?

I was thinking that if somehow _lastUpdatedAt will be same for say, 2 documents, and we use a limit of 2, it might not be possible get more than those two documents unless we use _lastUpdatedAt:{ gt: lastSync } (in which case we might miss some of the documents between two pages(say if in the previous example there were 3 documents with same _lastUpdatedAt and a limit of 2 was used) )

Do you think we should go this way for a starter and document the limitation or investigate on a far more suitable solution?

I would like to get started with server-side conflict resolution :P, but I feel that we should do this the proper way, as it is one of the defining features of datasync (the sync part :D). Perhaps I could investigate after I've done server-side conflict resolution, or we could implement a starter solution or we could investigate right now. I guess it depends on where our priorities lie

I would like to get your thoughts on what you think about this

[machi1990]

@ssd1, From top of my head, for the same value of lastSync, I am seeing count being non-static i.e changing all the time, if new data comes in or some modification happens. It's a approach to get started, but maybe not real suitable for real time data.

I understand. I was thinking that we were targeting real-time data :P, aren't we?

Yes, this is correct and the suggested starter approach is not far off really.

Do you see any other limitations?

I was thinking that if somehow _lastUpdatedAt will be same for say, 2 documents, and we use a limit of 2, it might not be possible get more than those two documents unless we use _lastUpdatedAt:{ gt: lastSync } (in which case we might miss some of the documents between two pages(say if in the previous example there were 3 documents with same _lastUpdatedAt and a limit of 2 was used) )

Maybe, that's where count kicks in, to give an indication of how many documents are there and the user can always change the requested limit (it should not be something we hard code, even though providing a good default of like 10 - 50 will be reasonable).

Do you think we should go this way for a starter and document the limitation or investigate on a far more suitable solution?

I would like to get started with server-side conflict resolution :P, but I feel that we should do this the proper way, as it is one of the defining features of datasync (the sync part :D). Perhaps I could investigate after I've done server-side conflict resolution, or we could implement a starter solution or we could investigate right now. I guess it depends on where our priorities lie

I would like to get your thoughts on what you think about this

I'd like to get @wtrocki thoughts on this as it touches priority and how this will be handled on the client side (something we also have to think about).

As for me, implementing the suggested solution will be a good way to start, and possibly opening an issue to investigate other pagination solutions (which I think could be also useful options in some other areas e.g findXXX)

[ssd71]

Maybe, that's where count kicks in, to give an indication of how many documents are there and the user can always change the requested limit (it should not be something we hard code, even though providing a good default of like 10 - 50 will be reasonable).

Hmm sounds good to me 🤔

I'd like to get @wtrocki thoughts on this as it touches priority and how this will be handled on the client side (something we also have to think about).

+1, I think he will be coming back on Thursday though,

As for me, implementing the suggested solution will be a good way to start, and possibly opening an issue to investigate other pagination solutions (which I think could be also useful options in some other areas e.g findXXX)

Agreed, I'll get to doing that now and we can check with him later, or maybe I should work on something else for now ?

[machi1990]

Agreed, I'll get to doing that now and we can check with him later, or maybe I should work on something else for now ?

This is what I think. In the meantime we can try to finish this PR https://github.com/aerogear/graphback/pull/1771 (taking out pagination from the TODO list for the moment).

Once it is done, we can get that merged and focus entirely on pagination and conflicts resolution on separate individual PRs. I feel like the later two issues need more discussion than what we have at the current PR (so let's try to get that's merged, and not block it with discussion on pagination issue). WDYT?

[ssd71]

This is what I think. In the meantime we can try to finish this PR #1771 (taking out pagination from the TODO list for the moment).

Once it is done, we can get that merged and focus entirely on pagination and conflicts resolution on separate individual PRs. I feel like the later two issues needs more discussion than what we have at the current PR (so let's try to get that's merged, and not block it with discussion on pagination issue). WDYT?

+100

[craicoverflow]

I think that offset-based pagination is out of the question here, due to the highly dynamic nature of the data. Look at the following possibility:

1. 100/1000 Documents update after 12:00
2. Developer fetches Note delta where lastSync = 12:00
3. User gets 100 documents.
4. 100 different documents get updated.
5. User navigates to the next page and gets a completely different set of records because the first delta returned has now become older than this new set.

Cursor-based pagination is the only feasible option, and this article linked by @ssd71 summed it up very nicely. Whether we need to use mongo-cursor-pagination I am not sure.

As you guys have said lets hold off on doing anything with this just yet until we get feedback from the entire team.

[wtrocki]

There is no sense to have offset for delta. Just limit that will be protecting us from overflooding with changes on client (thus number of changes will be soooo high that app will crash and we will get inconsistency.

TL;DR find and delta query should be the same apart from the offset which is not needed (Your lastSync is offset/cursor really.

[wtrocki]

As for doing it.. This is just single line of code + adding limit to the schema.
Let's do that in single PR so we will not have 2 separate testing etc.

[wtrocki]

This feature is not pagination - sorry for wrong title.
This is more to limit number of delta to not overflood client with items that can freeze app.
So client can setup delta limit 200 and then update lastUpdated and reissue another query.

This way we will ensure that things are consistent.