Skip to content

feat(generator): emit resumable upload settings and HttpJson upload stub - #14321

Merged
whowes merged 1 commit into
mainfrom
whowes/generator-resumable-upload-stub-composer
Sep 18, 2026
Merged

whowes merged 1 commit into
mainfrom
whowes/generator-resumable-upload-stub-composer

Conversation

@whowes

@whowes whowes commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Modified and introduces composers to emit an internal, dedicated REST stub and settings for resumable upload-powered services

@whowes
whowes added this pull request to stack #14327 September 9, 2026 06:35
gemini-code-assist[bot]

This comment was marked as outdated.

@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from f246196 to 1885bb8 Compare September 9, 2026 15:50
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 1885bb8 to 542f6e7 Compare September 9, 2026 20:16
@whowes
whowes removed this pull request from stack #14327 September 9, 2026 23:45
@whowes
whowes added this pull request to stack #14343 September 9, 2026 23:47
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 542f6e7 to 6f00489 Compare September 9, 2026 23:51
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 6f00489 to 296ed15 Compare September 10, 2026 00:19
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 296ed15 to bbc408e Compare September 10, 2026 01:15
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from bbc408e to c8c1e06 Compare September 10, 2026 06:05
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from c8c1e06 to c41b295 Compare September 11, 2026 17:24
@whowes
whowes removed this pull request from stack #14343 September 11, 2026 17:25
@whowes
whowes added this pull request to stack #14363 September 11, 2026 17:25
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from c41b295 to 63a3b57 Compare September 11, 2026 18:30
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 63a3b57 to f014476 Compare September 11, 2026 20:00
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from f014476 to 46d6fdf Compare September 11, 2026 21:14
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 46d6fdf to 2525926 Compare September 11, 2026 21:39
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 2525926 to 2f42f31 Compare September 11, 2026 22:15
@whowes
whowes changed the base branch from whowes/generator-rest-composer-helpers to main September 15, 2026 16:45
@whowes whowes changed the title feat(generator): add HttpJsonServiceResumableUploadStubClassComposer feat(generator): emit resumable upload settings and HttpJson upload stub Sep 15, 2026
@whowes
whowes added this pull request to stack #14390 September 15, 2026 16:48
*/
public static HttpJsonResumableUploadServiceResumableUploadStub createFrom(
ClientContext clientContext, ResumableUploadServiceStubSettings settings) {
String uploadEndpoint =

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.

Maybe create a new clientContext using ClientContext.create(settings)? So we don't have to manually re-create the clientContext from existing clientContext?

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.

After more investigation, I think ClientContext.create(settings) should work for httpjson stub without any issues. For grpc stub, we need to recreate the settings with a default internal header provider and httpjson channel, then ClientContext.create(settings) should work.

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.

Hmmm, I guess I'm not seeing how calling ClientContext.create(settings) inside the upload stub without referencing the parent context can avoid at least two main issues:

  • for both transports, in the top-level stub layer's create(ClientContext) factory overload, settings is just <Service>StubSettings.newBuilder().build(), so if we re-create from settings then everything caller-supplied on the ClientContext is dropped. We'd also be creating a separate thread pool and doing another ADC lookup on every client instance (even on REST stubs that already built an HTTP ClientContext- this I guess isn't functionally a problem, just suboptimal).
  • we can't swap the gRPC internal headers (x-goog-api-client: ... grpc/...) for HTTP headers via settings.toBuilder() because StubSettings.Builder#setInternalHeaderProvider is protected and in a different package (unless you think it's worth widening it for this, but the above issue still seems like a blocker alone)

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.

Took another look at this - I still don't see how we can fully rebuild from just settings without using some of the "parent" clientContext? In this PR as-is the generated code is pretty ugly but perhaps pushing that into the ClientContext itself makes sense - prototyped #14412 for illustration.

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.

ClientContext is not the source of truth and users are not supposed to modify it directly, it is derived from StubSettings which is considered the source of truth for all the customization. Also see the javadocs for ClientContext.

For thread pool and ADC, I do agree it is a slightly overhead, but it might be OK for simplicity. For the least performance overhead, we can use clientContext directly for httpjson and recreate clientContext from StubSettings for grpc.

StubSettings.Builder#setInternalHeaderProvider is indeed a problem. We could expose a new toHttpJsonBuilder() that copies everything but change the internal header provider to httpjson format.

@whowes whowes Sep 17, 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.

Since we're OK with the thread pool/ADC inefficiency this now is just creating fresh ClientContext. To make the internal headers work, there's now a generated setHttpJsonInternalHeaderProvider on the <Service>StubSettings.Builder that's only added for services that need resumable upload support.

* settings. This is protected so that it is easy to make a subclass, but otherwise, the static
* factory methods should be preferred.
*/
protected HttpJsonResumableUploadServiceResumableUploadStub(

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.

We have three constructor/factory methods that have the same arguments, consider consolidate them.

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.

Modified this to have one private constructor and one factory method (which has a conditional to adapt for gRPC v. HTTP/JSON).

@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch 3 times, most recently from 57daa1f to 954089e Compare September 16, 2026 00:45
RetrySettings settings = null;
settings = RetrySettings.newBuilder().setRpcTimeoutMultiplier(1.0).build();
definitions.put("no_retry_params", settings);
RETRY_PARAM_DEFINITIONS = definitions.build();

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.

The retry settings are not used currently, we need to add the initial unary call settings to ResumableUploadCallSettings. I'll work on it in a separate PR.

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.

SGTM, thanks!

@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 954089e to 5821e21 Compare September 16, 2026 05:46
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch 3 times, most recently from c7cffbf to 82fe151 Compare September 18, 2026 15:03
@whowes
whowes marked this pull request as ready for review September 18, 2026 15:07
@whowes
whowes requested review from a team as code owners September 18, 2026 15:07
@whowes
whowes requested a review from blakeli0 September 18, 2026 15:07
.build())
.setTransportChannelProvider(
InstantiatingHttpJsonChannelProvider.newBuilder()
.setEndpoint(settings.getEndpoint())

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.

Endpoint will be set in ClientContext, I don't think we need to set endpoint explicitly. For non-httpjson, I think we can use a default InstantiatingHttpJsonChannelProvider.

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.

Switched to this approach.

.setTransportToken(
GaxHttpJsonProperties.getHttpJsonTokenName(),
GaxHttpJsonProperties.getHttpJsonVersion())
.setQuotaProjectIdToken(settings.getQuotaProjectId())

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.

quotaProjectId is also handled in ClientContext, we don't have to set it here.

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.

Removed.

uploadMediaCallable;

public static HttpJsonResumableUploadServiceResumableUploadStub create(
ClientContext clientContext, ResumableUploadServiceStubSettings settings) throws IOException {

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.

Do we still need to pass clientContext in if we are always recreating it from settings? I think we can use info from settings to determine if the setting is for grpc or httpjson.

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.

I had intended that create would operate only on settings on the last round but clearly didn't get all the way there - apologies for the churn.

Now using only settings, withsettings.getTransportChannelProvider().getTransportName().equals(HttpJsonTransportChannel.getHttpJsonTransportName()) as the condition to determine whether we need to derive the ClientContext from gRPC.

Adds generator support for protocol-specific resumable upload options, independent of any transport stub wiring:

- Emit ResumableUploadCallSettings on the generated <Service>Settings and
  <Service>StubSettings, populated with global timeouts extracted from the
  service config while bypassing standard unary retries.
- Add HttpJsonServiceResumableUploadStubClassComposer, which generates the
  dedicated REST stub (HttpJson[Service]ResumableUploadStub) for services
  containing resumable upload RPC methods.
- Extract HttpJsonDescriptorComposer out of HttpJsonServiceStubClassComposer
  and add path prefix support. Pure refactor with no golden changes, reused by
  the upload stub composer above.

Supersedes #14318 and #14320, which are folded in here.
@whowes
whowes force-pushed the whowes/generator-resumable-upload-stub-composer branch from 82fe151 to 1dee45d Compare September 18, 2026 18:17
@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

settings
.toBuilder()
.setHttpJsonInternalHeaderProvider(
ApiClientHeaderProvider.newBuilder()

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.

Not in this PR: There might be a better way to do this without exposing a protected method. Or at least share the same generation code as the defaultHttpJsonHeaderProvider.

@whowes
whowes requested a review from blakeli0 September 18, 2026 20:00
@whowes
whowes merged commit c122474 into main Sep 18, 2026
306 of 310 checks passed
@whowes
whowes deleted the whowes/generator-resumable-upload-stub-composer branch September 18, 2026 22:20
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