http4s / http4s-grpc

https://http4s.github.io/http4s-grpc/
MIT License
38 stars 3 forks source link

Generated code is invalid for well-known types #70

Open andrew-selvia opened 11 months ago

andrew-selvia commented 11 months ago

Based on my experiments, if a message contains a well-known type, http4s-grpc produces invalid code. For example, this protobuf:

syntax = "proto3";
package com.selvia.andrew.demo;
import "google/protobuf/empty.proto";
import "google/protobuf/wrappers.proto";
service Foo {
  rpc Bar (google.protobuf.Empty) returns (stream google.protobuf.Int64Value) {}
}

produces the following code:

trait Foo[F[_]] { 
  def bar(request: _root_.com.google.protobuf.empty.Empty, ctx: _root_.org.http4s.Headers): _root_.fs2.Stream[F, _root_._root_.scala.Long]
}

Notice how the return type has the invalid type prefix _root_._root_.

ScalaPB seems to handle these alright. Will some additional sbt configuration make this work or is this a bug?

ChristopherDavenport commented 11 months ago

This seems like a bug, I think we may have prefixed with root which ends up wrong. 🤔

armanbilge commented 11 months ago

I have a fix for this in https://github.com/davenverse/http4s-grpc/pull/72, but unfortunately more things are broken for well-known types 😕

For example this generated code is broken:

_root_.org.http4s.grpc.codecs.ScalaPb.codecForGenerated(_root_.scala.Long)
armanbilge commented 11 months ago

Actually it's not clear to me at all how to use a Long as a message type 🤔 I think we are going to have to redirect through the Int64Value wrapper type somehow.

andrew-selvia commented 11 months ago

ScalaPB seems to be unwrapping into Long automatically, though that really isn't a blocker. Routing through Int64Value is preferable to invalid code.

armanbilge commented 11 months ago

Yes, to be clear, I'm saying we should still keep Long in the signature, just behind-the-scenes we need to encode that through the Int64Value wrapper to get a valid message.

andrew-selvia commented 8 months ago

Just confirming, #72 isn't fully ready to merge due to the issue mentioned here, right?

armanbilge commented 8 months ago

Ugh, yeah. ScalaPB does it so presumably we should be able to copy/adapt their approach.