compute: field validation errors are stripped of their detail

[bjohnso5]

Client

Compute (but likely others, as well, due to code generation and sharing)

Dependencies

go.mod

module modname

go 1.23.2

require (
    ...
    cloud.google.com/go/compute v1.31.1
    github.com/googleapis/gax-go/v2 v2.14.1
    google.golang.org/api v0.215.0
	google.golang.org/grpc v1.69.2
	google.golang.org/protobuf v1.36.2
    ...
)

Expected behavior

API field validation details are preserved in Bad Request scenarios.

Actual behavior

With the recent update to the compute/apiv1/helpers.go (https://github.com/googleapis/google-cloud-go/blob/d448fbb8ff228b1081ebbc98ee13e2a49e2b373c/compute/apiv1/helpers.go), 400 Bad Request errors are being stripped of their details due to what I'm fairly certain is a bug trying to double-read the response body.

The response body is read here, and then again within the googleapi.CheckResponse call.

This returns error strings like "googleapi: Error 400:", where previously the failure would contain the invalid field details (e.g. Invalid value for field 'resource.instanceProperties.machineType').

Additional context

I believe this issue is caused by this PR.

[bjohnso5]

I was able to successfully validate this as the root cause of the issue by adding a new implementation of a local copy of googleapi with a CheckResponseWithBody(res *http.Response, body []byte) that uses the previously read bytes and the previous error details are returned as expected.

[quartzmo]

@bjohnso5 Thank you for reporting this bug. If you can provide a simple repro script and a diff with your proposed fix, it will help us a lot. Don't create a PR in this repo, since this is generated code. The fix most likely needs to be applied to https://github.com/googleapis/gapic-generator-go. Working in that project is a bit involved, so I can make the change there if you provide the groundwork in this issue. Thanks!

[bjohnso5]

Based on that repo, it looks like https://github.com/googleapis/gapic-generator-go/blob/main/internal/gengapic/gengapic.go#L279 this read is the ultimate culprit. I'm not certain whether having a CheckResponseWithBody(res, body) option that the executeHTTPRequestWithResponse can invoke, or attempting to re-order the calls so that the body read does not occur twice is the right approach.

I'm definitely no expert in the generation, nor the overall impact of the decision one way or another. I hope this points you in the right direction, though!

[bjohnso5]

I'm happy to provide PRs to both repositories if you agree with the approach, or you can run with it if you have a different opinion on the appropriate resolution.

[bjohnso5]

@quartzmo I've opened a pair of PRs (linked here). Please let me know if anything else is required.

Cheers!

[quartzmo]

@bjohnso5 We certainly welcome contributions and it seems that you have a good solution. I'm happy to help you get these PRs merged. The first PR makes sense to me except for something unusual in the existing code which I have asked @codyoss to look at. Cody should also approve both since he is the author of the logic you're modifying. Thanks for your help!