refactor!: package split + issue #8 review fixes; sync spec to v2-2026-07-20T094852Z#10
Merged
Merged
Conversation
…6-07-20T094852Z Addresses the expert review at #8: splits the single com.apify.client package into resource-scoped sub-packages, renames the replaceable HTTP transport (HttpBackend -> HttpTransport), reworks the exception hierarchy (new ApifyClientException base, public ApifyTransportException, backend-agnostic HttpTimeoutException), makes ResourceContext/DatasetClient/KeyValueStoreClient immutable, centralizes resource path segments into ApiPaths, removes setStatusMessage() and the public getUserAgent()/getApiBaseUrl(), makes brotli opt-in and adds slf4j logging. Several review suggestions are explicitly skipped where they would conflict with the reference-client naming/behavior parity requirement (see notes.md); documented with rationale. Bumps to 0.4.0 (breaking) and syncs Version.API_SPEC_VERSION to v2-2026-07-20T094852Z (response-only spec delta, no behavior change needed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
t-tooling
…estore setStatusMessage Fixes the headline correctness bug (ApifyApiException now extends ApifyClientException) and a PaginationList NPE on "items": null. Hides implementation plumbing behind a non-exported com.apify.client.internal package with a module-info.java, restores ApifyClient.setStatusMessage (the prior removal was based on a false claim about the reference client), and removes the duplicated AI-disclaimer from the 10 docs pages. Also covers the accumulated code-quality, docs, changelog, test-coverage, parity-gap and issue #8 follow-ups from the review; see notes.md "Implementer — loop 2" for the itemized list and the explicit skip-with-justification entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
…ns, log-streaming call(), DRY collection clients Closes every item from the loop-2 review's remaining-actionable list: batchAddRequests now also splits by payload byte size (not just count) to avoid 413s; RequestQueueClient's lock/list operations return typed models instead of raw JsonNode; Schedule/Webhook/Task/ActorRun gained typed getters for fields previously reachable only via getExtra(); new ActorCallOptions/ TaskCallOptions call() overloads stream the run's log by default, matching the reference client; extracted AbstractCollectionClient to de-duplicate the 8 collection clients' list/iterate boilerplate; plus the requested docs completions and style nits. See notes.md "Implementer — loop 3" in apify-client-orchestration for the itemized list and test evidence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
…ver-throws, parity getters, call() DRY Closes every item from the loop-3 review's residual list: removed the duplicated AI/experimental disclaimer from ApifyClient's class javadoc (README-only now); RequestQueueClient.batchAddRequests no longer throws on a non-retryable 4xx, matching the reference client's never-throws contract (unprocessed requests reported via BatchAddResult instead); added offline mock-backend coverage for RunClient.metamorph/reboot and corrected the stale ActorRunIntegrationTest comment; added the remaining typed getters (ActorRun.usageTotalUsd/buildNumber/exitCode/isContainerServerReady/ gitBranchName/storageIds, Webhook.shouldInterpolateStrings/lastDispatch); extracted StartOptionsLike/CallOptionsLike + a RunStartSupport helper so ActorClient/TaskClient's start/call machinery stops duplicating ~185 lines; ApifyClientBuilder.timeout now rejects zero/negative durations; RequestQueueRequest.userData is defensively deep-copied; RequestQueueClient's cursor iterator now trims an overshot page to totalLimit; tightened spotbugs-exclude.xml to entries that actually reproduce a finding; plus the requested docs completions and CI permissions block. See notes.md "Implementer — loop 4" in apify-client-orchestration for the itemized list and test evidence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
github-actions
Bot
added
the
tested
…ocs contradictions, uniqueKey caveat Closes the loop-4 review's residual MAJORs and nits: - N1: RunStartSupport now takes Consumer<QueryParams>/plain values instead of the public CallOptionsLike/StartOptionsLike marker interfaces, so ActorClient/TaskClient can keep apply()/contentTypeOrDefault()/toStartOptions()/logStreamingEnabledValue()/logOptionsValue() package-private on ActorStartOptions/TaskStartOptions/ActorCallOptions/TaskCallOptions instead of leaking them (and the internal QueryParams type) into the options builders' public API. - N2: reconciled the examples.md contradiction across README.md/docs/README.md/docs/examples.md — all three now agree that the fragments are fragments and the runnable programs live under src/test/java/com/apify/client/examples/. - N3: added the missing WebhookLastDispatch import to docs/README.md's package table. - N4: added an offline mock-backend test for ApifyClient.setStatusMessage's ACTOR_RUN_ID-unset IllegalStateException path (previously zero coverage); documented why the ACTOR_RUN_ID-set path isn't offline- or live-testable, mirroring the metamorph/reboot/charge convention. - N5: added a javadoc caveat on batchAddRequests/RequestQueueRequest.getUniqueKey() about uniqueKey-based retry reconciliation falsely reporting a succeeded request as unprocessed. - Nits: isShouldInterpolateStrings() -> getShouldInterpolateStrings() (field name unchanged, so no wire-format effect); named the unused RequestQueueClient catch binding `ignored`; fixed the "one raw-JSON field" framing in docs/README.md; clarified the request-queue getOrCreate overload gap in docs/storages.md; documented SetStatusMessageOptions's primitive boolean; trimmed boilerplate javadoc on trivial RequestQueueRequest setters; documented DefaultHttpTransport(Duration) and evened out import disclosure in README fragments; strengthened the nested-webhook test comment to point at the real multi-item iteration coverage. mvn spotless:check/compile/spotbugs:check/test all green: 183/183 tests passing (unit + live integration + ExamplesTest + DocSnippetsTest), 0 failures, 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
…setter trim Addresses the two concrete findings from this loop's coding-style review: - The near-identical "package-private on purpose" paragraph was copy-pasted across ActorStartOptions/ActorCallOptions/TaskStartOptions/TaskCallOptions's class javadoc. Condensed each to a one-line pointer at RunStartSupport, whose own javadoc (now with @param docs on call/callWithLogStreaming) carries the full rationale once. - RequestQueueRequest's setter-javadoc trim was inconsistent: setId/setUrl/setMethod/setPayload had their boilerplate one-liner removed, but setHandledAt/setRetryCount/setLoadedUrl (equally trivial restatements of their getters) did not. Trimmed those three too for consistency; setUserData/ setHeaders/setNoRetry/setUniqueKey/setErrorMessages are untouched (still the mandated keep-as-informative set). mvn spotless:check/compile/spotbugs:check/test all green: 183/183 tests passing, 0 failures, 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
…doc gaps Addresses the second independent /review-client pass's residual findings on top of f705ae6: - P1 (CI, merge-blocking): ActorRunIntegrationTest.streamedLogRedirection failed in CI on f705ae6 (the same live race as loop 1/2 — a live-tail stream opened before the run finishes can reach EOF a hair before the final log bytes are flushed to that connection). Fix: after the bounded catch-up window, if the run produced a log but the first stream still came up empty, retry once with a brand-new StreamedLog opened strictly after the run is already finished — a GET against a static, fully-persisted log rather than a live tail, so it cannot race the run's own writer. Verified green 3x locally against the live API. - P2: fixed a factually wrong RunStartSupport javadoc claim that ActorStartOptions/TaskStartOptions/ ActorCallOptions/TaskCallOptions "stay package-private" — those classes are public (they must be, as public method parameter/return types); only 5 of their accessor methods are package-private. - P5: AbstractCollectionClient.list(options) NPE'd on null options while iterate(options) tolerated it — now consistent. BatchAddResult.getProcessedRequests()/getUnprocessedRequests()/merge() now null-coalesce, matching PaginationList's existing pattern, so an explicit "processedRequests": null / "unprocessedRequests": null API response can no longer NPE. - P7 (docs): documented that ValidateInputOptions lives in com.apify.client.task despite configuring an ActorClient method; added java.time.Instant to the standard-import list; noted that setStatusMessage/UserClient limits methods/ApifyClientBuilder setters throw plain JDK exceptions outside the ApifyClientException hierarchy; clarified waitForFinish/call's behavior when the wait budget expires before the run reaches a terminal state. mvn spotless:check/compile/spotbugs:check/test all green: 183/183 tests passing, 0 failures, 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
…d-Actor race, DTO null-guards, RunClient.metamorph validation, package move, docs Addresses every item from the loop-6 /review-client verdict: RequestQueueClient.batchAddChunkWithRetries now catches ApifyClientException (not just ApifyApiException) so transport failures also stay within the never-throws contract; ActorRunIntegrationTest#lastRunAccess no longer asserts run identity against the shared apify/hello-world Actor (only presence/status/origin); added last-run- and run-scoped nested storage GET coverage; null-coalesced every flagged list-returning DTO getter; moved ValidateInputOptions to com.apify.client.actor next to its only consumer; RunClient.metamorph validates targetActorId; various docs completeness, comment, and DRY nits. See notes.md "Implementer — loop 7" in apify-client-orchestration for the itemized list and test evidence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
- RequestQueueClient: null-coalesce BatchDeleteResult's list getters and RequestQueueRequest.getErrorMessages() to avoid NPEs on an explicit JSON null, mirroring BatchAddResult. - RunClient.metamorph: normalize targetActorId via ResourceContext.toSafeId before sending (matches the reference client's _toSafeId); update the offline mock test's expected query param. - RunClient.metamorph/resurrect and ActorClient.build: throw a clear IllegalArgumentException on a null required options argument instead of an NPE deep inside the method. - RunClient's default log-redirection prefix now uses the resolved run's real id instead of the field id (which is the literal "last" for a runs/last accessor). - Box ActorRunOptions.timeoutSecs/memoryMbytes/diskMbytes (Long) so an unset value is distinguishable from 0, matching the boxed sibling fields.
Flesh out response DTOs to match the JS reference's typed interfaces, resolving the recurring thin-DTO review category: - WebhookDispatch: typed status, eventType, userId, createdAt, calls (WebhookDispatchCall), webhook (WebhookDispatchWebhookInfo), eventData (WebhookDispatchEventData). - Actor: wire in getActorStandby() (ActorStandby, now also carrying the Actor-only isEnabled field), plus stats (ActorStats), versions, pricingInfos, defaultRunOptions (ActorDefaultRunOptions), taggedBuilds, actorPermissionLevel, categories, isDeprecated, deploymentKey, seoTitle/seoDescription. - Build: userId, meta (BuildMeta), stats (BuildStats), options (BuildOptions), usage/usageUsd/usageTotalUsd (BuildUsage). - User: profile (UserProfile), email, proxy (UserProxy), plan (UserPlan), effectivePlatformFeatures, createdAt, isPaying. - ActorStoreListItem: description, stats, currentPricingInfo (PricingInfo), pictureUrl, userPictureUrl, url, readmeSummary. Task/ActorRun were already complete from an earlier loop; verified against the JS reference and left unchanged. Every new field name/type was checked against apify-client-js's resource_clients interfaces before modelling. getExtra() is kept for forward-compat on every touched model. Docs field lists updated to match (actors.md, builds.md, misc.md, webhooks.md, examples.md); the GetAccount example and its doc snippet now read the typed getEmail() instead of reaching into getExtra(). spotbugs-exclude.xml: added the new pure-DTO classes to the existing UWF_UNWRITTEN_FIELD allow-list (same false-positive rationale as the pre-existing entries); rewrote the "historical note" to state only the current timeless rationale, dropping the loop-numbered narrative.
Store/WebhookDispatch collection clients now extend AbstractCollectionClient (StoreListOptions implements ListOptionsLike); duplicated deadline-poll loops in integration tests routed through IntegrationBase.pollUntil; stream catch-up and retry-bound literals extracted to named constants. Offline gates green (spotless:check, compile, test-compile, spotbugs:check); live integration suite runs in CI. Loop-8 remainder (Q5 comments, Q6 imports/records, Q7 CRUD-flow list steps, Q8 docs) still pending.
…s — Q5/Q6/Q7/Q8 Trims the spotbugs-exclude.xml/StreamedLog/PaginationList review-nit comments; imports java.util.Map/Locale instead of inline FQNs; converts ApiResponse and HttpClientCore.Compressed to records (matching QueryParams.Pair), updating every field-access site and adding the resulting EI_EXPOSE exclusion; adds a top-level list() assertion to the Actor/Dataset/RequestQueue/KeyValueStore/ Webhook/Task CRUD-flow integration tests; documents RequestQueueRequest's getters, adds argument types to Dataset*/SetRecord/ValidateInput option field lists, and adds an ApifyClientBuilder + HttpTransport contract reference to the README. mvn spotless:check/compile/spotbugs:check/test (unit + live integration + ExamplesTest + DocSnippetsTest) all green: 187/187 passing, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuTYpvY6WkVvmsXzupzEk9
…-parity package-table entries, HttpTransport JDK-type disclosure, typed field lists, builder validation-timing consistency Addresses R1-R5 from the loop-8 review: drops a process-narrative clause from CHANGELOG, adds the loop-8 DTO-parity nested types to docs/README.md's package table, discloses HttpRequest/HttpResponse<T> as java.net.http types in the HttpTransport contract, adds Java types to RunResurrectOptions/ TaskOptions/ActorStartOptions field lists plus a publicBaseUrl(String) explainer and ActorStandby package notes, and reconciles the ApifyClientBuilder validation-timing wording (call-time, not build-time) between CHANGELOG, README and the PR description.
ActorRunStats/ActorRunMeta/BuildStats gained the missing typed getters (migrationCount/rebootCount, scheduleId/scheduledAt, imageSizeBytes) and, along with every other nested value DTO in the client, now extend the shared ApifyResource base so any unmodelled field the API returns stays reachable via getExtra() instead of being silently discarded.
The builder-settings sentence still referenced "custom HTTP backend" from before the HttpBackend->HttpTransport rename. Updated to "custom HTTP transport" to match the public API and the rest of the docs.
A 404 error body carrying `type` (e.g. record-not-found) but no `message` was previously dropped entirely because type/data extraction was gated on message being present, causing ResourceContext.isNotFound to miss it and get()/delete() to throw instead of mapping to Optional.empty()/no-op. Extract type, message, and data independently; keep the generic fallback message only when message itself is absent. Adds unit tests covering the type-without-message 404 case for get()/delete() and the same shape for a non-404 error.
…esence callWithActorCallOptionsStreamsLogByDefault could red-flake when a short hello-world run's log-streaming lifecycle closed before the background reader caught any bytes, even though the log was fully persisted server- side. Extracted the deterministic gating already used by streamedLogRedirection (loop 6) into shared IntegrationBase helpers and applied it to both that test and TaskIntegrationTest's equivalent taskCallStreamsLogByDefault, so none of the live streamed-log assertions can race a fast-finishing run.
…st not be rescued by a bypassing stream The TEST-1 review finding: assertStreamedLogNonEmptyIfProduced's rescue retry opens a separate, explicit StreamedLog on a mismatch, which is fine for streamedLogRedirection (its own subject) but would silently mask a genuinely broken call()-default log-streaming wiring for callWithActorCallOptionsStreamsLogByDefault and taskCallStreamsLogByDefault, since the rescue goes through a different code path than the one under test. Split off assertCallDefaultStreamedLogNonEmptyIfProduced, which keeps the bounded catch-up poll and the "skip if the run produced no log at all" gate but drops the rescue, and repointed both call()-default tests to it.
mixalturek
reviewed
Jul 23, 2026
mixalturek
left a comment
mixalturek
left a comment
There was a problem hiding this comment.
7k added and 2k removed lines are very suspicious, why so huge increase? I'm sorry, review of so many changes is not doable, I gave it only half an hour.
Comment on lines
-3
to
-6
| > **Official, but experimental — AI-generated and AI-maintained.** This is an official Apify client, | ||
| > but it is experimental: it is generated and maintained by AI. Review the code before relying on it | ||
| > in production and report issues on the repository. | ||
|
|
There was a problem hiding this comment.
This info is important. The repeated parts should be deduplicated but not fully removed.
Collaborator
Author
There was a problem hiding this comment.
The agent did this because of the requirement to have this warning just once and in the top-level README:
https://github.com/apify/apify-client-java/blob/claude/vibrant-edison-vq1jx6/README.md#apify-api-client-for-java
Comment on lines
+117
to
+133
| ## Setting the current run's status message | ||
|
|
||
| `client.setStatusMessage(String message, boolean isTerminal)` updates the status message of the | ||
| current Actor run (identified by the `ACTOR_RUN_ID` environment variable); it only works from inside | ||
| a run and throws `IllegalStateException` otherwise. Returns the updated `ActorRun`. | ||
| `client.setStatusMessage(String message, SetStatusMessageOptions)` (import from | ||
| `com.apify.client.run`) updates the status message of the current Actor run (identified by the | ||
| `ACTOR_RUN_ID` environment variable); it only works from inside a run and throws | ||
| `IllegalStateException` otherwise. `SetStatusMessageOptions.isStatusMessageTerminal(boolean)` marks | ||
| the message as final so it won't be overwritten. Returns the updated `ActorRun`. | ||
|
|
||
| Note: `isStatusMessageTerminal(boolean)` takes a primitive `boolean`, not a boxed `Boolean` — an | ||
| intentional exception to the "optional fields are boxed so they can stay unset" convention used | ||
| elsewhere in this client's options classes, since this field always has a concrete on/off meaning | ||
| with no useful "unset" state (the field is simply omitted from the request body when the setter is | ||
| never called). | ||
|
|
||
| ```java | ||
| client.setStatusMessage("half way there", new SetStatusMessageOptions().isStatusMessageTerminal(false)); | ||
| ``` |
There was a problem hiding this comment.
That method doesn't make sense and should be removed. It shouldn't have several long paragraphs in the README.
Collaborator
Author
There was a problem hiding this comment.
The function is very strange, but the agent can't help it. It is mandated by requirements to implement all public convenience functions that the JS reference implementation has (this will also copy the weird ones...):
https://docs.apify.com/api/client/js/reference/class/ApifyClient#setStatusMessage
So the correct flow would be to deprecate and later remove it in the reference JS client and let it propagate this way to generated clients.
But I agree that it should have at least a minimal footprint in the docs.
Collaborator
Author
There was a problem hiding this comment.
Agreed on all points. The method stays (mandated parity with the reference JS client), and the clean removal path is deprecating it upstream in the reference client and letting that propagate to the generated clients.
Trimmed its docs footprint to a minimal mention in c46278b: the "Setting the current run's status message" section is now two sentences plus a single example, and the paragraph rationalizing the primitive-boolean choice is gone.
Generated by Claude Code
Address PR review comment: reduce the dedicated section to a terse what-it-does description and drop the primitive-vs-boxed rationale paragraph, since the method is deliberately kept low-profile in docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Syncs the Java client to Apify OpenAPI spec
v2-2026-07-20T094852Zand releases0.4.0, applying the structural changes from the expert review in #8. This is a breaking release (explicitly authorized).Structure & public API
com.apify.clientpackage into resource-scoped sub-packages (.actor,.build,.run,.dataset,.keyvalue,.requestqueue,.task,.schedule,.webhook,.user,.store,.log,.http). Amodule-info.javaexports only these and keeps implementation plumbing in a non-exportedcom.apify.client.internalpackage.HttpBackend→HttpTransport,DefaultHttpBackend→DefaultHttpTransport,sendStreaming→sendStreamingResponse.ApifyClientExceptionbase, withApifyApiExceptionand a transport-agnosticApifyTransportException/HttpTimeoutException.ApiPaths; makesResourceContextand the storage clients immutable.ValidateInputOptionslives incom.apify.client.actor, alongside its only consumer (ActorClient.validateInput).Behavior & correctness
RequestQueueClient.batchAddRequestschunks by both request count and cumulative payload byte size (so a batch of large requests can't 413), and never throws on a hard 4xx or a transport-level failure (timeout, connection error) — failed requests are returned viaBatchAddResult.getUnprocessedRequests(), matching the reference client's contract.JsonNode/getExtra()access across the request-queue lock operations and theSchedule/Webhook/Task/ActorRunmodels (and, in later revisions,Actor/Build/User/ActorStoreListItem); every list-returning DTO getter tolerates an explicit JSONnullwithout throwing.call(..., ActorCallOptions / TaskCallOptions, ...)overloads that stream the run's log during the wait (matching the reference client); the plaincall(..., *StartOptions, ...)overload is unchanged.RunClient.metamorphvalidates and normalizestargetActorId;ApifyClientBuilder's setters validate their inputs at call time (e.g. reject a zero/negative timeout immediately, not deferred tobuild()); brotli compression is opt-in; logging goes through SLF4J.Docs & tests
apify/hello-worldstore Actor.mvn test(unit + live integration +ExamplesTest+DocSnippetsTest) passing.Review points intentionally not applied
ApifyClient.setStatusMessage(...)is kept — the reference JS client has it, so removing it would break the required convenience-method parity.ApifyApiExceptionstays aRuntimeException-rooted type (parity with the JS error model, which is a plain unchecked error).Test plan
mvn spotless:checkmvn compilemvn spotbugs:checkmvn test(offline unit + live integration +ExamplesTest+DocSnippetsTest)