Refuse a page size that is not a page - #159
Merged
Merged
Conversation
Conversation::messages passes `limit` straight through to SQLite as `LIMIT ?`, where a negative value means no limit at all and zero means the end of the history. So a caller asking for -1 messages gets every message in the conversation loaded into memory, and one asking for 0 gets an empty page that looks like the end of the history rather than an error. The parameter was undocumented, so neither reading was wrong from the caller's side: the header explains `before` and `include_deleted` at length and said nothing about the range of `limit`. _require_page joins the other _require_ guards on the calling thread, where a caller bug surfaces at the call site with a stack still under it rather than as a string in an error leg. The two terminal overloads are the only callers of _messages, so guarding them covers all eight public spellings; the defaulted ones pass 50 and are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Conversation::messagespasseslimitstraight through to SQLite asLIMIT ?, where a negative value means no limit at all and zero means the end of the history. A caller asking for -1 messages gets every message in the conversation loaded into memory; one asking for 0 gets an empty page that reads as the end of the history rather than an error.The parameter was undocumented, so neither reading was wrong from the caller's side — the header explains
beforeandinclude_deletedat length and said nothing about the range oflimit.What this does
_require_pagejoins the other_require_*guards on the calling thread, where a caller bug surfaces at the call site with a stack still under it, rather than as a string in an error leg.The two terminal overloads are the only callers of
_messages, so guarding them covers all eight public spellings. The defaulted overloads pass 50 and are unaffected.The header now states the contract, including that there is deliberately no "no limit" spelling: a conversation's history has no upper bound, so a caller that wants all of it pages.
Testing
New case
Client: a page size has to be a pagecovers 0, -1 and -50 across both the waiting and handler forms, with a positive control.[client]is green: 1006 assertions in 134 test cases.Note for consumers
This turns a silent unbounded load into a refusal, so anything currently passing a non-positive limit and relying on getting everything back will start throwing. Nothing in-tree did.