Merge pull request #339 from higherkindness/issue337

Extend package choosing behavior to protobuf files with `Enum` types

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

@@ -47,17 +47,16 @@ object ParseProto {
   )
 
   final case class NamedDescriptor[A](
-      pkg: String,
+      scalaPackage: String,
+      filePackage: String,
       enclosingProto: String,
       parentMessageNames: List[String],
       name: String,
       desc: A
   ) {
 
-    val pkgParts = pkg.split('.').toList
-
     /*
-     * This is the full name that other protobuf messages will
+     * This is the full protobuf name that other protobuf messages will
      * use to reference this type.
      * e.g. .com.acme.Book
      *
@@ -66,15 +65,15 @@ object ParseProto {
      * with the name of the .proto file,
      * e.g. com.acme.book.Book
      */
-    def fullName: String =
-      List(pkgParts, parentMessageNames, List(name)).flatten.mkString(".", ".", "")
+    val fullProtoName: String =
+      (List(".", filePackage) ++ parentMessageNames :+ name).mkString(".")
 
     /*
      * The path to the Scala class that corresponds to this type.
      * e.g. com.acme.book
      */
-    def scalaPrefix: List[String] =
-      List(pkgParts, List(enclosingProto), parentMessageNames).flatten
+    val scalaPrefix: List[String] =
+      (scalaPackage.split('.').toList :+ enclosingProto) ++ parentMessageNames
 
   }
 
@@ -121,7 +120,7 @@ object ParseProto {
     }
   }
 
