feat(gax): implement uploadChunkCallable for resumable uploads

[whowes]

This handles sending binary chunks and finalize commands over HTTP/JSON, extracting offset and status from response headers/codes.

[whowes]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces support for transmitting individual chunks in a resumable upload session by adding ChunkUploadRequest and ChunkUploadResponse classes, implementing uploadChunkCallable() in HttpJsonResumableUploadClient, and adding corresponding unit tests. Feedback was provided to change the HTTP method in UPLOAD_CHUNK_DESCRIPTOR from POST to PUT to comply with the Google Scotty resumable upload protocol.

[sonarqubecloud]

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

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[whowes]

/gemini review

[blakeli0]

Can we use a Java native type like byte[]?

[whowes]

My main concern with the byte array is that it breaks the typical AutoValue immutability contract (unless using defensive copying which would be pretty expensive for multi-MB chunks). For Java native, ByteBuffer is the other option that comes to mind but it's stateful so not really suitable for use here.

Using ByteString seems to be within precedent when an immutable array/List of bytes is needed see e.g. ByteArray that uses it as backing storage even though the class itself isn't really protobuf-specific, wdyt?

[blakeli0]

My main concern with the byte array is that it breaks the typical AutoValue immutability contract

I think this is a valid concern but it is acceptable compare to other concerns. My two concerns are:

1. Unnecessary converting from InputStream to ByteString and then to byte[]. ResumableUploadCallable accepts an InputStream, so we would have to create a ByteString from it first in the upstream callable and get byte[] from it in low level callable.
2. Protobuf types such as ByteString are already exposed on the surface but we don't want to make it worse. Protobuf could still make breaking changes so using Java native types are preferred in general.

[whowes]

SGTM, switched to byte[]

[blakeli0]

I know we designed request.getUploadUrl to be a full url in previous PRs. But I'm leaning towards splitting the full url returned from server to endpoint and path to follow existing patterns now.

For example, split https://foo.com/upload?upload_id=xyz to https://foo.com as endpoint, upload as path, and upload_id=xyz as query parameters.
This is because we have some places like the getPathTemplate method below assuming the structure of the path and using a placeholder like PathTemplate.create("{+path}") could be misleading.

On the other hand, I know it might not be easy to split the full urls in a clean way either. Let me know what you think.

[whowes]

I don't know that we'd ever be able to use anything more specific than the {+path} placeholder here though since that may vary across the various services that use resumable uploads? e.g. we can't assume that we can parse out any upload ID (that limitation in particular is called out in one of our internal requirements docs.) I don't know that we even can assume that "upload" will be in the path.

In practice for the upload protocol code using the URL as an opaque string is fine since we just make requests directly to the URL without needing to make additions or subtractions to it (so there wouldn't really be much benefit to the protocol code specifically where this would be helpful), are there assumptions made elsewhere in GAX about the URL structure where this is problematic?

I'm not completely opposed to parsing here, just trying to understand the benefits v. complexity it would add (and probably preferring to defer that refactor to a future PR).

[blakeli0]

SGTM. Let's use PathTemplate.create("**") as a dummy placeholder for now.

[whowes]

Done, switched to "**" here and other relevant places.

[blakeli0]

Usually the creation of the callable is not done in the callable itself. For example, createOperationCallable in HttpJsonCallableFactory.
We probably need to do the same for the high level ResumableUploadCallable. For the low level callables, I think it is OK to do this for now in this PR, but we need to give it more thought later.

[whowes]

Acknowledged, I agree that ResumableUploadCallable creation definitely belongs there, and we can consider consolidating the lower-level Callable factory methods there too in the future.

[blakeli0]

My main concern with the byte array is that it breaks the typical AutoValue immutability contract

I think this is a valid concern but it is acceptable compare to other concerns. My two concerns are:

1. Unnecessary converting from InputStream to ByteString and then to byte[]. ResumableUploadCallable accepts an InputStream, so we would have to create a ByteString from it first in the upstream callable and get byte[] from it in low level callable.
2. Protobuf types such as ByteString are already exposed on the surface but we don't want to make it worse. Protobuf could still make breaking changes so using Java native types are preferred in general.

[sonarqubecloud]

Quality Gate passed for 'gapic-generator-java-root'

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

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

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

See analysis details on SonarQube Cloud