Handle google.api.HttpBody?

[broady]

Looks like this was handled specifically for ML engine in #241 (CL 34871) but maybe not for the general case.

For example, healthcare.Projects.Locations.Datasets.FhirStores.Fhir.CreateResource, which accepts raw JSON.

We currently re-encode the JSON and force a bunch of stuff on the user.

Do we know of any API where HttpBody in a request actually works? If not, I propose we apply the ML engine patch to any outgoing request using the HttpBody type.

/cc @jadekler @noahdietz @jba

[jeanbza]

Augmenting this post-conversation for posterity:

Do we know of any API where HttpBody in a request actually works?

ML, per https://code-review.googlesource.com/c/34871.

---

My understanding of this FR

Given a discovery doc like the following: https://ml.googleapis.com/$discovery/rest?version=v1, we should look for things like,

          "response": {
            "$ref": "GoogleApi__HttpBody"
          },

And when we see GoogleApi__HttpBody we should skip the JSON decoding step. We did this in cl34871 but the signal is the specific method name - this request asks us to generalize.

[broady]

Sorry, yes, I meant other than the ML APIs.

Yeah, I think we need to handle HttpBody for return values, too.

CL 34871 is missing a couple things. It ignores ContentType, which are important for the healthcare API, at least - maybe others.

It also treats Body's string type as an alias to []byte, which is problematic because of #346, creating an inconsistency with how it's handled in other parts of the surface.

Two options (there's probably more):

• apply CL 34871's strategy to all HttpBody, add ContentType. For responses, unpack the http.Response into the HttpBody struct.
• think about whether we want to expose more things on *http.Request, and forego generating the HttpBody struct entirely. And handle HttpBody responses in a similar way.

[broady]

Looks like only healthcare and ml use HttpBody.

For some reason, ML ends up with a type name of GoogleApi__HttpBody, and healthcare as simply HttpBody.

ML API has HttpBody as a field in GoogleCloudMlV1__PredictRequest

Healthcare API has HttpBody as a function parameter in CreateResource

See internal bug 38043930 for more detail on discovery generation problems with HttpBody.

[broady]

The following is maybe only for healthcare, since it has so many raw HTTP operations...

I'm leaning towards adding a top-level helper to Service:

func (s *Service) DoHTTP(method, path string, body io.Reader) (*http.Response, error)

This would essentially pass through to the underlying http.Client, also prefixing Service.BasePath to the provided path.

Then, for any method that asks for (or returns) an HttpBody, document that people should use DoHTTP instead.

Other names to bikeshed:
Do, DoRaw
Get/Post/Patch/Delete (like http.Client)

[tbpg]

Should we change the API (make it easier to use than work around it)?

I agree this would be OK to do for just Healthcare - not a widespread issue.

I'm good with any of the names. I like "HTTP" because it connects with HttpBody. I like "raw" because it implies it's not nicely typed.

[lukesneeringer]

Should we change the API (make it easier to use than work around it)?

Healthcare needs to return google.api.HttpBody. The basic reason is that the API conforms to a very specific external standard in the medical industry. (I do not think what it returns is even JSON; I think it is XML conforming to a specific schema.)

[broady]

Figured out a more elegant way to handle this.

Please review over on CL 39310.

Code looks like this to use:

call := fhirService.CreateResource(parent, resourceType, bytes.NewReader(jsonPayload))
call.Header().Set("Content-Type", "application/fhir+json;charset=utf-8")
resp, err := call.Do()
if err != nil {
	return fmt.Errorf("CreateResource: %v", err)
}
defer resp.Body.Close()

This is quite a bit better than requiring users construct their own *http.Request, because they get type safety on the correct method and URL path parameters via the discovery service.

This means you only get access to change the body and any headers. We can add more stuff later if we need to.

[codyoss]

@broady It looks like this could be closed, or is there still more work to do here?

[codyoss]

This looks to be solved for the APIs that we cared about. As there has not been much traction on this bug over the three years I think it is good enough to close this for now. We can re-evaluate individual cases should the need arise.