Skip to content

feat(gax): add ResumableUploadClient startUpload and HTTP/JSON implementation - #14086

Closed
whowes wants to merge 2 commits into
whowes/http-content-supportfrom
whowes/resumable-upload-start
Closed

feat(gax): add ResumableUploadClient startUpload and HTTP/JSON implementation#14086
whowes wants to merge 2 commits into
whowes/http-content-supportfrom
whowes/resumable-upload-start

Conversation

@whowes

@whowes whowes commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a low-level resumable upload client for the HTTP/JSON transport, including the HttpJsonResumableUploadClient implementation, supporting request/session models, and corresponding unit tests. The reviewer provided valuable feedback to improve the implementation, including handling case-insensitivity of HTTP headers to prevent duplicate entries, declaring listener fields as volatile to ensure thread safety during asynchronous callbacks, and optimizing path normalization in StartUploadRequest.Builder to avoid double-building the AutoValue object.

Comment on lines +117 to +120
Map<String, List<String>> extraHeaders = new HashMap<>(context.getExtraHeaders());
extraHeaders.putIfAbsent(UPLOAD_PROTOCOL_HEADER, Collections.singletonList("resumable"));
extraHeaders.putIfAbsent(UPLOAD_COMMAND_HEADER, Collections.singletonList("start"));
context = (HttpJsonCallContext) context.withExtraHeaders(extraHeaders);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

HTTP headers are case-insensitive, but HashMap is case-sensitive. If a user provides X-Goog-Upload-Protocol or X-Goog-Upload-Command in the ApiCallContext extra headers with a different casing (e.g., lowercase), putIfAbsent will not detect them and will add duplicate headers. We should perform a case-insensitive check before adding these default headers.

        Map<String, List<String>> extraHeaders = new HashMap<>(context.getExtraHeaders());
        boolean hasProtocol = false;
        boolean hasCommand = false;
        for (String key : extraHeaders.keySet()) {
          if (UPLOAD_PROTOCOL_HEADER.equalsIgnoreCase(key)) {
            hasProtocol = true;
          } else if (UPLOAD_COMMAND_HEADER.equalsIgnoreCase(key)) {
            hasCommand = true;
          }
        }
        if (!hasProtocol) {
          extraHeaders.put(UPLOAD_PROTOCOL_HEADER, Collections.singletonList("resumable"));
        }
        if (!hasCommand) {
          extraHeaders.put(UPLOAD_COMMAND_HEADER, Collections.singletonList("start"));
        }
        context = (HttpJsonCallContext) context.withExtraHeaders(extraHeaders);

Comment on lines +150 to +151
@Nullable private String uploadUrl;
private long chunkGranularity = 1L;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The StartUploadResponseListener callbacks (onHeaders and onClose) are executed asynchronously and potentially on different threads of the channel's executor. To guarantee proper memory visibility and thread safety across these callbacks, the uploadUrl and chunkGranularity fields should be declared volatile.

Suggested change
@Nullable private String uploadUrl;
private long chunkGranularity = 1L;
@Nullable private volatile String uploadUrl;
private volatile long chunkGranularity = 1L;

Comment on lines +83 to +100
@AutoValue.Builder
public abstract static class Builder {
public abstract Builder setPath(String path);

public abstract Builder setJsonPayload(@Nullable String jsonPayload);

public abstract Builder setQueryParams(Map<String, List<String>> queryParams);

abstract StartUploadRequest autoBuild();

public StartUploadRequest build() {
StartUploadRequest request = autoBuild();
if (request.getPath().startsWith("/")) {
return request.toBuilder().setPath(request.getPath().substring(1)).autoBuild();
}
return request;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation of build() creates a temporary StartUploadRequest instance, checks if the path starts with a slash, and if so, creates a new builder via toBuilder() to rebuild the request with the normalized path. This is highly inefficient and non-idiomatic for AutoValue. We can instead use a custom setter in the builder to normalize the path when it is set, avoiding any double-building overhead.

  @AutoValue.Builder
  public abstract static class Builder {
    abstract Builder autoSetPath(String path);

    public Builder setPath(String path) {
      if (path.startsWith("/")) {
        return autoSetPath(path.substring(1));
      }
      return autoSetPath(path);
    }

    public abstract Builder setJsonPayload(@Nullable String jsonPayload);

    public abstract Builder setQueryParams(Map<String, List<String>> queryParams);

    abstract StartUploadRequest autoBuild();

    public StartUploadRequest build() {
      return autoBuild();
    }
  }
References
  1. When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.

@whowes
whowes force-pushed the whowes/resumable-upload-start branch from 5dacb91 to 27993fe Compare August 17, 2026 21:34
@whowes whowes closed this Aug 17, 2026
@whowes whowes reopened this Aug 17, 2026
@whowes whowes closed this Aug 17, 2026
@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant