[Feature Request] Custom Type conversion

[AlmogBaku]

Adding a future to add custom types(especially to support "gogo's custom type" can be very useful.

In my scenario:

syntax = "proto3";
package org_svc.proto;

import "github.com/gogo/protobuf/gogoproto/gogo.proto";
import "google/api/annotations.proto";

service OrgSvc {
    rpc Read(Organization) returns (Organization) {
        option (google.api.http) = {
            get: "/org/{Id}"
        };
    };
}
message Organization {
    bytes Id = 2 [(gogoproto.nullable) = false, (gogoproto.customtype) = "cyren.io/go/sarameya/uuid.UUID"];
    string name = 3;
}

As you can guess, the UUID is a 16-bytes array that can be also translatable to text(UUID v4).
My custom type is converting from binary and to binary, and I also have a JSON marshaller which un/marshaling text to the binary version.

The thing is, the grpc-gateway treat that as binary, and embed this code instead of handling the UUID from GET param as string(and use the JSON Unmarshaller):

val, ok = pathParams["Id"]
if !ok {
	return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "Id")
}

protoReq.Id, err = runtime.Bytes(val)

if err != nil {
	return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "Id", err)
}

[johanbrandhorst]

Thanks for raising this issue! I like this idea. I think the problem is specific to path and query parameters like this one. Can you confirm whether this works when using a POST where the ID is in the body?

I'm not sure how we would tell the query/path resolver where to look for decoding. Any ideas?

[AlmogBaku]

1. I need to test the POST, but it doesn’t likely to be problematic imo. 2. I suggest to check for the gogo’s custom type and to use the MarshalJSON and Unmarshal json funcs as specified here as “convertFunc” https://github.com/gogo/protobuf/blob/master/custom_types.md

On Thu, 13 Sep 2018 at 0:05 Johan Brandhorst ***@***.***> wrote: Thanks for raising this issue! I like this idea. I think the problem is specific to path and query parameters like this one. Can you confirm whether this works when using a POST where the ID is in the body? I'm not sure how we would tell the query/path resolver where to look for decoding. Any ideas? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#754 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGCpoP-T79_RmfMB53nS3qN31rYR0qqks5uaXctgaJpZM4Wl33t> .

-- <http://www.rimoto.net/> www.rimoto.com <http://www.rimoto.net/> *Almog Baku* *CTO & Cofounder * Mobile: +972.50.2288.744 Social: * <http://www.facebook.com/AlmogBaku> <http://www.linkedin.com/in/almogbaku>*

[johanbrandhorst]

Is there anything I can do to help you get a PR for this going?

[wora]

gRPC and proto3 are language neutral. The mechanical conversion between JSON and proto3 are defined by proto3 spec. The custom type seems to be a Go-specific feature. Such custom conversion exists in many systems, such as Apigee, but such logic can be endless and very hard to maintain. You also need to update OpenAPI generator, documentation generator, and other tools to support these features.

I think it is generally better to handle the conversion in the backend, i.e., creating a separate gRPC method without custom type, the backend converts between method without custom type and method with custom type. The gRPC Gateway just handle the method without custom type. Without this approach, we can handle arbitrary custom types and the backend using the feature carries the burden. The rest of gRPC tooling doesn't need to know anything about it.

[ivanovaleksey]

@johanbrandhorst I have faced with the similar issue. Maybe over the time you have some thoughts about it?

[johanbrandhorst]

I think @wora made good points about this feature request. We certainly cannot include type checks for any gogoproto types. I am going to close this feature request as rejected.

[connyay]

This feature is mentioned in the changelog. Should probably be removed?

[johanbrandhorst]

The changelog is generated so changing it manually will just be reset next time we generate it. I'll update the release notes though.