Does grpc still not support string array
I used ent to create my grpc interface, but the following error occurred:
entproto: failed generating protos: entproto: failed parsing some schemas: unsupported field type "TypeJSON"
True, you can find the support and mapping matrix on the package readme.
Having said that, if you want to contribute support for this use case, I will be happy to help
I feel that I may not be able to deal with this temporarily.
I haven't understood some implementations of ent gen.
@godcong it should not be a very difficult implementation. if you like ping me on slack and I will break it down with you into small steps. I will b very happy to have more people contributing to this part of ent
I have been taking a crack at this (non PR ready code here: https://github.com/jordanrule/contrib), using a structpb Struct to represent the JSON on the proto side, but have run into one issue: JSON on the ent side flexibly accepts any type (https://github.com/ent/ent/blob/master/schema/field/field.go#L73), but the way entproto is wired up it requires an explicit type for the constructor (https://github.com/ent/contrib/blob/e3326c17331d010aff8290ea82df2e0dbc62a268/entproto/cmd/protoc-gen-entgrpc/converter.go#L108). I'm not sure how I should address this, I was looking into ways of interpreting a variety of types as a []byte but I figured I should ask before trying to do anything clever in case my approach is flawed.
hey @jordanrule , thanks for working on this!
I think we can start with a smaller use case before tackling JSONs at large.
In general, at Ent we like working in small increments so I would start by adding support in entproto's Adapter for fields defined with field.Strings, and solve the part on protoc-gen-entgrpc in a subsequent PR.
when a user adds a field with field.Strings("name") it is true that the field type is JSON, but the underlying type is []string.
This can be detected in https://github.com/ent/contrib/blob/2f98d3a15e7dfcc96aa696a5aceb0c8b1249f9e4/entproto/adapter.go#L482 , where the type would be JSON and f.Type.Ident == "[]string". The expected behavior would be that for a schema like
type MessageWithStrings struct {
ent.Schema
}
func (MessageWithStrings) Fields() []ent.Field {
return []ent.Field{
field.Strings("strings").
Annotations(
entproto.Field(2),
),
}
}
func (MessageWithStrings) Annotations() []schema.Annotation {
return []schema.Annotation{
entproto.Message(),
}
}
The generated proto would be
message MessageWithStrings {
int32 id = 1;
repeated string = 2;
}
WDYT?
I got this generating protos as a google.protobuf.ListValue, but of course the type of the internal type is lost in the proto. I think the cleanest way to do this is to add a repeatedType field in the typeConfig struct and treat it as a special case in the adapter (like TypeEnum). Coming back to this as I have time but I feel like I almost have it, as I become more comfortable with the adapter code.
For the special case of field.Strings I think (as mentioned above) it should be mapped to a repeated string field. Don't think ListValue is needed
I'm not sure I understand, there is no type for repeated in the descriptor (https://github.com/protocolbuffers/protobuf-go/blob/fb30439f551a7e79e413e7b4f5f4dfb58e117d73/types/descriptorpb/descriptor.pb.go#L53), the descriptor notes ListValue is the type (https://github.com/protocolbuffers/protobuf-go/blob/fb30439f551a7e79e413e7b4f5f4dfb58e117d73/types/known/structpb/struct.pb.go#L573) which I mapped to a slice of interface as a primitive in my first version. Like I said it came out messy in the proto, which is why I am treating all repeated types as a special case to arrive at your specification.
It's not a type, it's called a label in the proto descriptor lingo. The type would be string and the field would have a label of repeated
Okay, I did see the repeated label but I wasn't sure it would work in tandem with the string type - I will use that approach. Thank you for being patient and explicit while I learn!
Sure thing, thanks for working on this!
this issue now can closed?
I'm sorry, I ended up using rest.
Can't test this
@rotemtam this issue now looks okay, can closed?