Skip to content

Commit

Permalink
scrooge: Java throws an exception when field types don't match
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@dotordogh
dotordogh authored and jenkins committed Nov 18, 2020
1 parent 35441ac commit 39870f0
Show file tree
Hide file tree
Showing 15 changed files with 119 additions and 179 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
-------

Expand Down
10 changes: 4 additions & 6 deletions ...rces/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/AnotherException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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) {
Expand Down
10 changes: 4 additions & 6 deletions ...esources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/CollectionId.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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) {
Expand Down
36 changes: 11 additions & 25 deletions ...resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/GoldService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
10 changes: 4 additions & 6 deletions ...gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/OverCapacityException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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) {
Expand Down
31 changes: 10 additions & 21 deletions ...urces/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/PlatinumService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
17 changes: 6 additions & 11 deletions ...t/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/Recursive.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit 39870f0

Please sign in to comment.