Refactor Proto type marshaling
Closed 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.