feat: add native async STAC resolver - #16
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe package adds asynchronous STAC pagination and CID resolution with event-loop-scoped HTTP client pooling. Dataset loading now awaits the async resolver. The APIs are exported, documented, versioned, and covered by asynchronous tests. ChangesAsynchronous STAC resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant load_dataset
participant aresolve_cid_from_stac_server
participant AsyncClient
participant STACServer
load_dataset->>aresolve_cid_from_stac_server: Await CID resolution
aresolve_cid_from_stac_server->>AsyncClient: Send paginated search
AsyncClient->>STACServer: Send POST or GET STAC request
STACServer-->>AsyncClient: Return feature page or next link
aresolve_cid_from_stac_server-->>load_dataset: Return resolved CID
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
==========================================
+ Coverage 74.79% 75.56% +0.76%
==========================================
Files 16 16
Lines 1956 2050 +94
Branches 363 374 +11
==========================================
+ Hits 1463 1549 +86
- Misses 344 350 +6
- Partials 149 151 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dclimate_client_py/stac_server.py (1)
26-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose pooled async clients when their event loop ends.
dClimateClient.__aexit__does not callaclose_stac_server_client(). Directaresolve_cid_from_stac_servercalls also have no automatic cleanup. Add cleanup to the lifecycle and document the requirement for direct callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/stac_server.py` around lines 26 - 50, Update dClimateClient.__aexit__ to call aclose_stac_server_client() so pooled async clients are closed when the client lifecycle ends. Document that direct aresolve_cid_from_stac_server callers must explicitly invoke aclose_stac_server_client() after use, preserving existing pooling behavior.
🧹 Nitpick comments (1)
dclimate_client_py/stac_server.py (1)
267-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_asearch_pagesduplicates_search_pagesalmost line for line.Both functions repeat the page-key dedupe, the request-kwargs assembly, the
rel=nextlookup, the method allowlist, and the merge semantics. The async copy also drops the explanatory comments that document the STAC merge contract and the page-limit rationale. The two paths can diverge silently, and any pagination fix must be applied twice.Extract the pure planning logic into shared helpers and keep only the transport call in each function. For example, extract
_page_key(method, url, body, headers),_request_kwargs(method, body, headers, timeout), and_next_request(url, body, page)that returns the next(url, method, body, headers)tuple orNone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/stac_server.py` around lines 267 - 342, The pagination logic in _asearch_pages duplicates _search_pages and can diverge. Extract shared pure helpers for page-key generation, request-kwargs construction, and next-request planning, including the existing deduplication, method allowlist, STAC merge semantics, explanatory comments, and page-limit behavior; then update both functions to use them so each retains only its respective sync or async transport call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dclimate_client_py/stac_server.py`:
- Around line 322-327: Update the pagination logic in _search_pages and
_asearch_pages to validate each next_link against server_url, rejecting links
whose origin or scheme differs. Only forward linked headers for trusted HTTPS
destinations, removing credential headers when the destination is untrusted or
plaintext; preserve the existing GET/POST method validation and request flow for
valid links.
In `@tests/test_stac_server_async.py`:
- Around line 100-102: Update the test setup around
stac_server._ASYNC_HTTP_CLIENTS.clear() to close every existing pooled async
client with aclose() before removing registry references. Ensure cleanup is
awaited and guaranteed even when prior tests populated the registry, while
preserving the existing client_factory monkeypatch.
---
Outside diff comments:
In `@dclimate_client_py/stac_server.py`:
- Around line 26-50: Update dClimateClient.__aexit__ to call
aclose_stac_server_client() so pooled async clients are closed when the client
lifecycle ends. Document that direct aresolve_cid_from_stac_server callers must
explicitly invoke aclose_stac_server_client() after use, preserving existing
pooling behavior.
---
Nitpick comments:
In `@dclimate_client_py/stac_server.py`:
- Around line 267-342: The pagination logic in _asearch_pages duplicates
_search_pages and can diverge. Extract shared pure helpers for page-key
generation, request-kwargs construction, and next-request planning, including
the existing deduplication, method allowlist, STAC merge semantics, explanatory
comments, and page-limit behavior; then update both functions to use them so
each retains only its respective sync or async transport call.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ee2a6a1-5a34-4cf4-b18a-ca7406000f32
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
README.mddclimate_client_py/__init__.pydclimate_client_py/dclimate_client.pydclimate_client_py/stac_server.pypyproject.tomltests/conftest.pytests/test_stac_server_async.py
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
dclimate_client_py/dclimate_client.py (2)
198-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence BLE001 or confirm the rule is not enforced.
Ruff reports BLE001 on the three
except BaseExceptionclauses. The broad catches are intentional here, because cleanup must continue and_merge_cleanup_errorkeeps cancellation dominant. If CI runs Ruff with BLE001 enabled, the lint step fails. Add a targeted# noqa: BLE001on each clause, or confirm the rule is disabled in the project configuration.♻️ Proposed change
- except BaseException as error: + except BaseException as error: # noqa: BLE001 - cleanup must continue cleanup_error = _merge_cleanup_error(cleanup_error, error)Also applies to: 206-206, 212-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/dclimate_client.py` at line 198, Add a targeted `# noqa: BLE001` annotation to each of the three intentional `except BaseException` clauses in the cleanup logic, including the clauses near `_merge_cleanup_error`, so Ruff accepts the broad catches without changing their cancellation-dominant behavior.Source: Linters/SAST tools
229-235: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSSRF Via Unrestricted HTTP Redirects (CWE-918): Server-Side Request Forgery (SSRF)
Reachability: Internal · Exploitability: Difficult
Reachability path
● Entry tests/test_review_bugs_client.py │ ▼ ● Sink dclimate_client_py/dclimate_client.pyRestrict STAC redirects to the configured origin.
follow_redirects=Trueallows the initial/searchrequest to reach another host before_next_search_requestvalidates pagination links. Setfollow_redirects=Falseor validate the final response origin. The STAC client sends no credentials, but it can still accept results from an unexpected host.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/dclimate_client.py` around lines 229 - 235, Update _get_stac_http_client to disable automatic redirects by setting follow_redirects to false, ensuring STAC requests cannot reach another host before pagination-link validation in _next_search_request.dclimate_client_py/stac_server.py (1)
271-273: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider surfacing unsupported next-link methods.
An unsupported
methodstops pagination and returns partial results silently. The page-limit branch raisesValueErrorfor the same outcome, so callers can fall back to the catalog. Align the two paths, or add a comment that records the intentional difference.♻️ Proposed change
method = str(next_link.get("method", "GET")).upper() if method not in {"GET", "POST"}: - return None + raise ValueError( + f"STAC pagination link uses an unsupported method: {method!r}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/stac_server.py` around lines 271 - 273, Update the next-link method handling in the pagination logic to raise ValueError for methods outside GET and POST, matching the existing page-limit failure behavior and allowing callers to fall back to the catalog. Keep supported-method handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dclimate_client_py/dclimate_client.py`:
- Line 198: Add a targeted `# noqa: BLE001` annotation to each of the three
intentional `except BaseException` clauses in the cleanup logic, including the
clauses near `_merge_cleanup_error`, so Ruff accepts the broad catches without
changing their cancellation-dominant behavior.
- Around line 229-235: Update _get_stac_http_client to disable automatic
redirects by setting follow_redirects to false, ensuring STAC requests cannot
reach another host before pagination-link validation in _next_search_request.
In `@dclimate_client_py/stac_server.py`:
- Around line 271-273: Update the next-link method handling in the pagination
logic to raise ValueError for methods outside GET and POST, matching the
existing page-limit failure behavior and allowing callers to fall back to the
catalog. Keep supported-method handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2107b1f5-6b4c-446c-a690-2f3f3cefdb8c
📒 Files selected for processing (6)
dclimate_client_py/dclimate_client.pydclimate_client_py/stac_server.pytests/test_review_bugs_client.pytests/test_review_fu_variant.pytests/test_review_perf_async.pytests/test_stac_server_async.py
|
Addressed the substantive review-body feedback in f9b258c as well: sync and async pagination now share the same request-planning/security helpers, and the resolver docstring documents ownership for both pooled and injected clients. The high-level client uses a per-instance injected STAC transport with explicit async-context cleanup. Full local validation: 245 passed, 45 skipped; Ruff formatting/lint, mypy, uv lock check, and git diff checks also pass. |
|
Follow-up review triage is complete in 58ee6e8. STAC requests now disable redirects per request, so the protection also applies when a caller injects a client configured to follow redirects; the owned sync/async client defaults are disabled too. Unsupported next-link methods now raise ValueError so callers can fall back instead of accepting partial pagination. Added regressions for both behaviors. I did not add BLE001 suppressions because BLE001 is not enabled by this repository and the complete Ruff check passes. Updated validation: 247 passed, 45 skipped; formatting, Ruff, mypy, uv lock, and diff checks pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
11-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCommit the EOF normalization.
The
end-of-file-fixerhook modifiedAGENTS.mdin CI. Remove the extra blank line after the last bullet and keep one terminating newline, then rerun pre-commit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 11 - 12, Normalize AGENTS.md so the document ends immediately after the last bullet with exactly one terminating newline and no extra blank line, then rerun the pre-commit checks to confirm the end-of-file-fixer hook is clean.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@AGENTS.md`:
- Around line 11-12: Normalize AGENTS.md so the document ends immediately after
the last bullet with exactly one terminating newline and no extra blank line,
then rerun the pre-commit checks to confirm the end-of-file-fixer hook is clean.
| # extends the original request (keeping filters like "collections"); | ||
| # otherwise it replaces it wholesale. | ||
| request_body = ( | ||
| {**original_body, **linked_body} if next_link.get("merge") else linked_body |
There was a problem hiding this comment.
MEDIUM
The STAC link contract applies merge: true to the current request body, but this always merges into the initial body. In a multi-page chain, state introduced by an earlier continuation link is therefore dropped, potentially querying the wrong page or missing the target. Pass the current request_body into the planner; the sibling JS port needs the equivalent fix.
| for _ in range(_MAX_SEARCH_PAGES): | ||
| page_key = _search_page_key(method, url, request_body, request_headers) | ||
| if page_key in seen: | ||
| return |
There was a problem hiding this comment.
MEDIUM
A repeated request key means a next link was present but pagination could not complete. Returning here lets the resolver select from truncated results—for example, choosing a non-default variant before the loop—instead of activating the catalog fallback. Raise ValueError, as the page-limit and unsupported-method paths do.
| # extends the original request (keeping filters like "collections"); | ||
| # otherwise it replaces it wholesale. | ||
| request_body = ( | ||
| {**original_body, **linked_body} if next_link.get("merge") else linked_body |
There was a problem hiding this comment.
MEDIUM
Merge into the current request body. STAC defines merge: true against the current body; using the initial body drops continuation state introduced by earlier pages, potentially repeating or skipping pages. Pass the current request_body to the planner.
| for _ in range(_MAX_SEARCH_PAGES): | ||
| page_key = _search_page_key(method, url, request_body, request_headers) | ||
| if page_key in seen: | ||
| return |
There was a problem hiding this comment.
MEDIUM
Treat repeated requests as incomplete pagination. Returning here allows resolution from a truncated feature set instead of activating the catalog fallback. Raise ValueError, consistent with the page-limit path.
| dataset: Dataset name (e.g., 'temperature', 'precipitation') | ||
| variant: Optional variant name (e.g., 'ensemble', 'deterministic') | ||
| server_url: STAC server base URL | ||
| if not (page.get("features") or []): |
There was a problem hiding this comment.
MEDIUM
Follow rel=next even when a page is empty. STAC pagination permits an empty features page with a continuation link. Returning here can miss a later matching or preferred variant; determine termination from the next link instead, including in the sync path.
3 participants
What changed
Why
Async consumers previously had to run the synchronous resolver in a worker thread. Owning the async transport in the client library gives callers a non-blocking API and connection reuse without duplicating this integration in each application.
Impact
The existing synchronous resolver remains available and its selection behavior is unchanged. Async callers can use the new resolver directly, and the high-level dataset loader now uses it internally. This change does not include STAC service-side fixes.
Validation
Summary by CodeRabbit