Manual review, overall issues

[Pijukatel]

By @mixalturek:

Review of the AI generated code

README.md

• Quick start
  • Close to SLOP, I can't imagine anyone doing this in practice.
  • The snippet with main() is ok, but I would remove the rest.
• Replaceable HTTP transport
  • HttpBackend -> HttpClient :-D
• Fetching single resources
  • Prefer FP
  • if (maybeActor.isPresent()) ... -> maybeActor.ifPresent(it -> System.out.println(it.getTitle()))
• Versioning
  • the Apify OpenAPI specification version this client was verified against (v2-2026-07-13T092445Z). - What does it mean?
• I would expect an explicit info that the client is synchronous and thread safe.
  • Update: The code is not fully thread safe. Either remove that info or fix the code.
  • DatasetClient.withPublicBase() mutates itself, probably only at a build time.

pom.xml

• <jackson.version>2.17.2</jackson.version> - v2 is still widely spread, but it got replaced by v3 and started dying.
• Platform specific brotli dependencies are very strange here. No library should take such linux, windows, 2 osx ones variants for no reason. There should be only this one with <optional>true</optional> and opt-in.

<dependency>
    <groupId>com.aayushatharva.brotli4j</groupId>
    <artifactId>brotli4j</artifactId>
    <version>${brotli4j.version}</version>
    <optional>true</optional>
</dependency>

• I would expect slf4j dependency for logging. Is there a logging in the client library?

com.apify.client

• All classes are in single package com.apify.client, but there is an obvious split based on the names of the classes:
  • com.apify.client.http
  • com.apify.client.actor
  • com.apify.client.build
  • com.apify.client.dataset
  • com.apify.client.keyvalue
  • com.apify.client.requestqueue
  • ...
• This would be unfortunately a breaking change.

ApifyClient.java

• Duplicated info in javadoc.

 * <p><b>Official, but experimental — AI-generated and AI-maintained.</b> This is an official Apify
 * client, but it is experimental: it is generated and maintained by AI.

• getUserAgent() and getApiBaseUrl() are public, why? Make them private or remove?
• Consider rename actors() -> actorsCollection(), the same for builds(), runs() etc. The 'S' at the end of the name is too small difference and the returned classes use Collection suffix. No strong preference, up to you :-)
• I would expect the API resource paths to be always defined directly in the dedicated clients instead of the constructor calls in the ApifyClient. There is also no consistency, e.g. DatasetCollectionClient defines it internally but DatasetClient client takes "datasets" from the outside. There is also named constant ME_USER_PLACEHOLDER. The BE endpoints could be also listed at a single place in a "constants" class (better).

  /** A client for the dataset collection (list & get-or-create datasets). */
  public DatasetCollectionClient datasets() {
    return new DatasetCollectionClient(http, baseUrl);
  }

  /** A client for a specific dataset, addressed by ID or name. */
  public DatasetClient dataset(String id) {
    return new DatasetClient(http, baseUrl, "datasets", id).withPublicBase(publicBaseUrl);
  }

• The code uses builder pattern at various places, but not consistently. KeyValueStoreClient is similar to builder, but it mutates its internal state and returns itself. It isn't named Builder and doesn't have build() method. DatasetClient as well.

  public KeyValueStoreClient keyValueStore(String id) {
    return new KeyValueStoreClient(http, baseUrl, "key-value-stores", id)
        .withPublicBase(publicBaseUrl);
  }

• log(String buildOrRunId) - I would expect this one to be defined in BuildClient or RunClient.
• The same for run(String id) and build(String id), wouldn't it be better to define them in ActorClient?
• setStatusMessage() looks like SLOP. Taking method argument from env using System.getenv("ACTOR_RUN_ID") is strange.

ApifyClientBuilder

• /** Overrides the base URL of the API. The {@code /v2} suffix is appended automatically. */ - An URL normalization could be also in the code instead of the hardcoded String apiBase = trimTrailingSlash(baseUrl) + "/v2";. This would be about a few lines and simpler for everyone.
• Missing validations, I would expect at least basic non-null & non-blank strings, negative integers, etc. Pointing to config mistakes using IllegalArgumentException instead of failures much later would be nice from a library. This could be done also slightly later in build() or in ApifyClient constructor.
• /** Sets the overall per-request timeout (default 360s). */ - default 6 minutes timeout?! Is it due to downloads and uploads of large files? Please consider to split it to multiple dedicated ones with reasonable values.
• trimTrailingSlash() - Strange implementation, a sane agent would use String.endsWith().

HttpBackend

• It's great that this is an interface and that a concrete implementation is injectable (apache sync or async, webclient, okhttp, ...)
• Naming: HttpBackend for a client is strange :-D
• Naming: sendStreaming() does not stream request, it's response. It could be confusing, sendAndStreamResponse() or stream in both directions?

DefaultHttpBackend

• CONNECT_TIMEOUT is hardcoded instead of a config next to the other ones. 30 s is also extremely high, I would expect typically milliseconds and anything above a second too high.

HttpClientCore

• detectBrotli(), it's always available thanks to dependencies in pom.xml, this code has no effect.
• What is purpose of package private userAgent(), backend(), requestTimeoutSeconds(), baseRequestTimeout() that expose the internal details. The class itself is package private, so it can freely define its public interface (call()?).
• Define which methods are public (interface) and which ones private (internal details), package private ones are typically rare in general.
• So many call*() methods are suspicious. Are they needed in practice or is it a SLOP? Why not only with HttpRequest the param?
• The timeouts, retries and sleep in callWithHeaders() can steal a thread for long minutes or maybe even hours (with the default timeout 6 minutes, 500 ms delay and 8 retries, exponential backoff in the code). Using this library blindly - which we want to simplify the access to Apify API - would be very dangerous for stability of any BE service at a situation when the things go wrong. If we want this behavior, let's change the synchronous call model to async non-blocking that would much better match the requirements. If we decide for change, prefer project reactor to the low level CompletableFuture.
• isTimeout(TransportException e) and instanceof java.net.http.HttpTimeoutException expects DefaultHttpBackend, but there could be any HTTP client under the hood. HttpBackend interface specifies only IOException.
• buildApiError() - why so low-level and ugly work with JsonNode? Just map the response to a typed object using Jackson.
• extractPath() - build URI from the string to parse it instead of string operations.

ApifyApiException

• It should inherit IOException instead of RuntimeException, it is clearly an IO exception.
• Alternatively UncheckedIOException if you want it unchecked, but this would be uncommon.

TransportException

• It inherits RuntimeException instead of ApifyApiException.
• Why is it thrown also when gzip/brotli compression fails? There is no transport.