From 6bc2a2470f6ebce680924c0b3ac2d07ee111e40d Mon Sep 17 00:00:00 2001 From: Helen Woldesenbet Date: Tue, 14 Dec 2021 02:08:48 +0000 Subject: [PATCH] scrooge-generator: Java Api to Validate requests when servers receive them Problem For services defined in the IDL, we want to validate all input parameters for all methods defined in the service. If the parameter is of type struct, union, or exception type, we want to leverage the validateInstanceValue API. After we validated all parameters, we will throw a ThriftValidationException for all validation violations for the Java version Solution Modified Service.mustache template for Java generator to throw the exception in ServiceIface definitions JIRA Issues: CSL-11200 Differential Revision: https://phabricator.twitter.biz/D792676 --- .../ThriftValidationException.scala | 21 ++++++- .../test/gold/thriftjava/GoldService.java | 14 +++++ .../test/gold/thriftjava/PlatinumService.java | 7 +++ .../ValidationsJavaGeneratorSpec.scala | 62 +++++++++++++++++++ .../resources/apachejavagen/service.mustache | 13 ++++ .../java_generator/FieldController.scala | 6 +- .../java_generator/FunctionController.scala | 1 - 7 files changed, 121 insertions(+), 3 deletions(-) diff --git a/scrooge-core/src/main/scala/com/twitter/scrooge/thrift_validation/ThriftValidationException.scala b/scrooge-core/src/main/scala/com/twitter/scrooge/thrift_validation/ThriftValidationException.scala index b7a930a65..a82f64075 100644 --- a/scrooge-core/src/main/scala/com/twitter/scrooge/thrift_validation/ThriftValidationException.scala +++ b/scrooge-core/src/main/scala/com/twitter/scrooge/thrift_validation/ThriftValidationException.scala @@ -1,4 +1,23 @@ package com.twitter.scrooge.thrift_validation +import scala.collection.JavaConverters._ + +object ThriftValidationException { + + /** + * A java compatible exception which is used to communicate when a thrift_validation + * has failed with the respective [[ThriftValidationViolation]] + * + * @param endpoint thrift method the invalid request tries to reach + * @param requestClazz the type of request that was passed in + * @param validationViolations all violations collected while deserializing the thrift object + */ + def create( + endpoint: String, + requestClazz: Class[_], + validationViolations: java.util.Set[ThriftValidationViolation] + ): ThriftValidationException = + ThriftValidationException(endpoint, requestClazz, validationViolations.asScala.toSet) +} /** * An exception which is used to communicate when a thrift_validation @@ -12,7 +31,7 @@ final case class ThriftValidationException( endpoint: String, requestClazz: Class[_], validationViolations: Set[ThriftValidationViolation]) - extends Exception { + extends RuntimeException { override def getMessage: String = s" The validation for request ${requestClazz.getName} to endpoint $endpoint failed with messages: ${validationViolations diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/GoldService.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/GoldService.java index a51c8579d..30ff74465 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/GoldService.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/GoldService.java @@ -608,6 +608,13 @@ public Future apply(Throwable t) { private final com.twitter.finagle.Service methodService = new com.twitter.finagle.Service() { @Override public Future apply(doGreatThings_args args) { + try { + Set requestViolations = Request.validateInstanceValue(args.request); + if (!requestViolations.isEmpty()) { + throw com.twitter.scrooge.thrift_validation.ThriftValidationException.create("doGreatThings", args.request.getClass(), requestViolations); + } + } catch(NullPointerException e) { + } Future future = iface.doGreatThings(args.request); return future; } @@ -697,6 +704,13 @@ public Future apply(Throwable t) { private final com.twitter.finagle.Service methodService = new com.twitter.finagle.Service() { @Override public Future apply(noExceptionCall_args args) { + try { + Set requestViolations = Request.validateInstanceValue(args.request); + if (!requestViolations.isEmpty()) { + throw com.twitter.scrooge.thrift_validation.ThriftValidationException.create("noExceptionCall", args.request.getClass(), requestViolations); + } + } catch(NullPointerException e) { + } Future future = iface.noExceptionCall(args.request); return future; } diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/PlatinumService.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/PlatinumService.java index 6de3d6e46..fa878ca25 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/PlatinumService.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/PlatinumService.java @@ -461,6 +461,13 @@ else if (t instanceof OverCapacityException) { private final com.twitter.finagle.Service methodService = new com.twitter.finagle.Service() { @Override public Future apply(moreCoolThings_args args) { + try { + Set requestViolations = Request.validateInstanceValue(args.request); + if (!requestViolations.isEmpty()) { + throw com.twitter.scrooge.thrift_validation.ThriftValidationException.create("moreCoolThings", args.request.getClass(), requestViolations); + } + } catch(NullPointerException e) { + } Future future = iface.moreCoolThings(args.request); return future; } diff --git a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ValidationsJavaGeneratorSpec.scala b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ValidationsJavaGeneratorSpec.scala index f853e0f2e..0c09e948f 100644 --- a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ValidationsJavaGeneratorSpec.scala +++ b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ValidationsJavaGeneratorSpec.scala @@ -1,11 +1,39 @@ package com.twitter.scrooge.java_generator import apache_java_thrift._ +import com.twitter.conversions.DurationOps.richDurationFromInt +import com.twitter.finagle.Address +import com.twitter.finagle.Name +import com.twitter.finagle.ThriftMux import com.twitter.scrooge.testutil.Spec import com.twitter.scrooge.thrift_validation.ThriftValidationViolation +import com.twitter.util.Await +import com.twitter.util.Awaitable +import com.twitter.util.Duration +import com.twitter.util.Future +import java.lang +import java.net.InetSocketAddress +import org.apache.thrift.TApplicationException import scala.jdk.CollectionConverters._ class ValidationsJavaGeneratorSpec extends Spec { + def await[T](a: Awaitable[T], d: Duration = 5.seconds): T = + Await.result(a, d) + + private class ValidationServiceImpl extends ValidationService.ServiceIface { + override def validate( + structRequest: ValidationStruct, + unionRequest: ValidationUnion, + exceptionRequest: ValidationException + ): Future[lang.Boolean] = Future.value(true) + + override def validateOption( + structRequest: ValidationStruct, + unionRequest: ValidationUnion, + exceptionRequest: ValidationException + ): Future[lang.Boolean] = Future.value(true) + } + "Java validateInstanceValue" should { "validate Struct" in { val validationStruct = @@ -87,6 +115,40 @@ class ValidationsJavaGeneratorSpec extends Spec { val validationViolations = NonValidationStruct.validateInstanceValue(nonValidationStruct) assertViolations(validationViolations.asScala.toSet, 0, Set.empty) } + + "validate struct, union and exception request" in { + val validationStruct = new ValidationStruct( + "email", + -1, + 101, + 0, + 0, + Map("1" -> "1", "2" -> "2").asJava, + false, + "anything") + val impl = new ValidationServiceImpl() + val validationIntUnion = new ValidationUnion() + validationIntUnion.setIntField(-1) + val validationException = new ValidationException("") + val muxServer = ThriftMux.server.serveIface("localhost:*", impl) + val muxClient = ThriftMux.client.build[ValidationService.ServiceIface]( + Name.bound(Address(muxServer.boundAddress.asInstanceOf[InetSocketAddress])), + "a_client") + intercept[TApplicationException] { + await(muxClient.validate(validationStruct, validationIntUnion, validationException)) + } + } + + "validate null request" in { + val impl = new ValidationServiceImpl() + val muxServer = ThriftMux.server.serveIface("localhost:*", impl) + val muxClient = ThriftMux.client.build[ValidationService.ServiceIface]( + Name.bound(Address(muxServer.boundAddress.asInstanceOf[InetSocketAddress])), + "a_client") + //null values passed after code generation aren't checked so + // we catch NullPointerException in the mustache file + await(muxClient.validate(null, null, null)) + } } private def assertViolations( diff --git a/scrooge-generator/src/main/resources/apachejavagen/service.mustache b/scrooge-generator/src/main/resources/apachejavagen/service.mustache index e0bd9e893..1b444847e 100644 --- a/scrooge-generator/src/main/resources/apachejavagen/service.mustache +++ b/scrooge-generator/src/main/resources/apachejavagen/service.mustache @@ -610,6 +610,19 @@ public class {{name}} { private final com.twitter.finagle.Service<{{name}}_args, {{{return_type.type_name_in_container}}}> methodService = new com.twitter.finagle.Service<{{name}}_args, {{{return_type.type_name_in_container}}}>() { @Override public Future<{{{return_type.type_name_in_container}}}> apply({{name}}_args args) { +{{#has_args}} +{{#fields}} +{{#field_type.is_struct}} + try { + Set {{field_name}}Violations = {{arg_type}}.validateInstanceValue({{field_arg}}); + if (!{{field_name}}Violations.isEmpty()) { + throw com.twitter.scrooge.thrift_validation.ThriftValidationException.create("{{name}}", {{field_arg}}.getClass(), {{field_name}}Violations); + } + } catch(NullPointerException e) { + } +{{/field_type.is_struct}} +{{/fields}} +{{/has_args}} Future<{{{return_type.type_name_in_container}}}> future = iface.{{name}}({{{argument_list_with_args}}}); return future; } diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FieldController.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FieldController.scala index 5c44b6cb8..49b04b0e4 100644 --- a/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FieldController.scala +++ b/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FieldController.scala @@ -1,6 +1,8 @@ package com.twitter.scrooge.java_generator -import com.twitter.scrooge.ast.{Field, Identifier, Requiredness} +import com.twitter.scrooge.ast.Field +import com.twitter.scrooge.ast.Identifier +import com.twitter.scrooge.ast.Requiredness import com.twitter.scrooge.backend.Generator import com.google.common.base @@ -15,6 +17,8 @@ class FieldController(f: Field, generator: ApacheJavaGenerator, ns: Option[Ident val has_annotations: Boolean = f.fieldAnnotations.nonEmpty val field_type: FieldTypeController = new FieldTypeController(f.fieldType, generator) + val field_arg: String = "args." + f.sid.name + val arg_type: String = generator.typeName(f.fieldType) def getRequirement(field: Field): String = { field.requiredness match { diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FunctionController.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FunctionController.scala index 435dc76b9..386f2c4bf 100644 --- a/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FunctionController.scala +++ b/scrooge-generator/src/main/scala/com/twitter/scrooge/java_generator/FunctionController.scala @@ -26,7 +26,6 @@ class FunctionController( val argument_list_with_args: String = function.args map { a => "args." + a.sid.name } mkString ", " - val has_args: Boolean = function.args.size > 0 val fields: Seq[FieldController] = function.args map { a => new FieldController(a, generator, ns)