Skip to content

Commit ee1c90e

Browse files
committed
fix: address review feedback on batch_add_requests rework
- Correct dedup_key's doc comment: the API's keyless fallback dedups by the raw url, not a "normalized" one. - batch_add_requests now rejects an empty requests slice with InvalidArgument, matching batch_delete_requests and the reference client (both validate non-empty input). - Use the imported UnprocessedRequest name instead of the fully-qualified crate::models:: path in the retry loop's fail-safe return. - Replace a vacuous assert!(limit >= 0) in the lock-lifecycle test with a real invariant: filtering on both locked+pending must match the unfiltered listing.
1 parent 104b505 commit ee1c90e

3 files changed

Lines changed: 22 additions & 8 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ to [Semantic Versioning](https://semver.org/).
1919
(was `serde_json::Value`), and rejects more than 25 requests per call with
2020
`ApifyClientError::InvalidArgument` instead of forwarding an oversized payload to the API
2121
(matching the reference client, which validates rather than auto-chunks deletes). **Breaking.**
22+
- `RequestQueueClient::batch_add_requests` now also rejects an empty `requests` with
23+
`ApifyClientError::InvalidArgument` (was `Ok` with an empty result), matching
24+
`batch_delete_requests` and the reference client, which validates both as non-empty.
25+
**Breaking.**
2226
- `RequestQueueClient::list_and_lock_head` now returns the typed `LockedRequestQueueHead` (was
2327
`serde_json::Value`). **Breaking.**
2428
- `RequestQueueClient::list_requests` now returns the typed `RequestQueueRequestsPage` (was

src/clients/request_queue.rs

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ pub struct BatchAddRequestsOptions {
6868

6969
/// Returns the key used to correlate a request across the batch-add retry loop: its explicit
7070
/// `unique_key` if set, otherwise its `url` — matching the API's own fallback (a request added
71-
/// without a `unique_key` is deduplicated by its normalized URL).
71+
/// without a `unique_key` is deduplicated by its raw `url`).
7272
fn dedup_key(request: &RequestQueueRequest) -> &str {
7373
request.unique_key.as_deref().unwrap_or(&request.url)
7474
}
@@ -327,15 +327,20 @@ impl RequestQueueClient {
327327
/// Unlike most methods here, this does not propagate per-chunk API errors: a chunk that fails
328328
/// even after retries has its requests reported in the result's `unprocessed_requests`
329329
/// instead, so a batch add of many requests never fails outright over one bad chunk (matching
330-
/// the reference client). A [`ApifyClientError::InvalidArgument`] is still returned before any
331-
/// request is sent if a single request's JSON is too large to ever fit in a chunk.
330+
/// the reference client). It still returns [`ApifyClientError::InvalidArgument`] up front,
331+
/// before any request is sent, for an empty `requests` (matching
332+
/// [`batch_delete_requests`](Self::batch_delete_requests) and the reference client, which
333+
/// validates both as non-empty) or if a single request's JSON is too large to ever fit in a
334+
/// chunk.
332335
pub async fn batch_add_requests(
333336
&self,
334337
requests: &[RequestQueueRequest],
335338
options: BatchAddRequestsOptions,
336339
) -> ApifyClientResult<BatchRequestsOperationResult> {
337340
if requests.is_empty() {
338-
return Ok(BatchRequestsOperationResult::default());
341+
return Err(ApifyClientError::InvalidArgument(
342+
"RequestQueueClient::batch_add_requests requires at least 1 request".to_string(),
343+
));
339344
}
340345
let max_parallel = options
341346
.max_parallel
@@ -448,7 +453,7 @@ impl RequestQueueClient {
448453
processed_requests: processed,
449454
unprocessed_requests: remaining
450455
.iter()
451-
.map(|r| crate::models::UnprocessedRequest {
456+
.map(|r| UnprocessedRequest {
452457
unique_key: dedup_key(r).to_string(),
453458
url: r.url.clone(),
454459
method: r.method.clone(),

tests/request_queue.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -360,9 +360,14 @@ async fn request_queue_lock_lifecycle() {
360360
})
361361
.await
362362
.expect("list requests with filter");
363-
// The filter may legitimately exclude every request; the call succeeding (no parse/API
364-
// error) is what this test exercises.
365-
assert!(filtered.limit >= 0);
363+
// The single request we added is either `locked` or `pending` (every request is one or the
364+
// other), so filtering on both states must return exactly the same set as the unfiltered
365+
// listing above — a real invariant, not just "the call didn't error".
366+
assert_eq!(
367+
filtered.items.len(),
368+
listed.items.len(),
369+
"filtering on both locked and pending must match the unfiltered listing"
370+
);
366371

367372
// Lazily paginate requests; we added one, so at least one should be yielded.
368373
let mut iter = queue_client.paginate_requests(Some(10));

0 commit comments

Comments
 (0)