Imply idempotent when safe is set. Emit options only when idempotency…

… option is present. Add tests
scalapb · Nov 29, 2024 · 2d2f5aa · 2d2f5aa
1 parent f472ae9
commit 2d2f5aa
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 9 deletions.
diff --git a/compiler-plugin/src/main/scala/scalapb/compiler/GrpcServicePrinter.scala b/compiler-plugin/src/main/scala/scalapb/compiler/GrpcServicePrinter.scala
@@ -178,26 +178,33 @@ final class GrpcServicePrinter(service: ServiceDescriptor, implicits: Descriptor
       case StreamType.Bidirectional   => "BIDI_STREAMING"
     }
 
-    val grpcMethodDescriptor = "_root_.io.grpc.MethodDescriptor"
-
     val idempotencyLevel = method.getOptions.getIdempotencyLevel
     val safe             = idempotencyLevel == IdempotencyLevel.NO_SIDE_EFFECTS
-    val idempotent       = idempotencyLevel == IdempotencyLevel.IDEMPOTENT
+    val idempotent       = idempotencyLevel == IdempotencyLevel.IDEMPOTENT || safe
+
+    val grpcMethodDescriptor = "_root_.io.grpc.MethodDescriptor"
 
     p.add(
       s"""${method.deprecatedAnnotation}val ${method.grpcDescriptor.nameSymbol}: $grpcMethodDescriptor[${method.inputType.scalaType}, ${method.outputType.scalaType}] =
          |  $grpcMethodDescriptor.newBuilder()
          |    .setType($grpcMethodDescriptor.MethodType.$methodType)
          |    .setFullMethodName($grpcMethodDescriptor.generateFullMethodName("${service.getFullName}", "${method.getName}"))
          |    .setSampledToLocalTracing(true)
-         |    .setSafe($safe)
-         |    .setIdempotent($idempotent)
          |    .setRequestMarshaller(${marshaller(method.inputType)})
          |    .setResponseMarshaller(${marshaller(method.outputType)})
          |    .setSchemaDescriptor(_root_.scalapb.grpc.ConcreteProtoMethodDescriptorSupplier.fromMethodDescriptor(${method.javaDescriptorSource}))
-         |    .build()
-         |""".stripMargin
-    )
+         |""".stripMargin.stripSuffix("\n")
+    ).indented(
+      _.indented(
+        _.when(safe) {
+          _.add(".setSafe(true)")
+        }
+          .when(idempotent) {
+            _.add(".setIdempotent(true)")
+          }
+          .add(".build()")
+      )
+    ).newline
   }
 
   private[this] def serviceDescriptor(service: ServiceDescriptor) = {

diff --git a/e2e-grpc/src/main/protobuf/service.proto b/e2e-grpc/src/main/protobuf/service.proto
@@ -97,6 +97,10 @@ service Service1 {
   rpc PrimitiveValues(google.protobuf.Int32Value) returns (google.protobuf.StringValue) {};
 
   rpc EchoRequest(Req1) returns (Res6) {}
+
+  rpc NoSideEffects(Req1) returns (Res6) {
+    option idempotency_level = NO_SIDE_EFFECTS;
+  }
 }
 
 service Issue774 {

diff --git a/e2e-grpc/src/main/scala/com/thesamet/pb/Service1ScalaImpl.scala b/e2e-grpc/src/main/scala/com/thesamet/pb/Service1ScalaImpl.scala
@@ -126,4 +126,8 @@ class Service1ScalaImpl extends Service1 {
   }
 
   override def primitiveValues(request: Int): Future[String] = Future.successful("boo")
+
+  override def noSideEffects(request: Req1): Future[Res6] =
+    Future.successful(Res6(Some(request)))
+
 }
diff --git a/e2e-grpc/src/test/scala/MethodDescriptorSpec.scala b/e2e-grpc/src/test/scala/MethodDescriptorSpec.scala
@@ -7,9 +7,17 @@ import org.scalatest.{LoneElement, OptionValues}
 class MethodDescriptorSpec extends AnyFlatSpec with Matchers with LoneElement with OptionValues {
 
   "scala descriptor" must "expose correct input and output message descriptors" in {
-    val unaryMethod = Service1Grpc.Service1.scalaDescriptor.methods.find(_.name == "CustomUnary").get
+    val unaryMethod =
+      Service1Grpc.Service1.scalaDescriptor.methods.find(_.name == "CustomUnary").get
 
     unaryMethod.inputType must be theSameInstanceAs XYMessage.scalaDescriptor
     unaryMethod.outputType must be theSameInstanceAs Res5.scalaDescriptor
   }
+
+  "java descriptor" must "set idempotent and safe options when idempotency_level is set" in {
+    val noSideEffMethod = Service1Grpc.METHOD_NO_SIDE_EFFECTS
+
+    noSideEffMethod.isSafe must be(true)
+    noSideEffMethod.isIdempotent must be(true)
+  }
 }