fix: Respect limits on batched messages #90
Conversation
| requests.add(req); | ||
| if (res.pagingInfo.hasLimit() && | ||
| res.envelopes.length >= res.pagingInfo.limit && | ||
| res.pagingInfo.limit < 100) { |
There was a problem hiding this comment.
Flutter sets the default limit to 100 if no limit is set. So tests were failing as it would break out and not cursor further for more than 100 envelopes. This protects against that case. And from someone setting a limit larger than we currently request for.
There was a problem hiding this comment.
Might make sense to leave this as a comment for future maintainers?
Also, should it be <= 100, and > res.pagingInfo.limit?
There was a problem hiding this comment.
No since 100 is the default if we set it to equal it will always catch this case and not cursor for things that aren't limited.
| requests.add(req); | ||
| if (res.pagingInfo.hasLimit() && | ||
| res.envelopes.length >= res.pagingInfo.limit && | ||
| res.pagingInfo.limit < 100) { |
There was a problem hiding this comment.
Might make sense to leave this as a comment for future maintainers?
Also, should it be <= 100, and > res.pagingInfo.limit?
| if (res.pagingInfo.hasLimit() && | ||
| res.envelopes.length >= res.pagingInfo.limit && | ||
| res.pagingInfo.limit < 100) { | ||
| var envelopes = res.envelopes.take(res.pagingInfo.limit).toList(); |
There was a problem hiding this comment.
Why is the server yielding more than limit results in a page? This seems like a bug in the query implementation. There's an inherent ambiguity in batch querying) but the proto definition makes clear that it's a per-query pagination and this lib constructs each QueryRequest within the Batch to include its own limit -- so I'm not sure why the server is ignoring it.
I don't think we should implement this here -- it breaks pagination: since we didn't yield all the received envelopes, the next-page request cursor becomes invalid (this seems like it could cause non-contiguous message histories which become permanent gaps in apps that assume a contiguous history on first pull and then use fetch-since-last-message to incrementally grow the archive). And even if pagination worked, papering over the server-bug with a client-side fix is a bummer because the app still pays the full the price for transport/decode of the full list.
There was a problem hiding this comment.
This for the most part is how it's implemented in JS, Kotlin, and Swift.
https://github.com/xmtp/xmtp-js/blob/main/src/ApiClient.ts#L325-L337
https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/ApiClient.kt#L132-L135
The flutter SDK is written differently than the other SDKs so maybe I'm not translating between them correctly but this does fix it.
I don't think we should block this PR for a improvement to the backend. But agree we should probably open an issue for that as well.
There was a problem hiding this comment.
I don't think that's how they do/should implement batch queries though:
https://github.com/xmtp/xmtp-js/blob/main/src/ApiClient.ts#L441
There was a problem hiding this comment.
The more I dig into this the more I think it's broken. The limit is referred to as pageSize in js which would indicate more of a page size and less of a limit. How many items do you want per page and not how many items do you want period. I still think this is how we want to enforce limit without the backend backing but now that I remember from the last time looking into this I think limit is actually treated like a pageSize on the backend.
|
I'm going to merge this as is but flag that this is somewhere that some investigation/fixing should probably happen on the backend. |
fixes #89
Respects limit when batching messages