google-cloud-rust icon indicating copy to clipboard operation
google-cloud-rust copied to clipboard

Support multiple additional bindings for generated clients

Open coryan opened this issue 7 months ago • 2 comments

Currently we support only one binding, which limits what some clients can do.

coryan avatar May 29 '25 18:05 coryan

Checkpointing progress... I think I am finally over the hump with this feature.

https://github.com/dbolduc/google-cloud-rust/commits/additional-bindings-2-dev

This branch implements additional bindings / path validation for some of the services in our integration tests, e.g. secretmanager.

The generated code, and the golang to get there needs a lot of cleaning up. (transport.rs is 5x as long). Here is the generated code:

https://github.com/dbolduc/google-cloud-rust/blob/2f2420df38f2a9fa08d8032ed9941ffc65f38c1d/src/generated/cloud/secretmanager/v1/src/transport.rs#L481-L607

I am able to run the integration tests, although now this test fails (because the binding is invalid. Yay! But also, boo, now I have to update the test):

https://github.com/googleapis/google-cloud-rust/blob/aa1a7abf72d3a8a6ea6b414a2fd9093958c404ec/src/integration-tests/tests/idempotency.rs#L80

Time to start cleaning up and breaking this down into PRs....

dbolduc avatar Jun 17 '25 01:06 dbolduc

Another day of progress. I factored out the errors and supported non-string fields. The generated code looks like:

https://github.com/dbolduc/google-cloud-rust/blob/a4c36bc7358a3cbe4aad472f2b9cfd7a3ff31973/src/generated/cloud/secretmanager/v1/src/transport.rs#L456-L544

I can run the integration tests (including gapic-showcase) for real this time.

Some weirdness - additional bindings for mixins will be non-trivial. The LRO methods aggregate all of the additional bindings across all of the services. There are ~90 of these. Not super helpful. Hm.

dbolduc avatar Jun 18 '25 01:06 dbolduc

additional bindings for mixins will be non-trivial. The LRO methods aggregate all of the additional bindings across all of the services.

I was wrong.

There are ~90 of these

That is just aiplatform. Gross.

https://github.com/googleapis/googleapis/blob/8ee3aad1fd2f2376373a030cafc61b3e9788e48a/google/cloud/aiplatform/v1/aiplatform_v1.yaml#L253-L340

dbolduc avatar Jun 18 '25 22:06 dbolduc

Pushed another commit that simplified the path construction. No more local variables. They do not save us anything.

And another commit to support query parameters per binding. Note that "supporting query parameters" just means omitting fields that end up in the path. I have a lot of controversial thoughts here. I will write them up somewhere else...

https://github.com/dbolduc/google-cloud-rust/commits/additional-bindings-2-dev

dbolduc avatar Jun 19 '25 09:06 dbolduc

Here is the execution plan (each task is roughly a PR):

Gax changes

  • [x] Introduce BindingError et al. in gax
    • with fmt functions
  • [x] Introduce PathMismatchBuilder in gax-internal
  • [x] Add path_match() helper
    • (composable_matches() in my prototype)
  • [x] Test (once-generated) code for path matching?
    • (direct_format_path() in prototype)

Model changes

  • [x] PathSegment -> LegacyPathSegment
  • [x] refactor OpenAPI path parser to use httprule's parser
  • [x] define PathTemplate, PathVariable, PathSegment in api/model.go
    • Plus conversion function in parser.
  • [x] Add *api.PathTemplate field to api.PathBinding
    • we call conversion function in protobuf, OpenAPI parsers
  • [x] Refactor QueryParams() to accept a (*Method, *PathBinding) instead

Rust annotations

  • [x] Add bindingSubstitution func from variable segment
  • [x] Add Rust pathBindingAnnotation
    • Probably with the fieldPathRef business.
    • Maybe that gets its own PR.
    • Maybe I need to refactor.
  • [x] PathBinding gains codec, which we set in Rust to the pathBindingAnnotation

Generation

  • [x] Update transport template.
    • Only in gapic-showcase to start?
    • Add tests for:
      • first match wins
      • additional binding wins
      • has integers
      • error unset integer
      • error unset message
      • error empty string
      • error no matches
  • [x] Update tests that will fail due to binding errors
    • (SM invalid + aiplatform operation)
  • [x] Everyone gets the new transport

Documentation

  • [x] User guide section for parsing binding errors.
    • Basically, we need to explain what * in a template means. It is not obvious.

Long tail, out of scope for this issue

  • [x] update dart to use new path segments
  • [x] update golang to use new path segments
  • [x] clean up legacy code

dbolduc avatar Jun 20 '25 18:06 dbolduc