From 2d2f5aaa31b03fe5f463fec68c2dbe8b7a4a54c5 Mon Sep 17 00:00:00 2001
From: Ihor Vovk <adwolf15@gmail.com>
Date: Fri, 29 Nov 2024 08:56:44 +0100
Subject: [PATCH] Imply idempotent when safe is set. Emit options only when
 idempotency option is present. Add tests

---
 .../scalapb/compiler/GrpcServicePrinter.scala | 23 ++++++++++++-------
 e2e-grpc/src/main/protobuf/service.proto      |  4 ++++
 .../com/thesamet/pb/Service1ScalaImpl.scala   |  4 ++++
 .../src/test/scala/MethodDescriptorSpec.scala | 10 +++++++-
 4 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/compiler-plugin/src/main/scala/scalapb/compiler/GrpcServicePrinter.scala b/compiler-plugin/src/main/scala/scalapb/compiler/GrpcServicePrinter.scala
index 09df4d06c..d36ef4f14 100644
--- a/compiler-plugin/src/main/scala/scalapb/compiler/GrpcServicePrinter.scala
+++ b/compiler-plugin/src/main/scala/scalapb/compiler/GrpcServicePrinter.scala
@@ -178,11 +178,11 @@ 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}] =
@@ -190,14 +190,21 @@ final class GrpcServicePrinter(service: ServiceDescriptor, implicits: Descriptor
          |    .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
index 8dc3f72c4..1c0dbf135 100644
--- 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
index ebacfcdc5..0044aceed 100644
--- 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
index 376e16b7f..29ca9bd2b 100644
--- 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)
+  }
 }