From 39870f0f2187a209fe58f039ef22f647cba77c33 Mon Sep 17 00:00:00 2001 From: Dorothy Ordogh Date: Wed, 18 Nov 2020 23:03:53 +0000 Subject: [PATCH] scrooge: Java throws an exception when field types don't match Problem The scrooge generated scala code throws an exception when reading a struct that has different field types from that of the expected struct. The Java functionality doesn't match this, which creates inconsistencies and confusion. Solution The generated java code should also throw an exception when it is decoding a struct that has fields of different types than the expected struct. Result The generated java and scala code now match in functionality. JIRA Issues: CSL-10274 Differential Revision: https://phabricator.twitter.biz/D565373 --- CHANGELOG.rst | 3 + .../gold/thriftjava/AnotherException.java | 10 +- .../test/gold/thriftjava/CollectionId.java | 10 +- .../test/gold/thriftjava/GoldService.java | 36 ++---- .../thriftjava/OverCapacityException.java | 10 +- .../test/gold/thriftjava/PlatinumService.java | 31 ++--- .../test/gold/thriftjava/Recursive.java | 17 +-- .../scrooge/test/gold/thriftjava/Request.java | 120 ++++++------------ .../test/gold/thriftjava/Response.java | 17 +-- .../test/gold/thriftjava/ResponseUnion.java | 1 + .../scrooge/goldfile/GoldFileTest.scala | 2 +- .../ApacheJavaGeneratorSpec.scala | 24 +++- .../resources/apachejavagen/service.mustache | 1 + .../resources/apachejavagen/struct.mustache | 1 + .../apachejavagen/struct_inner.mustache | 15 +-- 15 files changed, 119 insertions(+), 179 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1eca2fd4a..84e057cef 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,6 +10,9 @@ Unreleased * scrooge: Make options parser a separate class. All fields of `com.twitter.scrooge.Compiler` class are changed to immutable types. ``PHAB_ID=D561738`` +* scrooge-generator: Java throws an exception when encountering incorrect field + types in a struct while deserializing. ``PHAB_ID=D565373`` + 20.10.0 ------- diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/AnotherException.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/AnotherException.java index a21fe8b09..33be227b1 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/AnotherException.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/AnotherException.java @@ -28,6 +28,7 @@ import com.twitter.finagle.JavaFailureFlags; import com.twitter.scrooge.ThriftStructIface; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. @@ -359,12 +360,9 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // ERROR_CODE - if (field.type == TType.I32) { - this.errorCode = iprot.readI32(); - setErrorCodeIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I32, field.type, "errorCode"); + this.errorCode = iprot.readI32(); + setErrorCodeIsSet(true); break; default: if (this.passThroughFields == null) { diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/CollectionId.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/CollectionId.java index a642bf473..67b716723 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/CollectionId.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/CollectionId.java @@ -26,6 +26,7 @@ import com.twitter.scrooge.ThriftStructIface; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. @@ -340,12 +341,9 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // COLLECTION_LONG_ID - if (field.type == TType.I64) { - this.collectionLongId = iprot.readI64(); - setCollectionLongIdIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "collectionLongId"); + this.collectionLongId = iprot.readI64(); + setCollectionLongIdIsSet(true); break; default: if (this.passThroughFields == null) { 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 068735054..ad6724efd 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 @@ -29,6 +29,7 @@ import com.twitter.scrooge.TReusableBuffer; import com.twitter.scrooge.TReusableMemoryTransport; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; import com.twitter.util.ConstFuture; import com.twitter.util.Future; import com.twitter.util.Function; @@ -1106,12 +1107,9 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // REQUEST - if (field.type == TType.STRUCT) { - this.request = new Request(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "request"); + this.request = new Request(); this.request.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { @@ -1550,20 +1548,14 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 0: // SUCCESS - if (field.type == TType.STRUCT) { - this.success = new Response(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "success"); + this.success = new Response(); this.success.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 1: // EX - if (field.type == TType.STRUCT) { - this.ex = new OverCapacityException(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "ex"); + this.ex = new OverCapacityException(); this.ex.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { @@ -1950,12 +1942,9 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // REQUEST - if (field.type == TType.STRUCT) { - this.request = new Request(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "request"); + this.request = new Request(); this.request.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { @@ -2319,12 +2308,9 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 0: // SUCCESS - if (field.type == TType.STRUCT) { - this.success = new Response(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "success"); + this.success = new Response(); this.success.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/OverCapacityException.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/OverCapacityException.java index ce0dae047..d0cda9671 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/OverCapacityException.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/OverCapacityException.java @@ -28,6 +28,7 @@ import com.twitter.finagle.JavaFailureFlags; import com.twitter.scrooge.ThriftStructIface; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. @@ -367,12 +368,9 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // CHILL_TIME_SECONDS - if (field.type == TType.I32) { - this.chillTimeSeconds = iprot.readI32(); - setChillTimeSecondsIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I32, field.type, "chillTimeSeconds"); + this.chillTimeSeconds = iprot.readI32(); + setChillTimeSecondsIsSet(true); break; default: if (this.passThroughFields == null) { 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 646f4d7e4..bdd7f5e3e 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 @@ -29,6 +29,7 @@ import com.twitter.scrooge.TReusableBuffer; import com.twitter.scrooge.TReusableMemoryTransport; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; import com.twitter.util.ConstFuture; import com.twitter.util.Future; import com.twitter.util.Function; @@ -870,12 +871,9 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // REQUEST - if (field.type == TType.STRUCT) { - this.request = new Request(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "request"); + this.request = new Request(); this.request.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { @@ -1387,28 +1385,19 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 0: // SUCCESS - if (field.type == TType.I32) { - this.success = iprot.readI32(); - setSuccessIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I32, field.type, "success"); + this.success = iprot.readI32(); + setSuccessIsSet(true); break; case 1: // AX - if (field.type == TType.STRUCT) { - this.ax = new AnotherException(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "ax"); + this.ax = new AnotherException(); this.ax.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 2: // OCE - if (field.type == TType.STRUCT) { - this.oce = new OverCapacityException(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "oce"); + this.oce = new OverCapacityException(); this.oce.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Recursive.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Recursive.java index 304d238a5..a16e289fb 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Recursive.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Recursive.java @@ -26,6 +26,7 @@ import com.twitter.scrooge.ThriftStructIface; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. @@ -409,20 +410,14 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // ID - if (field.type == TType.I64) { - this.id = iprot.readI64(); - setIdIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "id"); + this.id = iprot.readI64(); + setIdIsSet(true); break; case 2: // REC_REQUEST - if (field.type == TType.STRUCT) { - this.recRequest = new Request(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "recRequest"); + this.recRequest = new Request(); this.recRequest.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Request.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Request.java index 5880135d4..09bbe50a7 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Request.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Request.java @@ -26,6 +26,7 @@ import com.twitter.scrooge.ThriftStructIface; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. @@ -1423,8 +1424,8 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // A_LIST - if (field.type == TType.LIST) { - { + TProtocols.validateFieldType(TType.LIST, field.type, "aList"); + { TList _list0 = iprot.readListBegin(); this.aList = new ArrayList(_list0.size); for (int _i1 = 0; _i1 < _list0.size; ++_i1) @@ -1435,13 +1436,10 @@ public void read(TProtocol iprot) throws TException { } iprot.readListEnd(); } - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 2: // A_SET - if (field.type == TType.SET) { - { + TProtocols.validateFieldType(TType.SET, field.type, "aSet"); + { TSet _set3 = iprot.readSetBegin(); this.aSet = new HashSet(2*_set3.size); for (int _i4 = 0; _i4 < _set3.size; ++_i4) @@ -1452,13 +1450,10 @@ public void read(TProtocol iprot) throws TException { } iprot.readSetEnd(); } - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 3: // A_MAP - if (field.type == TType.MAP) { - { + TProtocols.validateFieldType(TType.MAP, field.type, "aMap"); + { TMap _map6 = iprot.readMapBegin(); this.aMap = new HashMap(2*_map6.size); for (int _i7 = 0; _i7 < _map6.size; ++_i7) @@ -1471,21 +1466,15 @@ public void read(TProtocol iprot) throws TException { } iprot.readMapEnd(); } - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 4: // A_REQUEST - if (field.type == TType.STRUCT) { - this.aRequest = new Request(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "aRequest"); + this.aRequest = new Request(); this.aRequest.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 5: // SUB_REQUESTS - if (field.type == TType.LIST) { - { + TProtocols.validateFieldType(TType.LIST, field.type, "subRequests"); + { TList _list10 = iprot.readListBegin(); this.subRequests = new ArrayList(_list10.size); for (int _i11 = 0; _i11 < _list10.size; ++_i11) @@ -1497,87 +1486,54 @@ public void read(TProtocol iprot) throws TException { } iprot.readListEnd(); } - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 6: // HAS_DEFAULT - if (field.type == TType.STRING) { - this.hasDefault = iprot.readString(); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.STRING, field.type, "hasDefault"); + this.hasDefault = iprot.readString(); break; case 7: // NO_COMMENT - if (field.type == TType.I64) { - this.noComment = iprot.readI64(); - setNoCommentIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "noComment"); + this.noComment = iprot.readI64(); + setNoCommentIsSet(true); break; case 8: // DOUBLE_SLASH_COMMENT - if (field.type == TType.I64) { - this.doubleSlashComment = iprot.readI64(); - setDoubleSlashCommentIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "doubleSlashComment"); + this.doubleSlashComment = iprot.readI64(); + setDoubleSlashCommentIsSet(true); break; case 9: // HASHTAG_COMMENT - if (field.type == TType.I64) { - this.hashtagComment = iprot.readI64(); - setHashtagCommentIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "hashtagComment"); + this.hashtagComment = iprot.readI64(); + setHashtagCommentIsSet(true); break; case 10: // SINGLE_ASTERISK_COMMENT - if (field.type == TType.I64) { - this.singleAsteriskComment = iprot.readI64(); - setSingleAsteriskCommentIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "singleAsteriskComment"); + this.singleAsteriskComment = iprot.readI64(); + setSingleAsteriskCommentIsSet(true); break; case 11: // DOC_STRING_COMMENT - if (field.type == TType.I64) { - this.docStringComment = iprot.readI64(); - setDocStringCommentIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "docStringComment"); + this.docStringComment = iprot.readI64(); + setDocStringCommentIsSet(true); break; case 12: // REC_REQUEST - if (field.type == TType.STRUCT) { - this.recRequest = new Recursive(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "recRequest"); + this.recRequest = new Recursive(); this.recRequest.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; case 13: // REQUIRED_FIELD - if (field.type == TType.STRING) { - this.requiredField = iprot.readString(); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.STRING, field.type, "requiredField"); + this.requiredField = iprot.readString(); break; case 14: // CONSTRUCTION_REQUIRED_FIELD - if (field.type == TType.I64) { - this.constructionRequiredField = iprot.readI64(); - setConstructionRequiredFieldIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I64, field.type, "constructionRequiredField"); + this.constructionRequiredField = iprot.readI64(); + setConstructionRequiredFieldIsSet(true); break; case 15: // AN_INT8 - if (field.type == TType.BYTE) { - this.anInt8 = iprot.readByte(); - setAnInt8IsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.BYTE, field.type, "anInt8"); + this.anInt8 = iprot.readByte(); + setAnInt8IsSet(true); break; default: if (this.passThroughFields == null) { diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Response.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Response.java index c92afb850..9e7010fc2 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Response.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Response.java @@ -26,6 +26,7 @@ import com.twitter.scrooge.ThriftStructIface; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. @@ -413,20 +414,14 @@ public void read(TProtocol iprot) throws TException { } switch (field.id) { case 1: // STATUS_CODE - if (field.type == TType.I32) { - this.statusCode = iprot.readI32(); - setStatusCodeIsSet(true); - } else { - TProtocolUtil.skip(iprot, field.type); - } + TProtocols.validateFieldType(TType.I32, field.type, "statusCode"); + this.statusCode = iprot.readI32(); + setStatusCodeIsSet(true); break; case 2: // RESPONSE_UNION - if (field.type == TType.STRUCT) { - this.responseUnion = new ResponseUnion(); + TProtocols.validateFieldType(TType.STRUCT, field.type, "responseUnion"); + this.responseUnion = new ResponseUnion(); this.responseUnion.read(iprot); - } else { - TProtocolUtil.skip(iprot, field.type); - } break; default: if (this.passThroughFields == null) { diff --git a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/ResponseUnion.java b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/ResponseUnion.java index 45899879f..e7459b067 100644 --- a/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/ResponseUnion.java +++ b/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/ResponseUnion.java @@ -26,6 +26,7 @@ import com.twitter.scrooge.ThriftStructIface; import com.twitter.scrooge.TFieldBlob; +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. diff --git a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/goldfile/GoldFileTest.scala b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/goldfile/GoldFileTest.scala index 7431a703a..1f294caa4 100644 --- a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/goldfile/GoldFileTest.scala +++ b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/goldfile/GoldFileTest.scala @@ -2,7 +2,7 @@ package com.twitter.scrooge.goldfile import com.twitter.io.Files import com.twitter.scrooge.Main -import com.twitter.scrooge.testutil.{Utils, TempDirectory} +import com.twitter.scrooge.testutil.{TempDirectory, Utils} import java.io.{ByteArrayInputStream, File, InputStream} import org.scalatest.BeforeAndAfterAll import org.scalatest.funsuite.AnyFunSuite diff --git a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ApacheJavaGeneratorSpec.scala b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ApacheJavaGeneratorSpec.scala index 0b6b4cc5a..40b97a9ad 100644 --- a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ApacheJavaGeneratorSpec.scala +++ b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/java_generator/ApacheJavaGeneratorSpec.scala @@ -8,7 +8,7 @@ import com.twitter.scrooge.testutil.Spec import com.twitter.scrooge.testutil.Utils.getFileContents import com.twitter.util.Try import java.util.{EnumSet, List => JList, Map => JMap, Set => JSet} -import org.apache.thrift.protocol.{TBinaryProtocol, TProtocolException} +import org.apache.thrift.protocol.{TBinaryProtocol, TField, TProtocolException, TStruct, TType} import org.apache.thrift.transport.TMemoryBuffer import scala.collection.JavaConverters._ import scala.collection.concurrent.TrieMap @@ -344,4 +344,26 @@ class ApacheJavaGeneratorSpec extends Spec { "test_enum.OrganizationType.findByValue(iprot.readI32())") } } + + "Structs" should { + "throw an error when the read field type is incorrect" in { + val prot = new TBinaryProtocol(new TMemoryBuffer(10000)) + + prot.writeStructBegin(new TStruct("ConstructorRequiredStruct")) + prot.writeFieldBegin(new TField("requiredField", TType.I64, 2)) + prot.writeI64(4000) + prot.writeFieldEnd() + prot.writeFieldStop() + prot.writeStructEnd() + + val ex = intercept[TProtocolException] { + val readStruct = new ConstructorRequiredStruct() + readStruct.read(prot) + } + + ex.toString.contains("requiredField") must be(true) + ex.toString.contains("actual=I64") must be(true) + ex.toString.contains("expected=STRING") must be(true) + } + } } diff --git a/scrooge-generator/src/main/resources/apachejavagen/service.mustache b/scrooge-generator/src/main/resources/apachejavagen/service.mustache index f062bedc1..4e56b2d15 100644 --- a/scrooge-generator/src/main/resources/apachejavagen/service.mustache +++ b/scrooge-generator/src/main/resources/apachejavagen/service.mustache @@ -9,6 +9,7 @@ import com.twitter.scrooge.TReusableMemoryTransport; {{#is_passthrough_service}} import com.twitter.scrooge.TFieldBlob; {{/is_passthrough_service}} +import com.twitter.scrooge.internal.TProtocols; {{^is_oneway_or_void}} import com.twitter.util.ConstFuture; {{/is_oneway_or_void}} diff --git a/scrooge-generator/src/main/resources/apachejavagen/struct.mustache b/scrooge-generator/src/main/resources/apachejavagen/struct.mustache index a3e8c28cc..2cde3da12 100644 --- a/scrooge-generator/src/main/resources/apachejavagen/struct.mustache +++ b/scrooge-generator/src/main/resources/apachejavagen/struct.mustache @@ -10,6 +10,7 @@ import com.twitter.scrooge.ThriftStructIface; {{#is_passthrough_struct}} import com.twitter.scrooge.TFieldBlob; {{/is_passthrough_struct}} +import com.twitter.scrooge.internal.TProtocols; // No additional import required for struct/union. diff --git a/scrooge-generator/src/main/resources/apachejavagen/struct_inner.mustache b/scrooge-generator/src/main/resources/apachejavagen/struct_inner.mustache index caaa38ba0..8e2f4cb40 100644 --- a/scrooge-generator/src/main/resources/apachejavagen/struct_inner.mustache +++ b/scrooge-generator/src/main/resources/apachejavagen/struct_inner.mustache @@ -458,18 +458,15 @@ this.{{name}} != that.{{name}} {{#fields}} case {{key}}: // {{#constant_name}}{{name}}{{/constant_name}} {{#field_type.is_enum}} - if (field.type == {{field_type.to_enum}} || field.type == TType.ENUM) { + TProtocols.validateEnumFieldType(field.type, "{{name}}"); {{/field_type.is_enum}} {{^field_type.is_enum}} - if (field.type == {{field_type.to_enum}}) { + TProtocols.validateFieldType({{field_type.to_enum}}, field.type, "{{name}}"); {{/field_type.is_enum}} - {{#consolidate_newlines}} - {{{deserialize_field}}} - {{>generate_isset_set}} - {{/consolidate_newlines}} - } else { - TProtocolUtil.skip(iprot, field.type); - } + {{#consolidate_newlines}} + {{{deserialize_field}}} + {{>generate_isset_set}} + {{/consolidate_newlines}} break; {{/fields}} default: