Fix empty GET request codec translation

[ls-aron-kyle]

Requiring request prep for all requests except for body: "*"

[CLAassistant]

All committers have signed the CLA.

[emcfarlane]

Thanks for looking into this! Would you be able to create a testcase in the file vanguard_restxrpc_test.go. You'll have to update the proto files under internal/proto/vanguard to cover the case. As this was an oversight in the testing, and isn't currently covered. To keep with the theme maybe in library.proto:

  // Lists shelves.
  rpc ListShelves(ListShelvesRequest) returns (ListShelvesResponse) {
    // List method maps to HTTP GET.
    option (google.api.http) = {
      get: "/v1/shelves"
    };
  }

And the testcase something like:

	}, {
		name: "ListShelves-GetNilRequest",
		input: input{
			method: http.MethodGet,
			path:   "/v1/shelves",
			body:   nil,
		},
		stream: testStream{
			method: testv1connect.LibraryServiceListShelvesProcedure,
			msgs: []testMsg{
				{in: &testMsgIn{
					msg: &testv1.ListShelvesRequest{},
				}},
				{out: &testMsgOut{
					msg: &testv1.ListShelvesResponse{},
				}},
			},
		},
		output: output{
			code: http.StatusOK,
			body: &testv1.ListShelvesResponse{},
		},

[emcfarlane]

This is checking for an unknown ContentLength as -1 is valid for a non empty body where the content-length can't be deduced up front.

[ls-aron-kyle]

I added some unit tests and took another stab at it using the restTarget values.

I'm open to other fixes.

[emcfarlane]

I believe the issue is here:

vanguard-go/transcoder.go

Line 2087 in 7aae240

case m.isRequest && op.clientReqNeedsPrep:

We need to not fallthrough to the default case as an empty body is not a valid JSON request. We must always use the prepareUnmarshalRequest for decoding the message if needed. We also must always emit a request message when we hit EOF here:

vanguard-go/transcoder.go

Lines 952 to 953 in 7aae240

	if !r.consumedFirst && errors.Is(err, io.EOF) && r.rw.op.clientReqNeedsPrep {
	r.msg.markReady()

I think for now the simplest fix is for any GET request to return true on requestNeedsPrep. So this would add a simple op.request.Method == http.MethodGET to the logic . The prepareUnmarshaledRequest is close to a no-op so shouldn't add much overhead. And almost all GET requests will require preparation of the request message.

[jhump]

I don't really understand this change.

For one, why does the request need preparation if the body is empty but all of those other conditions are false? That doesn't seem intuitive. The "request preparation" is to adjust the request -- based on other aspects of the request (like URI path, query string, etc) or to transform the request data when using google.api.HttpBody.

Also, the helper function looks wrong. If the ContentLength field of an *http.Request is less than zero, it means that the length of the body is not known -- i.e. you must actually read the body. This is common for HTTP/2 and HTTP 1.1 chunked requests.

[emcfarlane]

Looks good, thanks!