fix: raise NotFoundError on ambiguous 404 responses - #755
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #755 +/- ##
==========================================
+ Coverage 95.55% 96.10% +0.54%
==========================================
Files 45 45
Lines 5152 5105 -47
==========================================
- Hits 4923 4906 -17
+ Misses 229 199 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
`.get()` previously collapsed every 404 into `None` via `catch_not_found_or_throw`. That works for direct, ID-identified fetches (`client.dataset(id).get()`), where a 404 unambiguously means the named resource is missing. It's misleading for chained calls that target a default sub-resource without an ID (`run.dataset()`, `run.key_value_store()`, `run.request_queue()`, `run.log()`): a 404 there could mean the parent run is missing or the default sub-resource is missing, and the API body cannot disambiguate the two. This change keeps the `None` behavior for ID-identified clients and propagates `NotFoundError` from chained clients (`self._resource_id is None`). The v3 upgrade guide is updated to document the new semantics, and sync/async tests cover both code paths.
84e57c0 to
8b6773d
Compare
Pijukatel
left a comment
There was a problem hiding this comment.
Should this also be applied to more than just GET methods?
For example DELETE?
Also there are probably more such endpoints where this could be applied, for example ScheduleClient.get_log
There was a problem hiding this comment.
Also could you please test the special cases like
client.actor('actor-id').last_run().dataset().get()
Where cases should be:
- missing actor -> raise 404
- existing actor, but missing last run -> raise 404
- existing actor, existing run, missing dataset -> return None
- everything exists -> return dataset (probably already tested somewhere)
Good point. Updated, now there is the same guard for the
I let claude audit every call of Here is the summary of updates:
These all hit a fixed path like Left
Added tests:
|
A 404 collapses into `undefined` only where it names one resource: a direct
`get()` or `delete()` on a client created with an ID, and the record lookups
`getRecord()` and `getRequest()`. Where it could mean either the parent or
the sub-resource is missing, the `ApifyApiError` now propagates.
- Chained clients without an ID (`run.dataset()`, `run.keyValueStore()`,
`run.requestQueue()`, `run.log()`, `build.log()`): `get()`, `delete()`,
`log().get()` and `log().stream()` throw. `client.log(id)` still resolves
to `undefined`.
- Singleton sub-path endpoints: `DatasetClient.getStatistics()`,
`UserClient.monthlyUsage()`, `UserClient.limits()`,
`ScheduleClient.getLog()`, `TaskClient.getInput()` and
`WebhookClient.test()` throw, and their return types drop `| undefined`.
- `StreamedLog` logs a warning and stops when the run log answers 404,
instead of leaving its background task rejected.
The mock server's text handler now sends the status code it derives from
the resource ID, so `run('404').log().get()` can be exercised.
Mirrors apify/apify-client-python#755. Closes #1030.
BREAKING CHANGE: `run.dataset()`, `run.keyValueStore()`,
`run.requestQueue()`, `run.log()` and `build.log()` throw an `ApifyApiError`
on a 404 instead of resolving to `undefined`. `DatasetClient.getStatistics()`,
`UserClient.monthlyUsage()`, `UserClient.limits()`, `ScheduleClient.getLog()`,
`TaskClient.getInput()` and `WebhookClient.test()` throw on a 404 and no
longer include `undefined` in their return types.
A 404 collapsed into `undefined` everywhere, including where it can't be pinned to one resource: `run.dataset().get()` couldn't tell a missing run from a missing dataset. Those calls now throw. - Chained clients without an ID (`run.dataset()`, `run.keyValueStore()`, `run.requestQueue()`, `run.log()`, `build.log()`) throw from `get()`, `delete()`, `log().get()` and `log().stream()`. A new `catchNotFoundForResourceOrThrow(err, this.id)` keys on the ID, so ID-addressed clients still resolve to `undefined`. - Fixed sub-paths throw and drop `| undefined`: `getStatistics()`, `monthlyUsage()`, `limits()`, `getLog()`, `getInput()`, `test()`. - Unchanged: `getRecord()`, `getRequest()`, `recordExists()` and `lastRun()`. - `version()`, `build()` and `envVar()` reject an empty string, which used to address the collection. - `UserClient.get()` is now `Promise<User | undefined>`, matching what it always returned. - `getStreamedLog()` on a missing run warns and stops. Merging `v3` also moved `LogClient.stream()` under the same rule. #1041 made `catchNotFoundOrThrow()` a plain `instanceof NotFoundError` check, so a streamed 404 matches it and `client.log(id).stream()` resolves to `undefined` the way `get()` does. #1043 raised that flip as an open question; the unusable error body it reports is still open. Mirrors apify/apify-client-python#755. Closes #1030 *✍️ Drafted by Claude Code*
Follow-up to #737 (comment)
.get()previously collapsed every 404 intoNoneviacatch_not_found_or_throw. That works for direct, ID-identified fetches (client.dataset(id).get()), where a 404 unambiguously means the named resource is missing. It's misleading whenever a 404 is ambiguous — the client can't tell which resource in the path is actually gone, and silently returningNonehides the cause.This PR identifies three categories where a 404 is ambiguous and propagates
NotFoundErrorinstead:run.dataset(),run.key_value_store(),run.request_queue(),run.log()) — a 404 could mean either the parent run is missing or the default sub-resource. Covered by the base_get/_deletevia aresource_id is None → raiseguard, so.delete()on a chained client also raises now; directdataset(id).delete()keeps its idempotent-DELETE semantics.LogClient—run.log().get()/.get_as_bytes()/.stream()raise; directclient.log(id).get()still returnsNone.ScheduleClient.get_log,TaskClient.get_input,DatasetClient.get_statistics,UserClient.monthly_usage,UserClient.limits,WebhookClient.test. These hit a fixed path (/.../{id}/log,/.../{id}/input, etc.) where a 404 effectively always means the parent is missing. Return types moved fromT | NonetoT.Record-by-key lookups (
KeyValueStoreClient.get_record(key),RequestQueueClient.get_request(request_id)) keep the existing "None on missing" behavior — the 404 is specifically about the record/request, which is the natural meaning.A shared helper
catch_not_found_for_resource_or_throw(exc, resource_id)in_utils.pycentralizes theresource_id is None → raisepattern across all 10 call sites. The v3 upgrade guide documents the new semantics. Sync/async tests cover every code path, includingclient.actor('id').last_run().dataset().get()(happy path + ambiguous-404 case).