-  private def getPackageOrJavaPackage(file: FileDescriptorProto): String = {
+  private def scalaPackage(file: FileDescriptorProto): String = {
 
     val javaPackage: String = file.getOptions.getJavaPackage
 
@@ -138,17 +137,6 @@ object ParseProto {
 
   }
 
-  private def getTypeNameFromQualifiedName(fullName: String): String =
-    // When comparing the types, we only need to know the name of the type, not the
-    // fully-qualified name (since we use the value of the package passed into the protoc to create
-    // the TNamedType.  However, depending on how we access the type name via `getTypeName`, there is
-    // different behavior depending on whether we access a type name from a file with a `package` header
-    // (which will return the fully-qualified name) vs accessing a type name from a file with a `java_package`
-    // header (which will just return the name of the type).  This method trims the type name from a fully-
-    // qualified name if necessary, which provides consistent behavior when comparing the name of a message
-    // to the name of a type
-    fullName.substring(fullName.lastIndexOf(".") + 1)
-
   def fromProto[A](
       descriptorFileName: String,
       files: List[FileDescriptorProto]
@@ -163,7 +151,7 @@ object ParseProto {
 
         Protocol[A](
           fileName,
-          getPackageOrJavaPackage(file),
+          scalaPackage(file),
           Nil,
           messages ++ enums,
           file.getServiceList.asScala.toList.map(s => toService[A](s, files)),
@@ -207,7 +195,7 @@ object ParseProto {
    * `value`. A better explanation is available in the protobuf docs:
    * https://developers.google.com/protocol-buffers/docs/proto3#backwards-compatibility
    *
-   * The protoc tool generates types for these 'psuedo-message' pairs, but we
+   * The protoc tool generates types for these 'pseudo-message' pairs, but we
    * don't care about them (we don't want to generate code for them) so we want
    * to filter them out when collecting message types.
    */
@@ -437,7 +425,8 @@ object ParseProto {
 
       def rec(m: DescriptorProto, parents: List[String]): List[NamedDescriptor[DescriptorProto]] =
         NamedDescriptor(
-          getPackageOrJavaPackage(f),
+          scalaPackage(f),
+          f.getPackage,
           enclosingProto,
           parents,
           m.getName,
@@ -451,16 +440,13 @@ object ParseProto {
   }
 
   def findMessage(name: String, files: List[FileDescriptorProto]): Option[NamedDescriptor[DescriptorProto]] =
-    // Note that this method checks that the name of the message matches against the type name that is
-    // extracted from the fully-qualified type name via `getTypeNameFromQualifiedName`.  More context
-    // on why that happens is in the method signature for `getTypeNameFromQualifiedName`.
-    allMessages(files).find(_.fullName.contains(getTypeNameFromQualifiedName(name)))
+    allMessages(files).find(_.fullProtoName.endsWith(name))
 
   def findEnum(name: String, files: List[FileDescriptorProto]): Option[NamedDescriptor[EnumDescriptorProto]] = {
     val allTopLevel: List[NamedDescriptor[EnumDescriptorProto]] = files.flatMap { f =>
       val enclosingProto = formatName(f.getName)
       f.getEnumTypeList.asScala.toList.map(e =>
-        NamedDescriptor(getPackageOrJavaPackage(f), enclosingProto, Nil, e.getName, e)
+        NamedDescriptor(scalaPackage(f), f.getPackage, enclosingProto, Nil, e.getName, e)
       )
     }
 
@@ -469,11 +455,9 @@ object ParseProto {
       e <- m.desc.getEnumTypeList.asScala
     } yield {
       val parentMessageNames = m.parentMessageNames :+ m.name
-      NamedDescriptor(m.pkg, m.enclosingProto, parentMessageNames, e.getName, e)
+      m.copy(parentMessageNames = parentMessageNames, name = e.getName, desc = e)
     }
-
-    (allTopLevel ++ allNestedInsideMessages)
-      .find(_.fullName === name)
+    (allTopLevel ++ allNestedInsideMessages).find(_.fullProtoName.endsWith(name))
   }
 
   implicit class LabelOps(self: Label) {

src/test/resources/protobuf/packages/test_enum_java_package.proto

@@ -0,0 +1,14 @@
+syntax = "proto3";
+
+package example;
+
+option java_package="better_example";
+
+message Example {
+    ExampleEnum enum = 1;
+}
+
+enum ExampleEnum {
+    Option1 = 0;
+    Option2 = 1;
+}

src/test/resources/protobuf/packages/test_enum_no_package.proto

@@ -0,0 +1,10 @@
+syntax = "proto3";
+
+message Example {
+    ExampleEnum enum = 1;
+}
+
+enum ExampleEnum {
+    Option1 = 0;
+    Option2 = 1;
+}

src/test/resources/protobuf/packages/test_enum_only_java_package.proto

@@ -0,0 +1,12 @@
+syntax = "proto3";
+
+option java_package="better_example";
+
+message Example {
+    ExampleEnum enum = 1;
+}
+
+enum ExampleEnum {
+    Option1 = 0;
+    Option2 = 1;
+}

src/test/resources/protobuf/packages/test_enum_package.proto

@@ -0,0 +1,12 @@
+syntax = "proto3";
+
+package example;
+
+message Example {
+    ExampleEnum enum = 1;
+}
+
+enum ExampleEnum {
+    Option1 = 0;
+    Option2 = 1;
+}

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

@@ -57,12 +57,20 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
 
   The generated Scala code should use the `java_package` option when `package` isn't present in the file but the `java_package` is. $codeGenProtobufOnlyJavaPackage
 
-  The generated Scala code should use the `java_package` option when both `package` and `java_package` are present in a file. $codeGenProtobufJavaPackage
+  The generated Scala code should use the `java_package` option when both `package` and `java_package` are present in a file. $codeGenProtobufBothPackages
 
-  The generated Scala code should use the filename as a package option when neither `package` nor `java_package` are present in a file. $codegenProtobufNoPackage
+  The generated Scala code should use the filename as the package name when neither `package` nor `java_package` are present in a file. $codegenProtobufNoPackage
+
+  The generated Scala code should handle enums when `package` is not present in a file. $codeGenProtobufEnumWithPackage
+
+  The generated Scala code should use the `java_package` option for enums when `package` is present in a file. $codeGenProtobufEnumOnlyJavaPackage
+
+  The generated Scala code should use the filename for enums when neither `package` nor `java_package` in a file. $codeGenProtobufEnumNoPackage
+
+  The generated Scala code should handle enums when and use the `java_package` when both `package` and `java_package` are present in a file. $codeGenProtobufEnumBothPackages
   """
 
-  def codegenProtobufProtocol =
+  private def codegenProtobufProtocol =
     prop { (compressionType: CompressionType, useIdiomaticEndpoints: Boolean) =>
       val toMuProtocol: protobuf.Protocol[Mu[ProtobufF]] => mu.Protocol[Mu[MuF]] = { p =>
         mu.Protocol.fromProtobufProto(compressionType, useIdiomaticEndpoints)(p)
@@ -95,7 +103,7 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
       """.stripMargin
     }
 
-  def codegenExpectation(
+  private def codegenExpectation(
       compressionType: CompressionType,
       namespace: Option[String],
       useIdiomaticEndpoints: Boolean
@@ -227,7 +235,7 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     check(opencensusProtocol, opencensusExpectation)
   }
 
-  val opencensusExpectation = s"""
+  private val opencensusExpectation = s"""
     |package opencensus.proto.trace.v1
     |
     |import _root_.higherkindness.mu.rpc.protocol._
@@ -353,7 +361,7 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     |  )
     |}""".stripMargin
 
-  def codegenTaggedIntegers = {
+  private def codegenTaggedIntegers = {
     val integerTypesProtocol: protobuf.Protocol[Mu[ProtobufF]] = {
       val path   = workingDirectory + s"$testDirectory/models"
       val source = ProtoSource(s"integer_types.proto", path, importRoot)
@@ -363,7 +371,7 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     check(integerTypesProtocol, taggedIntegersExpectation)
   }
 
-  val taggedIntegersExpectation = s"""
+  private val taggedIntegersExpectation = s"""
     |package com.acme
     |
     |import _root_.higherkindness.mu.rpc.protocol._
@@ -394,7 +402,7 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     check(googleApiProtocol, googleApiExpectation)
   }
 
-  val googleApiExpectation = s"""
+  private val googleApiExpectation = s"""
     |package com.google.`type`
     |import _root_.higherkindness.mu.rpc.protocol._
     |object date { final case class Date(@_root_.pbdirect.pbIndex(1) year: _root_.scala.Int, @_root_.pbdirect.pbIndex(2) month: _root_.scala.Int, @_root_.pbdirect.pbIndex(3) day: _root_.scala.Int) }
@@ -410,7 +418,7 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     check(optionalPackage, protobufOnlyJavaPackage)
   }
 
-  val protobufOnlyJavaPackage =
+  private val protobufOnlyJavaPackage =
     s"""
        |package my_package
        |import _root_.higherkindness.mu.rpc.protocol._
@@ -433,7 +441,7 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     check(optionalPackage, protobufNoJavaPackage)
   }
 
-  val protobufNoJavaPackage =
+  private val protobufNoJavaPackage =
     s"""
     |package test_no_package
     |import _root_.higherkindness.mu.rpc.protocol._
@@ -446,17 +454,17 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     |}
     |""".stripMargin
 
-  private def codeGenProtobufJavaPackage = {
+  private def codeGenProtobufBothPackages = {
     val javaPackageAndRegularPackage: protobuf.Protocol[Mu[ProtobufF]] = {
       val path   = workingDirectory + s"$testDirectory/packages"
       val source = ProtoSource(s"test_java_package.proto", path, importRoot)
       parseProto[IO, Mu[ProtobufF]].parse(source).unsafeRunSync()
     }
 
-    check(javaPackageAndRegularPackage, protobufJavaPackageExpectation)
+    check(javaPackageAndRegularPackage, protobufJavaPackageAndRegularPackageExpectation)
   }
 
-  val protobufJavaPackageExpectation =
+  private val protobufJavaPackageAndRegularPackageExpectation =
     s"""
     |package my_package
     |import _root_.higherkindness.mu.rpc.protocol._
@@ -469,4 +477,103 @@ class ProtobufProtocolSpec extends Specification with ScalaCheck {
     |}
     |""".stripMargin
 
+  private def codeGenProtobufEnumWithPackage = {
+    val enumWithPackage: protobuf.Protocol[Mu[ProtobufF]] = {
+      val path   = workingDirectory + s"$testDirectory/packages"
+      val source = ProtoSource(s"test_enum_package.proto", path, importRoot)
+      parseProto[IO, Mu[ProtobufF]].parse(source).unsafeRunSync()
+    }
+
+    check(enumWithPackage, protobufEnumWithPackageExpectation)
+  }
+
+  private val protobufEnumWithPackageExpectation =
+    s"""
+    |package example
+    |import _root_.higherkindness.mu.rpc.protocol._
+    |object test_enum_package {
+    |  final case class Example(@_root_.pbdirect.pbIndex(1) enum: _root_.scala.Option[_root_.example.test_enum_package.ExampleEnum])
+    |  sealed abstract class ExampleEnum(val value: _root_.scala.Int) extends _root_.enumeratum.values.IntEnumEntry
+    |  object ExampleEnum extends _root_.enumeratum.values.IntEnum[ExampleEnum] {
+    |    case object Option1 extends ExampleEnum(0)
+    |    case object Option2 extends ExampleEnum(1)
+    |    val values = findValues
+    |  }
+    |}
+    |""".stripMargin
+
+  private def codeGenProtobufEnumOnlyJavaPackage = {
+    val enumOnlyJavaPackage: protobuf.Protocol[Mu[ProtobufF]] = {
+      val path   = workingDirectory + s"$testDirectory/packages"
+      val source = ProtoSource(s"test_enum_only_java_package.proto", path, importRoot)
+      parseProto[IO, Mu[ProtobufF]].parse(source).unsafeRunSync()
+    }
+
+    check(enumOnlyJavaPackage, protobufEnumOnlyJavaPackageExpectation)
+  }
+
+  private val protobufEnumOnlyJavaPackageExpectation =
+    s"""
+    |package better_example
+    |import _root_.higherkindness.mu.rpc.protocol._
+    |object test_enum_only_java_package {
+    |  final case class Example(@_root_.pbdirect.pbIndex(1) enum: _root_.scala.Option[_root_.better_example.test_enum_only_java_package.ExampleEnum])
+    |  sealed abstract class ExampleEnum(val value: _root_.scala.Int) extends _root_.enumeratum.values.IntEnumEntry
+    |  object ExampleEnum extends _root_.enumeratum.values.IntEnum[ExampleEnum] {
+    |    case object Option1 extends ExampleEnum(0)
+    |    case object Option2 extends ExampleEnum(1)
+    |    val values = findValues
+    |  }
+    |}
+    |""".stripMargin
+
+  private def codeGenProtobufEnumNoPackage = {
+    val enumWithNoPackage: protobuf.Protocol[Mu[ProtobufF]] = {
+      val path   = workingDirectory + s"$testDirectory/packages"
+      val source = ProtoSource(s"test_enum_no_package.proto", path, importRoot)
+      parseProto[IO, Mu[ProtobufF]].parse(source).unsafeRunSync()
+    }
+
+    check(enumWithNoPackage, protobufEnumNoPackageExpectation)
+  }
+
+  private val protobufEnumNoPackageExpectation =
+    s"""
+    |package test_enum_no_package
+    |import _root_.higherkindness.mu.rpc.protocol._
+    |object test_enum_no_package {
+    |  final case class Example(@_root_.pbdirect.pbIndex(1) enum: _root_.scala.Option[_root_.test_enum_no_package.test_enum_no_package.ExampleEnum])
+    |  sealed abstract class ExampleEnum(val value: _root_.scala.Int) extends _root_.enumeratum.values.IntEnumEntry
+    |  object ExampleEnum extends _root_.enumeratum.values.IntEnum[ExampleEnum] {
+    |    case object Option1 extends ExampleEnum(0)
+    |    case object Option2 extends ExampleEnum(1)
+    |    val values = findValues
+    |  }
+    |}
+    |""".stripMargin
+
+  private def codeGenProtobufEnumBothPackages = {
+    val enumWithJavaPackageAndRegularPackage: protobuf.Protocol[Mu[ProtobufF]] = {
+      val path   = workingDirectory + s"$testDirectory/packages"
+      val source = ProtoSource(s"test_enum_java_package.proto", path, importRoot)
+      parseProto[IO, Mu[ProtobufF]].parse(source).unsafeRunSync()
+    }
+
+    check(enumWithJavaPackageAndRegularPackage, protobufEnumBothPackagesExpectation)
+  }
+
+  private val protobufEnumBothPackagesExpectation =
+    s"""
+    |package better_example
+    |import _root_.higherkindness.mu.rpc.protocol._
+    |object test_enum_java_package {
+    |  final case class Example(@_root_.pbdirect.pbIndex(1) enum: _root_.scala.Option[_root_.better_example.test_enum_java_package.ExampleEnum])
+    |  sealed abstract class ExampleEnum(val value: _root_.scala.Int) extends _root_.enumeratum.values.IntEnumEntry
+    |  object ExampleEnum extends _root_.enumeratum.values.IntEnum[ExampleEnum] {
+    |    case object Option1 extends ExampleEnum(0)
+    |    case object Option2 extends ExampleEnum(1)
+    |    val values = findValues
+    |  }
+    |}
+    |""".stripMargin
 }