scrooge-generator: Make Field class annotation flag always consistent; make sure we don't miss custom validations

Problem:
--------
Scrooge AST `Field.hasValidationAnnotation` can be set independently of
`Field.fieldAnnotations`, creating instances in inconsistent state.
Further, it will miss custom annotations unless we enforce that custom
annotations also must have the `validation.` prefix.

Solution:
---------
Make `Field.hasValidationAnnotation` a computed field that is always
consistent with the value in `Field.fieldAnnotations`. Enforce that custom
annotations must start with the `validation.` prefix.

JIRA Issues: CSL-12123

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

Author: szegedi

CHANGELOG.rst

@@ -7,12 +7,23 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
 Unreleased
 ----------
 
+Breaking API Changes
+~~~~~~~~~~~~~~~~~~~~
+
+* scrooge-generator: `c.t.scrooge.ast.Field.hasValidationAnnotation` field can no longer
+  be set in the constructor, but it is now a property derived from the value in the
+  `Field.fieldAnnotations` field; if any annotations have a name starting with `validation.`
+  it is set to true, otherwise it is false. ``PHAB_ID=D912077``
+
 Runtime Behavior Changes
 ~~~~~~~~~~~~~~~~~~~~~~~~
 
 * scrooge-generator: Move `ServerValidationMixin` trait to be in the companion object of the service, this is to
   avoid ambiguities when calling the trait from inherited services. ``PHAB_ID=D943975``
 
+* scrooge-thrift-validation: custom annotations in `ThriftValidator` must have a
+  name starting with `validation.` ``PHAB_ID=D912077``
+
 22.7.0
 ------
 

scrooge-core/src/main/scala/com/twitter/scrooge/UtilValidator.scala

@@ -24,7 +24,7 @@ object UtilValidator {
  */
 final class UtilValidator extends BaseValidator {
 
-  override def annotations: Set[String] = DefaultAnnotations.metadata.keySet
+  override def annotations: Set[String] = DefaultAnnotations.keys
 
   // validate each field value for default annotations defined on
   // this field.

scrooge-generator-tests/src/test/scala/com/twitter/scrooge/frontend/TypeResolverSpec.scala

@@ -410,7 +410,6 @@ class TypeResolverSpec extends Spec {
                   _,
                   _,
                   _,
-                  _,
                   _
                 ) => // pass
             case _ => fail()
@@ -425,7 +424,6 @@ class TypeResolverSpec extends Spec {
                   _,
                   _,
                   _,
-                  _,
                   _
                 ) => // pass
             case _ => fail()

scrooge-generator/src/main/scala/com/twitter/scrooge/AST/Node.scala

@@ -15,6 +15,7 @@
  */
 package com.twitter.scrooge.ast
 
+import com.twitter.scrooge.thrift_validation.ThriftValidator
 import scala.util.parsing.input.Positional
 
 sealed abstract class Node extends Positional
@@ -66,9 +67,12 @@ case class Field(
   requiredness: Requiredness = Requiredness.Default,
   typeAnnotations: Map[String, String] = Map.empty,
   fieldAnnotations: Map[String, String] = Map.empty,
-  docstring: Option[String] = None,
-  hasValidationAnnotation: Boolean = false)
-    extends Node
+  docstring: Option[String] = None)
+    extends Node {
+
+  val hasValidationAnnotation =
+    fieldAnnotations.keysIterator.exists(ThriftValidator.isValidationAnnotationName)
+}
 
 case class Function(
   funcName: SimpleID,

scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftParser.scala

@@ -237,8 +237,7 @@ class ThriftParser(
           transformedReq,
           typeAnnotations,
           fieldAnnotations,
-          comm,
-          fieldAnnotations.keySet.exists(_.startsWith("validation."))
+          comm
         )
     }
   )

scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FunctionController.scala

@@ -85,8 +85,7 @@ class FunctionController(
         fieldType = a.fieldType,
         default = a.default,
         requiredness = requiredness,
-        fieldAnnotations = a.fieldAnnotations,
-        hasValidationAnnotation = a.fieldAnnotations.keySet.exists(_.startsWith("validation."))
+        fieldAnnotations = a.fieldAnnotations
       )
     }
     val structName = function.funcName.name + "_args"

scrooge-thrift-validation/src/main/scala/com/twitter/scrooge/thrift_validation/DefaultAnnotations.scala

@@ -132,4 +132,6 @@ object DefaultAnnotations {
     ),
     "validation.UUID" -> AnnotationMetaData(None, Set(classOf[String]), classOf[UUID])
   )
+
+  private[twitter] val keys = metadata.keySet
 }

scrooge-thrift-validation/src/main/scala/com/twitter/scrooge/thrift_validation/ThriftValidator.scala

@@ -1,30 +1,15 @@
 package com.twitter.scrooge.thrift_validation
 
 import com.twitter.scrooge.thrift_validation.ThriftValidator.DefaultAnnotationKeys
+import com.twitter.scrooge.thrift_validation.ThriftValidator.isValidationAnnotationName
 import scala.collection.JavaConverters
 import scala.collection.mutable
 
 object ThriftValidator {
-  val DefaultAnnotationKeys: Set[String] = Set(
-    "validation.assertFalse",
-    "validation.assertTrue",
-    "validation.countryCode",
-    "validation.EAN",
-    "validation.email",
-    "validation.UUID",
-    "validation.ISBN",
-    "validation.length.min",
-    "validation.length.max",
-    "validation.max",
-    "validation.min",
-    "validation.negative",
-    "validation.negativeOrZero",
-    "validation.notEmpty",
-    "validation.positive",
-    "validation.positiveOrZero",
-    "validation.size.min",
-    "validation.size.max"
-  )
+  def isValidationAnnotationName(annName: String): Boolean =
+    annName.startsWith("validation.")
+
+  val DefaultAnnotationKeys: Set[String] = DefaultAnnotations.keys
 }
 
 /**
@@ -33,8 +18,11 @@ object ThriftValidator {
 abstract class ThriftValidator extends BaseValidator {
   // Do not allow to override default annotations since we have a different
   // set of rules for default annotations and they are done before performing
-  // custom validations.
-  require(customAnnotations.keySet.intersect(DefaultAnnotationKeys).isEmpty)
+  // custom validations. All custom validations must have a name that starts
+  // with "validation."
+  require(customAnnotations.keysIterator.forall { name =>
+    isValidationAnnotationName(name) && !DefaultAnnotationKeys.contains(name)
+  })
 
   /**
    * A map of String annotations to [[ThriftConstraintValidator]].