ent icon indicating copy to clipboard operation
ent copied to clipboard

Does grpc still not support string array

Open godcong opened this issue 4 years ago • 14 comments

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"

godcong avatar May 21 '21 04:05 godcong

True, you can find the support and mapping matrix on the package readme.

rotemtam avatar May 21 '21 04:05 rotemtam

Having said that, if you want to contribute support for this use case, I will be happy to help

rotemtam avatar May 21 '21 04:05 rotemtam

I feel that I may not be able to deal with this temporarily. I haven't understood some implementations of ent gen.

godcong avatar Jul 14 '21 10:07 godcong

@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

rotemtam avatar Jul 14 '21 11:07 rotemtam

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.

jordanrule avatar Oct 21 '21 18:10 jordanrule

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?

rotemtam avatar Oct 27 '21 08:10 rotemtam

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.

jordanrule avatar Nov 05 '21 16:11 jordanrule

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

rotemtam avatar Nov 05 '21 16:11 rotemtam

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.

jordanrule avatar Nov 05 '21 16:11 jordanrule

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

rotemtam avatar Nov 06 '21 13:11 rotemtam

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!

jordanrule avatar Nov 06 '21 20:11 jordanrule

Sure thing, thanks for working on this!

rotemtam avatar Nov 07 '21 07:11 rotemtam

this issue now can closed? I'm sorry, I ended up using rest. Can't test this

godcong avatar Oct 31 '22 10:10 godcong

@rotemtam this issue now looks okay, can closed?

godcong avatar Jul 25 '24 10:07 godcong