Repository navigation
Fix bounded OneDrive search pagination - #10
Conversation
📝 WalkthroughWalkthroughChangesDrive search pagination
Sequence Diagram(s)sequenceDiagram
participant SearchDrive
participant MicrosoftGraph
participant graphCollectionRoute
SearchDrive->>MicrosoftGraph: Request drive search page
MicrosoftGraph-->>SearchDrive: Return items and continuation URL
SearchDrive->>graphCollectionRoute: Validate continuation scope
graphCollectionRoute-->>SearchDrive: Return validated drive search route
SearchDrive->>MicrosoftGraph: Request continuation with remaining $top
MicrosoftGraph-->>SearchDrive: Return next page items
Merge Risk: 🔵 Low · up to Update the continuation expectation before merge so regressions that request more results than remain are caught. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Essentials
Run ID: 6688888a-0c95-4eb0-88ad-2e48331c30f8
📒 Files selected for processing (3)
internal/graphapi/drive.gointernal/graphapi/drive_search_test.gointernal/graphapi/mail_pages.go
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.
Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a03f80-d9ec-7780-9014-8097c4496303
f78fb0b to
823adc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Essentials
Run ID: 26f9a429-b4c0-4898-8627-b1fef287679e
📒 Files selected for processing (2)
internal/graphapi/drive.gointernal/graphapi/drive_search_test.go
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.
| const wantQuery = "$skiptoken=opaque%20cursor&foo=a%2Bb&$top=3" | ||
| if got := req.URL.RawQuery; got != wantQuery { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the remaining result limit in the continuation expectation.
After the first page returns two items, SearchDrive needs one more item. This test currently requires $top=3, so it accepts a continuation request that exceeds the caller's remaining limit. Preserve $skiptoken and foo, but expect $top=1.
Proposed test update
- const wantQuery = "$skiptoken=opaque%20cursor&foo=a%2Bb&$top=3"
+ const wantQuery = "$skiptoken=opaque%20cursor&foo=a%2Bb&$top=1"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const wantQuery = "$skiptoken=opaque%20cursor&foo=a%2Bb&$top=3" | |
| if got := req.URL.RawQuery; got != wantQuery { | |
| const wantQuery = "$skiptoken=opaque%20cursor&foo=a%2Bb&$top=1" | |
| if got := req.URL.RawQuery; got != wantQuery { |
There was a problem hiding this comment.
Not changing this. Microsoft Graph documents @odata.nextLink as opaque, so replacing $top=3 with $top=1 would reintroduce the exact issue fixed from the prior review. SearchDrive enforces the remaining bound locally and returns after collecting the third item.
The requested change was implemented in 823adc7. Graph continuation URLs are now validated and followed without query rewriting, with a regression test for percent-escaped opaque parameters.
Summary
Follow Microsoft Graph continuation pages during OneDrive search while preserving the caller's requested result bound.
What changed
Verification
PATH=/tmp/go1.26.4/bin:$PATH make testPATH=/tmp/go1.26.4/bin:$PATH go test ./internal/graphapiThis supports the person-scoped OneDrive document ingestion work in PlanMonster NanoClaw.