Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Bytes and Enum handlers to proto3ConvertFuncs #423

Open
gdbelvin opened this issue Jun 27, 2017 · 9 comments
Open

Add Bytes and Enum handlers to proto3ConvertFuncs #423

gdbelvin opened this issue Jun 27, 2017 · 9 comments

Comments

@gdbelvin
Copy link

This is an issue to track the TODO on this line in types.go.

var (
	proto3ConvertFuncs = map[descriptor.FieldDescriptorProto_Type]string{
		descriptor.FieldDescriptorProto_TYPE_STRING:  "runtime.String",
		// FieldDescriptorProto_TYPE_GROUP
		// FieldDescriptorProto_TYPE_MESSAGE
		// FieldDescriptorProto_TYPE_BYTES
		// TODO(yugui) Handle bytes
		descriptor.FieldDescriptorProto_TYPE_UINT32: "runtime.Uint32",
		// FieldDescriptorProto_TYPE_ENUM
		// TODO(yugui) Handle Enum
	}

)
@lucasvo
Copy link
Contributor

lucasvo commented Feb 7, 2018

Bytes was implemented in: #489

@lucasvo
Copy link
Contributor

lucasvo commented Feb 7, 2018

@loderunner, thanks for implementing the bytes support.

I've been wondering if anyone ever considered encoding bytes as a hex string instead of base64. I'd love to make this configurable. I've been digging through the different runtime.convert methods to see if there is any support for configuration in runtime.convert or in the ConvertFuncExpr.

But it looks like there isn't really anything that would do something similar.

I might go ahead and propose an implementation but I've wanted to open the discussion here first to hear what you think.

@loderunner
Copy link
Collaborator

@lucasvo I went for base64 because it is the standard mapping for protobuf bytes. bytes were already handled for methods other than GET that supported sending JSON in the body. It did indeed use the standard base64 mapping from protobuf. I only added support for bytes fields in as URL variables.

I'm curious as to how you would want to configure this. It seems to me the operation of the gRPC protoc plugin is fully automatic, and based on the HTTP annotations. Also, wouldn't this break the JSON mapping of protocol buffers? How would you handle having a message be hex characters in a URL variable, but base64 in the request body?

I'm interested in how you'd do this, keep me posted if you get started, and don't be shy to ask questions.

@lucasvo
Copy link
Contributor

lucasvo commented Feb 11, 2018

Thanks for the comment @loderunner

Having spent a bit more time with protobuf in the past couple days your argument makes sense. I didn't know the base64 encoding was a protobuf default.

However one small detail I'm trying to fix is the padding: Is there a way to get it to accept a padded value or remove the padding from the json response body?

# URL is /v1/document/get/{documentIdentifier} & returns the same message  in the body.
$ curl -X GET "https://localhost:8082/v1/document/get/QM%3D%3D" -H "accept: application/json" -k
{"error":"type mismatch, parameter: documentIdentifier, error: illegal base64 data at input byte 2","code":3}

$ curl -X GET "https://localhost:8082/v1/document/get/QM" -H "accept: application/json" -k
{"documentIdentifier":"QA=="}

@loderunner
Copy link
Collaborator

I agree. I first thought I'd use url-safe base64 encoding with no padding, to make sure we could send it in a URL. But I now think the discrepancy between the JSON body and the URL variable is unacceptable. We'll just have to make sure unsafe characters are correctly escaped.

I won't be doing it right now, but if you want to check, I can give you a couple of pointers.

You'll want to try replacing base64.RawURLEncoding with base64.StdEncoding. Keep me posted on how this goes.

@lucasvo
Copy link
Contributor

lucasvo commented Feb 12, 2018

I can take care of changing this. I'll try to get a PR ready for this in the next couple of days.

@lucasvo
Copy link
Contributor

lucasvo commented Mar 1, 2018

@loderunner Hey, I just tried to get make test to pass and it looks like I'm stuck with incompatible versions. swagger-codegen generates code that references ByteArray in the generated code which is nowhere to be found when I install version 1.2.6 (as described in CONTRIBUTING.md) when I run the newest version of swagger-codegen there are other issues. Do you know what version of swagger-codegen you ran to get it to work?

# github.com/grpc-ecosystem/grpc-gateway/examples/clients/abe/abe
examples/clients/abe/abe/CamelCaseServiceNameApi.go:4:5: imported and not used: "strings"
examples/clients/abe/abe/CamelCaseServiceNameApi.go:5:5: imported and not used: "fmt"
examples/clients/abe/abe/ExamplepbABitOfEverything.go:20:17: undefined: ByteArray

@loderunner
Copy link
Collaborator

The CI on travis validates against protoc 3.1.0 and swagger-codegen 2.2.2

Look here and here to get the details.

@lucasvo
Copy link
Contributor

lucasvo commented Mar 4, 2018

@loderunner would love to get your feedback on #565 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants