JSONCodec and nil request

[fridolin-koch]

Hello, first of all, cool project! I predict it will be very useful in the future :)

I was tinkering around a bit and noticed an issue with the REST translation. Consider the following gRPC service definition:

service Schedules {
  rpc ListSchedules(ListSchedulesRequest) returns (ListSchedulesResponse) {
    option (google.api.http) = {get: "/v1/schedules"};
  }
}

message ListSchedulesRequest {
  int32 page_size = 1 [(google.api.field_behavior) = OPTIONAL];
  string page_token = 2 [(google.api.field_behavior) = OPTIONAL];
  string filter = 3 [(google.api.field_behavior) = OPTIONAL];
  string order_by = 4 [(google.api.field_behavior) = OPTIONAL];
}

I should be able to issue a GET /v1/schedules request, without any parameters (i.e. empty request). Unfortunately this fails, since nil is then passed to protojson, which fails as it won't handle nil (IIRC it's an intentional design decision).

My suggestion to handle that would be to either treat nil as {} or to not call Unmarshal if the input is nil. Naively I'd change it here:

vanguard-go/codec.go

Line 212 in 7aae240

func (j JSONCodec) Unmarshal(bytes []byte, msg proto.Message) error {

func (j JSONCodec) Unmarshal(bytes []byte, msg proto.Message) error {
        if bytes == nil {
          return nil
          // or
          bytes = []byte("{}") // probably less efficient and unnecessary 
        }
	return j.UnmarshalOptions.Unmarshal(bytes, msg)
}

Not sure if this is the best place to change this.

Best Regards,
Frido

[jhump]

Hi, @fridolin-koch, thanks for the report!

I think the real issue is that the marshaler is even getting a nil request in the first place. This shouldn't ever happen. I'm guessing the lack of path parameters and lack of query parameters means the initialization of the request to a new, empty message is inadvertently being skipped.

[fridolin-koch]

I'm not sure, I looked at how this is handled in transcoder.go and it seems fine to me. If the request has no query params or body, there is no reason to to do any preprocessing ? As far as I understand the request gets passed (more or less) as is here

vanguard-go/transcoder.go

Line 590 in 7aae240

o.request.Body = &envelopingReader{rw: rw, r: o.request.Body}

to the underlying gRPC server, which then calls the configured decoder:

encoding.RegisterCodec(vanguardgrpc.NewCodec(&vanguard.JSONCodec{
	MarshalOptions:   protojson.MarshalOptions{EmitUnpopulated: true},
	UnmarshalOptions: protojson.UnmarshalOptions{DiscardUnknown: true},
}))```

So the error occurs here:

https://github.com/connectrpc/vanguard-go/blob/7aae240d504a5f2ce8aa6cf148232fcbf88eaf36/vanguardgrpc/vanguardgrpc.go#L94-L100

Since `protojson.Unmarshal` (like `json.Unmarshal`) does not work with `nil` as input, it could be fixed like that:

```go
func (g *grpcCodec) Unmarshal(data []byte, v any) error {
	msg, ok := v.(proto.Message)
	if !ok {
		return fmt.Errorf("value is not a proto.Message: %T", v)
	}
	if data == nil {
		return nil
	}
	return g.codec.Unmarshal(data, msg)
}

Another option would be to add some logic to the transcoder that transform http.NoBody to empty message equivalent of the target protocol/target (i.e {}). But it feels like unnecessary overhead, why umarshal, if it is priori clear, that there is nothing to unmarshal?

[ls-aron-kyle]

So I think the fix is to add a case checking for empty requests as needing request prep in protocol_rest.go

vanguard-go/protocol_rest.go

Line 139 in 7aae240

func (r restClientProtocol) requestNeedsPrep(op *operation) bool {

.

The request prep correctly handles empty requests.

func (r restClientProtocol) requestNeedsPrep(op *operation) bool {
	return len(op.restTarget.vars) != 0 ||
		len(op.request.URL.Query()) != 0 ||
		op.restTarget.requestBodyFields != nil ||
		restHTTPBodyRequest(op) ||
		restHTTPBodyRequestIsEmpty(op)
}

// This check enables request prep.
func restHTTPBodyRequestIsEmpty(op *operation) bool {
	return op.request.ContentLength < 0
}

It's actually a fairly rare case as almost anything like a query param or path variable will already trigger the client prep.