Refactor Proto type marshaling
alehechka opened this issue · 2 comments
Due to the changes made on #14, the default behavior of the generated GraphQL resolvers is to use the custom MarshalResponse function which makes use of a series of recursive calls to marshal the provided interface into a more generic structure with json_name appropriate keys.
However, It is not ideal to use this because it is one extra thing this package has to deal with and since it is now the default behavior after generation, it leads to a much greater dependency on this custom marshaling.
A more ideal solution would be to of course not introduce an intermediate marshaling step at all, but due to the protobuf compiler not generating the json tag with appropriate json_name from the .proto spec file, it leads to naming conventions of fields not matching and thus not being returned correctly from GraphQL.
The following code change shows a possible solution that simply uses the jsonpb package (that is also being used for MarshalRequest.
Console logging both the above code changes as well as the output of the current custom marshaling shows that they are near identical. The only difference being that in the starwars.proto example the returned id field in the jsonpb functions returns as a string instead of int64. However, this has shown to be an oddity with int64 in general and after it goes through the graphql resolver it is outputted as an integer regardless.
However, after running through the graphql resolver, all enum values are null. The output for these enums is the same after either custom marshaling steps, but after the graphql resolver, the jsonpb version only returns nulls. If this can be overcome, this would be the much preferred solution to this problem as it would result in much less custom code and rely on a more battle hardened solution.
Figured this problem out. After re-reading the docs for the standard encoding/json packages Marshal and Unmarshal libraries, it reminded me that JSON numbers get unmarshaled as float64. The current template.go will generate the enum values as int32(0) which causes the graphql resolver to not equate the values for deciding which enum string to return.
Changing the generated enum values to instead cast as float64(0) does resolve the issue of enums returning from the graphql-gateway as null.
However, after further thought I'm releasing that this shorter, simpler solution may actually be less performant. The reasoning is due to first marshaling the proto message into a bytes.Buffer as encoded JSON. Then decoding the JSON buffer into a Go interface{} that will be returned from the function. This requires both of those steps before it hits the graphql resolver which undoubtedly also does some kind of encoding.
The current custom solution in place, while more complex, and and extra thing to manage for this code base, does lend itself to not having this marshal/unmarshal step. It simply takes a raw interface{} and uses reflection to transform the entire provided object into a new one that uses the protobuf tag to extract the key for each variable in the object.
Before I close this issue, I am going to give this some more thought and potentially try and test some larger datasets to check performance.
Additionally, the current jsonpb marshaling solution takes protoiface.MessageV1 as its only parameter which is fine for most cases, but if the pluck option is used within a service it will pluck a child object from the parent and in the base example for starwars, the humans and droids queries end up returning a slice of protoiface.MessageV1 objects which causes a mismatch type compile error. This would undoubtedly need to be coded for and end up adding more complexity to the jsonpb solution.
Sadly another con against the jsonpb marshaling solution.