Scrooge: support validations on nested fields that are struct, union,…

… and exception

Problem

The nested validations was not working as expected. The expected behavior
when passing `invalidNestedRequest` to `methodPerEndPointClient.validateNestedRequest`
is to throw `TApplicationException` but we get nothing instead. This was because we look for
validation annotations on nestedStructs themselves instead of inspecting deeper in the struct.

Solution

Run validation checks recursively so that If nested fields are passed and their fields are defined with validations annotations, we validate the nestedRequest and return the expected violations.

JIRA Issues: CSL-12029

Differential Revision: https://phabricator.twitter.biz/D911262

CHANGELOG.rst

@@ -13,8 +13,12 @@ New Features
 * scrooge-core: `c.t.scrooge.ThriftUnion.fieldInfoForUnionClass` API for retrieving
   `ThriftStructFieldInfo` for a `ThriftUnion` member class without having to instantiate
   it. ``PHAB_ID=D871986``
+
 * scrooge-generator: Add @.generated annotation to Swift generated code ``PHAB_ID=D879262``
 
+* scrooge-generator: support thrift validations on nested fields that are struct, union, and
+  exception ``PHAB_ID=D911262``
+
 22.4.0
 ------
 

scrooge-generator-tests/src/test/scala/com/twitter/scrooge/backend/ValidationsSpec.scala

@@ -90,6 +90,15 @@ class ValidationsSpec extends JMockSpec with OneInstancePerTest {
     override def validateOnlyNonValidatedRequest(
       nonValidationRequest: NoValidationStruct
     ): Future[Boolean] = Future.True
+
+    override def validateNestedRequest(
+      nestedNonRequest: NestedNonValidationStruct
+    ): Future[Boolean] = Future.True
+
+    override def validateDeepNestedRequest(
+      deepNestedRequest: DeepNestedValidationstruct
+    ): Future[Boolean] =
+      Future.True
   }
 
   val clientReceiver = new InMemoryStatsReceiver
@@ -105,6 +114,22 @@ class ValidationsSpec extends JMockSpec with OneInstancePerTest {
       Name.bound(Address(thriftServer.boundAddress.asInstanceOf[InetSocketAddress])),
       "client")
 
+  "validate nestedStruct with annotation in the innerStruct" in { _ =>
+    val nestedNonValidationStruct = NestedNonValidationStruct("whatever", invalidStructRequest)
+    intercept[TApplicationException] {
+      await(methodPerEndpointClient.validateNestedRequest(nestedNonValidationStruct))
+    }
+  }
+
+  "validate deepNestedStruct with annotation in the deepInnerStruct" in { _ =>
+    val nestedNonValidationStruct = NestedNonValidationStruct("whatever", invalidStructRequest)
+    val deepNestedValidationStruct =
+      DeepNestedValidationstruct("whatever", nestedNonValidationStruct)
+    intercept[TApplicationException] {
+      await(methodPerEndpointClient.validateDeepNestedRequest(deepNestedValidationStruct))
+    }
+  }
+
   "validateInstanceValue" should {
     "validate Struct" in { _ =>
       val validationViolations = ValidationStruct.validateInstanceValue(invalidStructRequest)
@@ -252,6 +277,15 @@ class ValidationsSpec extends JMockSpec with OneInstancePerTest {
           validationRequest: ValidationStruct,
           validationRequestViolations: Set[ThriftValidationViolation]
         ): Future[Boolean] = Future.value(validationRequestViolations.nonEmpty)
+
+        override def validateNestedRequest(
+          nestedNonRequest: NestedNonValidationStruct
+        ): Future[Boolean] = Future.False
+
+        override def validateDeepNestedRequest(
+          deepNestedRequest: DeepNestedValidationstruct
+        ): Future[Boolean] =
+          Future.False
       }
 
       val thriftServer =
@@ -330,6 +364,15 @@ class ValidationsSpec extends JMockSpec with OneInstancePerTest {
           override def validateOnlyNonValidatedRequest(
             nonValidationRequest: NoValidationStruct
           ): Future[Boolean] = Future.False
+
+          override def validateNestedRequest(
+            nestedNonRequest: NestedNonValidationStruct
+          ): Future[Boolean] = Future.False
+
+          override def validateDeepNestedRequest(
+            deepNestedRequest: DeepNestedValidationstruct
+          ): Future[Boolean] =
+            Future.False
         }
 
         val thriftServer =

scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ValidationsJavaGeneratorSpec.scala

@@ -46,6 +46,15 @@ class ValidationsJavaGeneratorSpec extends Spec {
     override def validateOnlyValidatedRequest(
       validationRequest: ValidationStruct
     ): Future[lang.Boolean] = Future.value(true)
+
+    override def validateNestedRequest(
+      nestedNonRequest: NestedNonValidationStruct
+    ): Future[lang.Boolean] = Future.value(true)
+
+    override def validateDeepNestedRequest(
+      deepNestedRequest: DeepNestedValidationstruct
+    ): Future[lang.Boolean] =
+      Future.value(true)
   }
 
   "Java validateInstanceValue" should {
@@ -203,6 +212,14 @@ class ValidationsJavaGeneratorSpec extends Spec {
           // should return false if `structRequest` is invalid
           Future.value(structRequestViolations.isEmpty)
         }
+
+        override def validateNestedRequest(
+          nestedNonRequest: NestedNonValidationStruct
+        ): Future[lang.Boolean] = Future.value(true)
+
+        override def validateDeepNestedRequest(
+          deepNestedRequest: DeepNestedValidationstruct
+        ): Future[lang.Boolean] = Future.value(true)
       }
       val validationStruct = new ValidationStruct(
         "email",

scrooge-generator-tests/src/test/thrift/standalone/validations.thrift

@@ -30,6 +30,16 @@ struct NonValidationStruct {
   1: string stringField (structFieldKey = "")
 }
 
+struct NestedNonValidationStruct {
+ 1: string stringField
+ 2: ValidationStruct nestedStruct
+}
+
+struct DeepNestedValidationstruct {
+ 1: string stringField
+ 2: NestedNonValidationStruct deepNestedStruct
+}
+
 struct NestedValidationStruct {
   1: string stringField (validation.email = "")
   2: ValidationStruct nestedStructField
@@ -69,4 +79,10 @@ service ValidationService {
   bool validateOnlyValidatedRequest (
     1: ValidationStruct validationRequest
   )
+  bool validateNestedRequest (
+   1: NestedNonValidationStruct nestedNonRequest
+  )
+  bool validateDeepNestedRequest (
+   1: DeepNestedValidationstruct deepNestedRequest
+  )
 }

scrooge-generator/src/main/scala/com/twitter/scrooge/backend/ServiceTemplate.scala

@@ -444,17 +444,21 @@ trait ServiceTemplate { self: TemplateGenerator =>
 
   // return true if the `Field` is of struct type (struct, union, or exception), and any fields
   // of the `Field` has validation annotation (annotation key starts with "validation.")
-  private[this] def hasValidationAnnotation(field: Field): Boolean =
+  private[this] def hasValidationAnnotation(field: Field): Boolean = {
     field.fieldType match {
       case structType: StructType =>
         val structLike = structType.struct
         structLike match {
           case _: Struct | _: Union | _: Exception_ =>
-            structLike.fields.exists(_.hasValidationAnnotation)
+            structLike.fields.exists { field =>
+              // recursively look for validation annotations for nestedFields
+              field.hasValidationAnnotation || hasValidationAnnotation(field)
+            }
           case _ => false
         }
       case _ => false
     }
+  }
 
   private[this] class NameDeduplicator() {
     private[this] val seenIDs = new mutable.HashSet[String]