Skip to content

feat(Gax): Resumable Uploads - #9456

Open
bshaffer wants to merge 19 commits into
mainfrom
scotty
Open

feat(Gax): Resumable Uploads#9456
bshaffer wants to merge 19 commits into
mainfrom
scotty

Conversation

@bshaffer

@bshaffer bshaffer commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@bshaffer
bshaffer marked this pull request as ready for review August 7, 2026 15:33
@bshaffer
bshaffer requested a review from a team as a code owner August 7, 2026 15:33
Comment thread Gax/src/ResumableUpload/ResumableUploadClient.php
Comment thread Gax/src/ResumableUpload/ResumableUploadClient.php

$headers = ['Content-Type' => 'application/x-protobuf'] + $headers;
$headers += ['x-goog-api-client' => []];
$headers['x-goog-api-client'][] = 'grpc-web';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There seems to be a bug here:

We are using:

$headers += ['x-goog-api-client'] => []];

That is great as we are trying to not overwrite something that already exists, so far so good. But if we check for example in this test we are passing the x-goog-api-client as a string:

 $client = new ResumableUploadClient(
     $this->createStubTransport($requestBuilder->reveal(), $httpHandler),
     $this->prophesize(CredentialsWrapper::class)->reveal(),
     headers: ['x-goog-api-client' => 'test-agent/1.0']
);

So the $headers += ['x-goog-api-client'] => []]; line tries to assign an array but it does not overwrites it. But the header is still a string, and the next line:

$headers['x-goog-api-client'][] = 'grpc-web';

We try to add grpc-web to an array, but it is a string, turning into an invalid string operatior:

`some_string`[] = 'grpc-web';

@bshaffer bshaffer Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch... and also, the HttpUnaryTransporTrait isn't grpc-web, nor should it have a content type of application/x-protobuf... That should be just in GrpcFallbackTransport, which doesn't need to support ResumableUploads anyway.

I think we should just remove the ResumableUploadTransportInterface from GrpcFallbackTransport, and move these methods to RestTransport. That will simplify things. There is no need to support ResumableUploads on GrpcFallbackTransport at this time (I did it mostly just for fun and to see what the design would look like, but it's over-complicated for the initial release)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok I've moved these methods to RestTransport, revert the adding the interface to GrpcFallbackTransport and reverted all changes to HttpUnaryTransportTrait, and added tests in RestTransport specifically for the new methods.

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.

2 participants