feat(client): Add exist_ok ConflictResolver to auto-retry on 409s - #579
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds end-to-end 409 replay support for Changesexist_ok Conflict Replay via 409 GET
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Client as NemoClient.send
participant API as HTTP API
participant Resolver as on_conflict_get
Caller->>Client: create(..., exist_ok=True)
Client->>API: POST create
API-->>Client: 409 Conflict
Client->>Resolver: replay GET request
Resolver-->>Client: PreparedRequest
Client->>API: GET existing entity
API-->>Client: 200 OK
Client-->>Caller: entity
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/nemo_platform_plugin/tests/client/test_client_options.py (2)
182-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an async equivalent for the flat-method-wrapper replay.
TestExistOkViaMethodonly covers the sync path viamethod(CREATE_ITEM_LINKED);TestAsyncExistOkViaSendcovers async but only throughclient.send()directly, not through amethod()-built wrapper onAsyncNemoClient. Given async is a fully supported path per the client layer, an async flat-wrapper test would close the coverage gap.🤖 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 `@packages/nemo_platform_plugin/tests/client/test_client_options.py` around lines 182 - 201, Add an async coverage test for the flat method-wrapper replay path, since `TestExistOkViaMethod` only exercises `method(CREATE_ITEM_LINKED)` on the sync client. Mirror that scenario with `AsyncNemoClient` by defining an async wrapper class using `method()` and verifying `exist_ok=True` on `create_item` retries/replays correctly after a 409, similar to the existing async send-based test.
112-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
methodparam shadows importedmethodhelper.
_resp'smethod: str = "POST"parameter shadows themethodimport fromnemo_platform_plugin.client.method(Line 15) within this function's scope. No functional bug since scope is local, but worth a distinct name (e.g.http_method) for clarity givenmethodis also used as a class-level attribute name inTestExistOkViaMethod.🤖 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 `@packages/nemo_platform_plugin/tests/client/test_client_options.py` around lines 112 - 121, The `_resp` helper in `test_client_options.py` uses a local parameter name that shadows the imported `method` helper, which hurts readability and can be confusing alongside `TestExistOkViaMethod.method`. Rename the `_resp` parameter to something clearer like `http_method`, and update its use inside `_resp` so the helper remains unambiguous while preserving the existing behavior.packages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.py (1)
170-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail fast: validate
exist_okrequires a resolver at decoration time.Today a create endpoint can declare
exist_okwithoutget_on_conflict; the misconfiguration only surfaces as aValueErrorthe first time a real 409 occurs in production (see_should_resolve_conflictin client.py).client_option_namesis already available here — use it to catch this at import time instead.♻️ Proposed early validation
hints = get_type_hints(fn) ret = hints.get("return") response_type = ret if ret is not None and ret is not type(None) else None + + if "exist_ok" in client_option_names and get_on_conflict is None: + raise ValueError( + f"{fn.__name__} declares an 'exist_ok' option but no get_on_conflict " + "resolver was provided; a real 409 would fail instead of retrieving " + "the existing entity. Add get_on_conflict=<resolver> to `@post`(...)." + )🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.py` around lines 170 - 200, The _make_endpoint decorator should fail fast when an endpoint exposes the exist_ok client option but no get_on_conflict resolver was provided. Use the existing client_option_names check in _make_endpoint, alongside _validate_params, to validate this at decoration/import time instead of deferring the error until _should_resolve_conflict is hit in client.py. If the misconfiguration is detected, raise a clear ValueError before returning prepare so create endpoints are configured correctly up front.
🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.py`:
- Around line 404-406: The 409 replay in `send()` drops caller-supplied headers
because the recursive call to `request.on_conflict_get` omits the original
`headers` argument. Update both the sync and async conflict-handling paths in
`Client.send` / the corresponding async send method so the replay passes through
`headers=headers` (or the equivalent merged headers) when calling `self.send` on
`request.on_conflict_get`, preserving caller-provided headers across retries.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.py`:
- Around line 170-200: The _make_endpoint decorator should fail fast when an
endpoint exposes the exist_ok client option but no get_on_conflict resolver was
provided. Use the existing client_option_names check in _make_endpoint,
alongside _validate_params, to validate this at decoration/import time instead
of deferring the error until _should_resolve_conflict is hit in client.py. If
the misconfiguration is detected, raise a clear ValueError before returning
prepare so create endpoints are configured correctly up front.
In `@packages/nemo_platform_plugin/tests/client/test_client_options.py`:
- Around line 182-201: Add an async coverage test for the flat method-wrapper
replay path, since `TestExistOkViaMethod` only exercises
`method(CREATE_ITEM_LINKED)` on the sync client. Mirror that scenario with
`AsyncNemoClient` by defining an async wrapper class using `method()` and
verifying `exist_ok=True` on `create_item` retries/replays correctly after a
409, similar to the existing async send-based test.
- Around line 112-121: The `_resp` helper in `test_client_options.py` uses a
local parameter name that shadows the imported `method` helper, which hurts
readability and can be confusing alongside `TestExistOkViaMethod.method`. Rename
the `_resp` parameter to something clearer like `http_method`, and update its
use inside `_resp` so the helper remains unambiguous while preserving the
existing behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f4f2c71c-1b51-466b-9de0-2209f94d3928
📒 Files selected for processing (6)
packages/filesets/src/filesets/resources.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/types.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/files/endpoints.pypackages/nemo_platform_plugin/tests/client/test_client_options.py
|
- Preserve caller-supplied headers on the 409 conflict-replay GET (sync + async). - Fail fast at decoration time when an endpoint declares exist_ok without a get_on_conflict resolver, instead of raising on the first real 409. - Rename the _resp test helper's shadowing `method` param to `http_method`. - Add async flat-method-wrapper replay coverage + a header-preservation test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
Addressed the review feedback in 045ee87 (all four items): Actionable — dropped headers on conflict replay ✅ Fail fast:
Async flat-method replay coverage ✅ All 66 client tests pass; |
The new decoration-time check (exist_ok requires a get_on_conflict resolver) surfaced that the example plugin's create_item declared exist_ok without one, failing at import. Link it to get_item, mirroring create_fileset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
* feat(client): Add exist_ok ConflictResolver to auto-retry on 409s Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * remove comments Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * lint Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(client): address review feedback on exist_ok - Preserve caller-supplied headers on the 409 conflict-replay GET (sync + async). - Fail fast at decoration time when an endpoint declares exist_ok without a get_on_conflict resolver, instead of raising on the first real 409. - Rename the _resp test helper's shadowing `method` param to `http_method`. - Add async flat-method-wrapper replay coverage + a header-preservation test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(example-plugin): wire get_on_conflict resolver on create_item The new decoration-time check (exist_ok requires a get_on_conflict resolver) surfaced that the example plugin's create_item declared exist_ok without one, failing at import. Link it to get_item, mirroring create_fileset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> --------- Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Restores
exist_ok=Trueon create endpoints (starting withcreate_fileset) so a caller gets the existing entity back on conflict, without a manual follow-up GET:Fixes AIRCORE-866.
Background
exist_okwas previously removed (commit1a78606e48) because it was inert. The old implementation swallowed theConflictErrorand parsed the 409 response body as the entity — i.e. it assumed the server returns the entity on 409 (Option A). The real server returns{"detail": "... already exists"}, somodel_validate(raw.json())would have failed on a genuine conflict; it only ever "worked" in tests that mocked entity-shaped 409 bodies.Rather than change every server endpoint to return the entity on 409 (non-standard — RFC 9110 treats the 409 body as diagnostic), this handles
exist_okentirely client-side: on a 409, the client replays a linked GET and returns that entity. This is exactly whatFilesetsSubResource.createdid by hand via try/except + GET — lifted into the client so every consumer gets it declaratively.How it works
A create endpoint declares a
get_on_conflictresolver — a named, typed function that builds the retrieve request:bodymodel still exists, before it's serialized to bytes) and stashes the resulting GET onPreparedRequest.on_conflict_get.send()(sync + async): on409 + exist_ok, it replayson_conflict_getand returns that entity instead of raisingConflictError. Withoutexist_ok, a 409 still raises as before.exist_okset on an endpoint with no resolver raises a clearValueError(not a parse crash).NemoResponse.bodystays non-nullable (T) — noT | Nonetax on other callers.Correctness is enforced by the type checker: the resolver returns
get_fileset(name=..., workspace=...), a real typed endpoint call, so a wrong kwarg/type is caught statically at the call site.Changes
client/types.pyConflictResolverProtocol +PreparedRequest.on_conflict_getfieldclient/endpoint.py@post(..., get_on_conflict=...); resolver invoked at request-build timeclient/client.py_should_resolve_conflicthelper; sync + asyncsend()replay the linked GET on 409 +exist_okfiles/endpoints.pycreate_filesetgainsexist_ok+get_on_conflict=_get_fileset_on_conflictfilesets/resources.pycreate; passexist_okthroughtests/client/test_client_options.py{"detail": ...}409 body + follow-up GETTesting
exist_okreturns the entity via auto-GET, 409 withoutexist_okraisesConflictError, non-409 passthrough, missing-resolverValueError, GET-404-after-409 surfaced, the flatclient.create_item(exist_ok=True)method path, and async twins.packages/nemo_platform_plugin/tests/client/— 64 passed; full plugin suite — 871 passed.tyandruffclean on changed code.Out of scope
Idempotency-Key, orPUTcreate-or-replace) — the standards-aligned path if we ever change the server; deferred.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
exist_okis enabled (sync and async).exist_okconflict replay.Bug Fixes
exist_ok=Truenow returns the existing resource on 409 instead of requiring manual conflict handling.exist_okis enabled without configuring conflict replay, a clear error is raised.Tests