feat: align async STAC resolver with Python client - #12
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe client now uses shared, bounded STAC pagination for CID resolution and dataset discovery. It validates continuation requests, supports GET and POST pages, handles redirects manually, exposes related APIs and types, updates documentation and version metadata, and adds parity and pagination tests. ChangesSTAC pagination and public API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant resolveCidFromStacServer
participant searchPages
participant STACServer
Client->>resolveCidFromStacServer: request CID resolution
resolveCidFromStacServer->>searchPages: start paginated search
searchPages->>STACServer: send validated search request
STACServer-->>searchPages: return features and continuation link
searchPages->>STACServer: request validated next page
STACServer-->>searchPages: return additional features
searchPages-->>resolveCidFromStacServer: return collected features
resolveCidFromStacServer-->>Client: return resolved CID
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| yield page; | ||
|
|
||
| const features = Array.isArray(page.features) ? page.features : []; | ||
| if (features.length === 0) return; |
There was a problem hiding this comment.
MEDIUM
Returning when a page has no features ignores an explicit rel="next" link. An empty intermediate page therefore prevents later pages from being fetched, causing false “not found” errors and incomplete catalog listings. Treat the absence of a next request—not an empty feature array—as the terminal condition.
There was a problem hiding this comment.
Addressed in 18e5a21 by making the absence of a valid next request the pagination terminator, regardless of whether the current page is empty. Added a regression test with an empty second page linking to a non-empty third page; all 27 focused STAC tests, lint, and both builds pass.
| stableJson(request.body), | ||
| stableJson(request.headers), | ||
| ].join("\n"); | ||
| if (seen.has(pageKey)) return; |
There was a problem hiding this comment.
MEDIUM
A repeated request is treated as successful completion. This return lets callers consume a partial feature set when a malformed next link cycles: CID resolution can report “not found” or select the wrong variant, while catalog listing returns incomplete data without triggering its fallback. Throw a pagination/truncation error instead.
There was a problem hiding this comment.
Addressed in f569492. A repeated pagination request key now throws an explicit truncation error instead of returning accumulated partial features. The regression verifies the resolver rejects after the first request; all 29 focused STAC tests, lint, and both builds pass.
| ? { "Content-Type": "application/json", ...request.headers } | ||
| : request.headers, | ||
| ...(request.method === "POST" | ||
| ? { body: JSON.stringify(request.body ?? null) } |
There was a problem hiding this comment.
MEDIUM
Do not serialize an absent POST continuation body as null. A POST next link may paginate solely through its URL or headers, leaving request.body undefined; this then sends literal JSON null, although STAC search bodies are objects, so compliant servers may reject the continuation. Send {} or omit the body when absent. STAC pagination specification
There was a problem hiding this comment.
Addressed in f569492. A POST continuation without a body now sends an empty JSON object instead of literal null, preserving a valid STAC search body for URL- or header-only cursors. Regression coverage passes with all 29 focused STAC tests, lint, and both builds.
| for (let pageNumber = 0; pageNumber < MAX_STAC_SEARCH_PAGES; pageNumber++) { | ||
| const pageKey = [ | ||
| request.method, | ||
| request.url, |
There was a problem hiding this comment.
MEDIUM
pageKey uses the raw link URL, but fragments are never sent in HTTP requests. Links such as /search#1, /search#2, etc. therefore trigger up to 50 identical requests while appearing unique to seen. Build the key from the final request URL and remove its fragment before checking for repetition.
There was a problem hiding this comment.
Addressed in 8856f7c. The repetition key now uses the final request URL after GET parameter expansion and fragment removal, and POST bodies are keyed exactly as transmitted. A regression cycles fragment-only next links and verifies the duplicate request is rejected; all 30 focused STAC tests, lint, and both builds pass.
| function stringHeaders(value: unknown): StacSearchHeaders { | ||
| if (!isRecord(value)) return {}; | ||
| const entries = Object.entries(value); | ||
| if (!entries.every(([, headerValue]) => typeof headerValue === "string")) { |
There was a problem hiding this comment.
MEDIUM
STAC 1.1 Link headers permit values to be strings or arrays of strings. This all-or-nothing check drops every continuation header when any value is an array, so header-based cursors can fetch the wrong page and end in a truncation error. Normalize valid arrays and reject only invalid entries.
There was a problem hiding this comment.
Addressed in 54a2c9b. Continuation headers now accept strings and arrays of strings, join valid arrays using the standard comma-separated representation, and discard only invalid entries while preserving valid siblings. The regression covers all three cases. I also updated the live grouped FPAR fixture to select its advertised Zarr group 0; all 213 tests, lint, and both builds pass.
| } | ||
|
|
||
| function requestUrl(request: StacSearchRequest): string { | ||
| const url = new URL(request.url); |
There was a problem hiding this comment.
MEDIUM
Relative STAC server URLs now fail before the first request. Browser callers could previously use a same-origin URL such as /stac, which fetch resolves normally. new URL(request.url) has no base and throws for /stac/search, breaking both resolution and catalog listing. Resolve relative configuration against location.href before pagination and origin validation.
There was a problem hiding this comment.
Addressed in 1020b87. STAC server URLs are now resolved against location.href when a browser supplies one, before request construction and same-origin validation. Catalog listing reuses the same resolved base for /collections. Regressions cover /stac, a relative next link, and both catalog endpoints; all 216 tests, lint, and both builds pass.
| parsedNextUrl.password !== "" | ||
| ) { | ||
| throw new Error( | ||
| `STAC pagination link must use the configured server origin ${normalizedOrigin(serverUrl)}: ${nextUrl}` |
There was a problem hiding this comment.
LOW
Rejected pagination links expose embedded credentials. nextUrl retains username, password, and query tokens, so interpolating the full URL into the exception can disclose them through application error handling or logs. Report a sanitized URL with userinfo and sensitive query data removed.
There was a problem hiding this comment.
Addressed in 1020b87. Rejected pagination URLs are now sanitized before interpolation: userinfo, the complete query string, and fragments are removed while the safe origin/path remain useful for diagnosis. The regression verifies the error contains only https://attacker.example/collect; all 216 tests, lint, and both builds pass.
1 participant
What changed
Why
The JavaScript client was already asynchronous and already used the resolver internally, so Python event-loop pooling and explicit client cleanup do not apply. However, the resolver was not exported from the package root, catalog listing stopped after one page, and pagination did not have the request hardening added in dClimate/dclimate-client-py#16.
Impact
Consumers can resolve a CID directly through the public package API. Existing high-level loading behavior is unchanged. Resolver and listing pagination now have the same trust boundary and bounded-walk behavior as the Python client, adapted to the platform-owned
fetchtransport.Validation
npm run lint: passednpm run build: browser and Node builds passedSummary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores