Protobuf compilation: schema and protocol #66

rafaparadela · 2019-02-13T07:32:00Z

Given a proto file, we wanted to generate of Scala code, and this PR does this.

Included goodness:

Proto3 and proto2 compatible
Infers messages across imported protos
Infers services
Deleted ScalaPB dependency.
Infers nested types
Map types
Coproducts
Add unit test to prove the generation of scala code
Update the docs to show an example

This is the example:

Protobuf

Parsing nested `.proto` files and converting into Scala code

Given these proto files below:

author.proto

syntax = "proto3";
package com.acme;

message Author {
    string name = 1;
    string nick = 2;
}

book.proto

syntax = "proto3";
package com.acme;
import "author.proto";

message Book {
    reserved 4, 8;
    reserved 12 to 15;
    int64 isbn = 1;
    string title = 2;
    repeated Author author = 3;
    BindingType binding_type = 9;
}

message GetBookRequest {
    int64 isbn = 1;
}

message GetBookViaAuthor {
    Author author = 1;
}

message BookStore {
    string name = 1;
    map<int64, string> books = 2;
    repeated Genre genres = 3;

    oneof payment_method {
        int64 credit_card_number = 4;
        int32 cash = 5;
        string iou_note = 6;
        Book barter = 7;
    }
}

enum Genre {
    option allow_alias = true;
    UNKNOWN = 0;
    SCIENCE_FICTION = 1;
    SPECULATIVE_FICTION = 1;
    POETRY = 2;
    SCI_FI = 1;
}

enum BindingType {
    HARDCOVER = 0;
    PAPERBACK = 1;
}

service BookService {
    rpc GetBook (GetBookRequest) returns (Book) {}
    rpc GetBooksViaAuthor (GetBookViaAuthor) returns (stream Book) {}
    rpc GetGreatestBook (stream GetBookRequest) returns (Book) {}
    rpc GetBooks (stream GetBookRequest) returns (stream Book) {}
}

We can parse and convert them into Scala code as:

val source = ParseProto.ProtoSource("book.proto", "resources")
val nativeDescriptor: NativeFile = parseProto[IO].parse(source).unsafeRunSync()

val parseNative: NativeFile => Protocol[Mu[ProtobufF]] = Protocol.fromProto(_)

val parseProtocol: Protocol[Mu[ProtobufF]] => mu.Protocol[Mu[MuF]] =
{ p: Protocol[Mu[ProtobufF]] => mu.Protocol.fromProtobufProto(p) }

val printProtocol: mu.Protocol[Mu[MuF]] => String = { p: mu.Protocol[Mu[MuF]] =>
  higherkindness.skeuomorph.mu.print.proto.print(p)
}

(parseNative andThen parseProtocol andThen printProtocol)(nativeDescriptor))

It would generate:

package com.acme

object book {

  @message final case class Author(name: String, nick: String)
  @message final case class Book(isbn: Long, title: String, author: List[Author], binding_type: BindingType)
  @message final case class GetBookRequest(isbn: Long)
  @message final case class GetBookViaAuthor(author: Author)
  @message final case class BookStore(name: String, books: Map[Long, String], genres: List[Genre], payment_method: Cop[Long :: Int :: String :: Book:: TNil])

  sealed trait Genre
  object Genre {
    case object UNKNOWN extends Genre
    case object SCIENCE_FICTION extends Genre
    case object POETRY extends Genre
  }

  sealed trait BindingType
  object BindingType {
    case object HARDCOVER extends BindingType
    case object PAPERBACK extends BindingType
  }

  @service(Protobuf) trait BookService[F[_]] {
    def GetBook(req: GetBookRequest): F[Book]
    def GetBooksViaAuthor(req: GetBookViaAuthor): Stream[F, Book]
    def GetGreatestBook(req: Stream[F, GetBookRequest]): F[Book]
    def GetBooks(req: Stream[F, GetBookRequest]): Stream[F, Book]
  }
}

…ing protoc-jar maven plugin

…orking through coalgebra implementation

…eam help/feedback

…e working just yet

…ess/skeuomorph into protobuf_compilation # Conflicts: # src/main/scala/protobuf/schema.scala

…tor code still failing

…into protobuf_compilation

…ess/skeuomorph into protobuf_compilation

AdrianRaFo · 2019-02-13T11:38:59Z

Few questions:

sealed trait Genre
  object Genre {
    case object UNKNOWN extends Genre
    case object SCIENCE_FICTION extends Genre
    case object POETRY extends Genre
  }

Why not use scala enum instead?

How do you define an option on proto3?

Should the scala generated code have some imports?

Is this supporting bigdecimal?

rafaparadela · 2019-02-13T19:16:51Z

@juanpedromoreno Interesting. I barely haven't changed the printer, but yes, I should have detected this issue. I'm on it.

rafaparadela · 2019-02-13T19:29:22Z

@AdrianRaFo

Do we want to replace the ADT by scala Enumeration? @pepegar @juanpedromoreno
In proto3, all fields are "optional"
Yesterday at some point I was printing the imports in the Scala representation. But it was adding an extra level of complexity dealing with packages, so I decided to follow another approach (similar that Avro's) and I merged the dependent messages in one single protocol.

  def dependency: Printer[String] = konst("import ") *< string >* konst("._")

No BigDecimal at the moment.

AdrianRaFo · 2019-02-13T19:37:20Z

@rafaparadela I know all fields in proto3 are optional but if you are translating them to non-optional Scala fields, how actually can I set a field as optional? On proto2 is easy because you have the keyword optional but, on proto3, you haven't. Unless you're translating them as an Option, what could result a little ugly but more aligned what proto3 really offers.

rafaparadela · 2019-02-13T22:47:08Z

@juanpedromoreno Solved printer of the operations at 769ff31

rafaparadela · 2019-02-13T23:29:14Z

@AdrianRaFo The long debate about the lack of optional and required in proto 3, of course, affect our implementation. I don't think that we should, by default, translate all the fields into scala Options. As has been implemented, it supposes that all the fields are required and in the case that you want to have an Option in the scala result, you should set as a coproduct with only one value. Thanks to the optimization made by @paco, will be converted into an Option[T]. I hope this answers your question.

AdrianRaFo · 2019-02-13T23:34:10Z

Thanks, rafa just make sure to include into the doc 😉👏

pepegar · 2019-02-14T09:21:44Z

I'd avoid the Enum, I prefer having a proper ADT. @rafaparadela

pepegar

Awesome work @rafaparadela, thank you very much to push this forward! Also, thanks to all other people involved, @rlmark @oli-kitty, you rock!

@rafaparadela , I added a couple of comments and suggestions, none of them are a blocker though. If you prefer merging & addressing those later on, I'm OK with that too!

pepegar · 2019-02-14T09:31:42Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+          fields
+            .filter(t => t.hasOneofIndex && t.getOneofIndex == index)
+            .map(fromFieldDescriptorProto(_, source, files))
+            .collect { case b: NativeField => b })


I believe this b: NativeField is doing a ClassTag check at runtime. Can you change it with a pattern match like:

Suggested change

.collect { case b: NativeField => b })

.collect { case b@NativeField(_, _, _, _, _, _) => b })

Solved at 78581e0

pepegar · 2019-02-14T09:38:00Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+      .map(_.msg)
+  }
+
+  implicit class LabelOps(self: Label) {


Can we make this implicit class private? I'd like to avoid polluting the namespace in case someone imports NativeField._.

Suggested change

implicit class LabelOps(self: Label) {

private implicit class LabelOps(self: Label) {

Solved at 78581e0

pepegar · 2019-02-14T09:38:13Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+    def isRepeated: Boolean = self.name() == "LABEL_REPEATED"
+  }
+
+  implicit class JavaListOps[B](self: util.List[B]) {


same here

Suggested change

implicit class JavaListOps[B](self: util.List[B]) {

private implicit class JavaListOps[B](self: util.List[B]) {

Solved at 78581e0

pepegar · 2019-02-14T09:45:18Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+
+final case class NativeOneOfField(name: String, tpe: NativeOneOf) extends NativeFieldF
+
+sealed trait NativeDescriptor


Following with the conversation in #64 (comment),

What's the use of this ADT? Is it just intermediate representation that will not be exposed to the user? If we want to keep it, can we make it non-recursive at least? We can throw more recursion schemes if so! 😋

I was probably wrong, but I assumed that if I need to define a Coalgebra[ProtobufF, A] to go from A to ProtobufF[A], then A needs to be recursive. At least, I couldn't express Coalgebra[ProtobufF, FileDescriptorSet] <- being FileDescriptorSet the original Java descriptor for the schema of protobuf.
Actually, NativeDescriptor is not a 1-to-1 representation of FileDescriptorSet, but contains a lot of business logic to aggregate data, and to discard unnecessary fields down the road.
Frankly, I'd love to try Coalgebra[ProtobufF, FileDescriptorSet] again with your help.

Solved at 7ce48d6

pepegar · 2019-02-14T09:48:16Z

src/main/scala/higherkindness/skeuomorph/protobuf/schema.scala

+  def toTOption(no: NativeOption): ProtobufF.Option =
+    ProtobufF.Option(no.name, no.value)
+
+  def toJson: Algebra[ProtobufF, Json] = Algebra {


What are we using this for? Is there a JSON representation for Protobuf?

If we're using this to translate to Avro, I believe that the correct path would be:

Trans[ProtobufF, AvroF] and then Algebra[AvroF, Json]

We can tackle this in a different PR, I have removed the Algebra[ProtobufF, Json], since is not being used so far.

pepegar · 2019-02-14T09:51:31Z

src/test/scala/higherkindness/skeuomorph/protobuf/ProtobufProtocolSpec.scala

+      |  def GetBooks(req: Stream[F, GetBookRequest]): Stream[F, Book]
+      |}
+      |
+      |}""".stripMargin


👏 👏 👏 👏 👏 !!!!!! 🎉 🎉 🎈 🎉!!!

THIS IS AWESOME

juanpedromoreno · 2019-02-14T13:56:32Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+
+final case class NativeOneOfField(name: String, tpe: NativeOneOf) extends NativeFieldF
+
+sealed trait NativeDescriptor


juanpedromoreno · 2019-02-14T13:58:03Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+          file.getServiceList.j2s.map(s => toNativeService(s, files))
+        )
+      }
+      .getOrElse(throw new Exception(s"Could not find descriptors for: $descriptorFileName"))


Could we use some of the defined errors here, rather than Exception?

Solved at 78581e0

juanpedromoreno · 2019-02-14T13:58:21Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+    val (hasAlias, noAlias) = valuesAndAliases.groupBy(_.getNumber).values.partition(_.lengthCompare(1) > 0)
+    val separateValueFromAliases: Iterable[(EnumValueDescriptorProto, List[EnumValueDescriptorProto])] = hasAlias.map {
+      case h :: t => (h, t)
+      case _      => throw new Exception(s"Wrong number of aliases")


Same here: Could we use some of the defined errors here, rather than Exception?

Solved at 78581e0

juanpedromoreno · 2019-02-14T13:58:29Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+      maybeKey   <- getMapField(maybeMsg, "key")
+      maybeValue <- getMapField(maybeMsg, "value")
+    } yield NativeMap(fromFieldType(maybeKey, files), fromFieldType(maybeValue, files)))
+      .getOrElse(throw new Exception(s"Could not find map entry for: $name"))


and here: Could we use some of the defined errors here, rather than Exception?

Solved at 78581e0

juanpedromoreno · 2019-02-14T13:58:41Z

src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala

+            .filter(t => t.hasOneofIndex && t.getOneofIndex == index)
+            .map(fromFieldDescriptorProto(_, source, files))
+            .collect { case b: NativeField => b })
+        .getOrElse(throw new Exception(s"Empty set of fields in OneOf: ${oneof.getName}"))


and here: Could we use some of the defined errors here, rather than Exception?

Solved at 78581e0

juanpedromoreno · 2019-02-14T14:02:09Z

Regarding #66 (comment), I'd prefer ADT for enums too.

# Conflicts: # src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala # src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala # src/main/scala/higherkindness/skeuomorph/protobuf/schema.scala

rafaparadela · 2019-02-18T16:00:40Z

@juanpedromoreno Finally the intermediate NativeDescriptor has been removed with the great help of @pepegar . I think this is ready to be merged (once we got a green from CI)

pepegar

Looks awesome to me! 👍

rlmark and others added 30 commits November 29, 2018 10:56

WIP: Protobuf files can be compiled to ProtoDescriptors at runtime us…

1a32eae

…ing protoc-jar maven plugin

WIP: Switching things up and potentially using scala protoc library w…

37635da

…orking through coalgebra implementation

WIP: Moved Parser work to its own file

a90fb79

WIP: rewrote Parser with Resource for file handling

e3dbc87

WIP: Adding simpleProto.proto file in resources for testing

997ce49

WIP: adding back in missing sbt config

8a433a0

Working code for reading file and getting FileDescriptorSet

0235e9b

WIP: protbuf coalgebra in progress and test started

06244a9

WIP: Cleaning up createEnum and createMessage methods

8c3adfc

WIP: 2 competing coalgebra methods

bbfca6c

WIP: reading FileDescroptor into ProtoF has Playground instance for t…

1ddebc2

…eam help/feedback

Working coalgebra: ProtobufF does not need covariance

92a700a

Adding OptionalField to protobuf coalgebra and nicer fileStream closing

6db5707

WIP adding Options for Enums and Messages

452160c

combine TFileDescriptor parameters into one list

b679222

WIP adding Options to FileDescriptor

ca99e05

WIP demonstrating 3 different implementations for Protobuf Specs, non…

2a54c67

…e working just yet

Merge branch 'protobuf_compilation' of https://github.com/higherkindn…

2f15ffc

…ess/skeuomorph into protobuf_compilation # Conflicts: # src/main/scala/protobuf/schema.scala

WIP: Renaming ProtobufF.FileDescriptor's field from messages to values

253eca0

Compiling proto test code, fails for stack overflow error

6c05739

WIP: Enum generator created, problem in TFileDescriptor fixed, Genera…

1b8723f

…tor code still failing

Property driven protobuf specs passing.

ee67509

refactoring test to make it less repetitive, adding ProtobufF.TRequired

5f3cca3

Bringing back TOptional and TRepeated, test fixed

1c629ca

Adding TRequired and TRepeated back to functor

30a49af

Adding Trequred and Trepeated back to schema

5e5c89b

Bringing back in TRepeated and adjusting tests for bug reproduction

7c9fd2e

Merge branch 'master' of https://github.com/higherkindness/skeuomorph …

bb80f69

…into protobuf_compilation

add version variables for dependencies in build.sbt

3e3423a

Merge branch 'protobuf_compilation' of https://github.com/higherkindn…

7a30dd8

…ess/skeuomorph into protobuf_compilation

rafaparadela added 2 commits February 13, 2019 14:37

fixes operation printer

769ff31

improves operations printer

7294958

fixes minor compiling issue in Scala 2.11

a4b1e99

pepegar reviewed Feb 14, 2019

View reviewed changes

juanpedromoreno reviewed Feb 14, 2019

View reviewed changes

rafaparadela and others added 6 commits February 14, 2019 13:51

address some comments from the code review

78581e0

delete the intermediate native AST

7ce48d6

fixes tests

7b06193

fixes tests

a1b41ce

Merge branch 'ppg/delete-native-ast' into all-protobuf-support

f4df2af

# Conflicts: # src/main/scala/higherkindness/skeuomorph/protobuf/Native.scala # src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala # src/main/scala/higherkindness/skeuomorph/protobuf/schema.scala

fixes the inference of oneof fields

735522a

pepegar approved these changes Feb 18, 2019

View reviewed changes

juanpedromoreno approved these changes Feb 18, 2019

View reviewed changes

rafaparadela merged commit 6688e3c into master Feb 18, 2019

rafaparadela deleted the all-protobuf-support branch February 18, 2019 16:35

juanpedromoreno mentioned this pull request Mar 18, 2019

Parse proto files #30

Closed

5 tasks

diesalbla added Protocol Buffers and removed Protocol Buffers labels Jul 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protobuf compilation: schema and protocol #66

Protobuf compilation: schema and protocol #66

rafaparadela commented Feb 13, 2019 •

edited

Loading

AdrianRaFo commented Feb 13, 2019

rafaparadela commented Feb 13, 2019

rafaparadela commented Feb 13, 2019 •

edited

Loading

AdrianRaFo commented Feb 13, 2019 •

edited

Loading

rafaparadela commented Feb 13, 2019

rafaparadela commented Feb 13, 2019

AdrianRaFo commented Feb 13, 2019

pepegar commented Feb 14, 2019

pepegar left a comment

pepegar Feb 14, 2019

rafaparadela Feb 14, 2019

pepegar Feb 14, 2019

rafaparadela Feb 14, 2019

pepegar Feb 14, 2019

rafaparadela Feb 14, 2019

pepegar Feb 14, 2019

juanpedromoreno Feb 14, 2019

rafaparadela Feb 14, 2019

rafaparadela Feb 18, 2019

pepegar Feb 14, 2019

rafaparadela Feb 14, 2019

pepegar Feb 14, 2019

juanpedromoreno Feb 14, 2019

juanpedromoreno Feb 14, 2019

rafaparadela Feb 14, 2019

juanpedromoreno Feb 14, 2019

rafaparadela Feb 14, 2019

juanpedromoreno Feb 14, 2019

rafaparadela Feb 14, 2019

juanpedromoreno Feb 14, 2019

rafaparadela Feb 14, 2019

juanpedromoreno commented Feb 14, 2019

rafaparadela commented Feb 18, 2019

pepegar left a comment

	.collect { case b: NativeField => b })
	.collect { case b@NativeField(_, _, _, _, _, _) => b })

	implicit class LabelOps(self: Label) {
	private implicit class LabelOps(self: Label) {

	implicit class JavaListOps[B](self: util.List[B]) {
	private implicit class JavaListOps[B](self: util.List[B]) {


		final case class NativeOneOfField(name: String, tpe: NativeOneOf) extends NativeFieldF

		sealed trait NativeDescriptor

Protobuf compilation: schema and protocol #66

Protobuf compilation: schema and protocol #66

Conversation

rafaparadela commented Feb 13, 2019 • edited Loading

Protobuf

Parsing nested .proto files and converting into Scala code

AdrianRaFo commented Feb 13, 2019

rafaparadela commented Feb 13, 2019

rafaparadela commented Feb 13, 2019 • edited Loading

AdrianRaFo commented Feb 13, 2019 • edited Loading

rafaparadela commented Feb 13, 2019

rafaparadela commented Feb 13, 2019

AdrianRaFo commented Feb 13, 2019

pepegar commented Feb 14, 2019

pepegar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpedromoreno commented Feb 14, 2019

rafaparadela commented Feb 18, 2019

pepegar left a comment

Choose a reason for hiding this comment

rafaparadela commented Feb 13, 2019 •

edited

Loading

Parsing nested `.proto` files and converting into Scala code

rafaparadela commented Feb 13, 2019 •

edited

Loading

AdrianRaFo commented Feb 13, 2019 •

edited

Loading