Permalink
Browse files

[split] scrooge: capture unknown union values as its own value (THRIF…

…T-99)

Motivation:
To allow for safely adding new entries in unions. Currently, clients are not forced to deal with unknown types which makes adding and returning new types impossible safely.

Modifications:
Instead of throwing an exception at deserialize time, set the result to an Unknown value. Generated code with pass-through properly maintains pass-through behavior for the unknown union value.

Result:
Clients will now be able to deserialize unions with unknown values, and clients will be able to deal with the unknown value in appropriate way for their use case.

RB_ID=289649
  • Loading branch information...
1 parent 3b3d29d commit 8a67b9160e7dad5132d5b3d2aa3c268b3e435339 Arya Asemanfar committed with CI Feb 18, 2014
View
@@ -5,8 +5,8 @@ import com.typesafe.sbt.site.SphinxSupport.Sphinx
object Scrooge extends Build {
val libVersion = "3.12.2"
- val utilVersion = "6.12.0"
- val finagleVersion = "6.12.0"
+ val utilVersion = "6.11.1"
+ val finagleVersion = "6.11.1"
def util(which: String) = "com.twitter" %% ("util-"+which) % utilVersion
def finagle(which: String) = "com.twitter" %% ("finagle-"+which) % finagleVersion
@@ -29,7 +29,8 @@
public enum Field {
{{#fields}}
{{FIELD_NAME}}{{/fields|,
-}}
+}},
+UNKNOWN_UNION_VALUE;
}
@@ -53,6 +54,9 @@
{{/readWriteInfo}}
{{/fields}}
default:
+ if (_field.type != TType.STOP) {
+ result = new {{StructName}}();
+ }
TProtocolUtil.skip(_iprot, _field.type);
}
if (_field.type != TType.STOP) {
@@ -93,6 +97,13 @@ public static void encode({{StructName}} struct, TProtocol oprot) throws org.apa
CODEC.encode(struct, oprot);
}
+ private {{StructName}}() {
+ this.setField = Field.UNKNOWN_UNION_VALUE;
+ {{#fields}}
+ this.{{fieldName}} = null;
+ {{/fields}}
+ }
+
public {{StructName}}(
Field setField,
Object value
@@ -9,7 +9,6 @@ _field.`type` match {
{{#required}}
{{gotName}} = true
{{/required}}
- _readField = true
}
case _ => // skip
}
@@ -90,7 +90,6 @@ object {{StructName}} extends ThriftStructCodec3[{{StructName}}] {
if (_field.`type` == TType.STOP) {
_done = true
} else {
- var _readField = false
_field.id match {
{{#fields}}
case {{id}} =>
@@ -100,10 +99,6 @@ object {{StructName}} extends ThriftStructCodec3[{{StructName}}] {
if (_passthroughFields == null)
_passthroughFields = immutable$Map.newBuilder[Short, TFieldBlob]
_passthroughFields += (_field.id -> TFieldBlob.read(_field, _iprot))
- _readField = true
- }
- if (!_readField) {
- TProtocolUtil.skip(_iprot, _field.`type`)
}
_iprot.readFieldEnd()
}
@@ -1,7 +1,7 @@
{{#public}}
package {{package}}
-import com.twitter.scrooge.{ThriftStruct, ThriftStructCodec3}
+import com.twitter.scrooge.{ThriftStruct, ThriftStructCodec3, TFieldBlob}
import org.apache.thrift.protocol._
import java.nio.ByteBuffer
import java.util.Arrays
@@ -36,7 +36,12 @@ object {{StructName}} extends ThriftStructCodec3[{{StructName}}] {
{{>readUnionField}}
{{/readWriteInfo}}
{{/fields}}
- case _ => TProtocolUtil.skip(_iprot, _field.`type`)
+ case _ =>
+ if (_field.`type` != TType.STOP) {
+ _result = UnknownUnionField(TFieldBlob.read(_field, _iprot))
+ } else {
+ TProtocolUtil.skip(_iprot, _field.`type`)
+ }
}
if (_field.`type` != TType.STOP) {
_iprot.readFieldEnd()
@@ -79,4 +84,13 @@ object {{StructName}} extends ThriftStructCodec3[{{StructName}}] {
}
}
{{/fields}}
+
+ case class UnknownUnionField private[{{StructName}}](private val field: TFieldBlob) extends {{StructName}} {
+ override def write(_oprot: TProtocol) {
+ _oprot.writeStructBegin(Union)
+ field.write(_oprot)
+ _oprot.writeFieldStop()
+ _oprot.writeStructEnd()
+ }
+ }
}
@@ -14,6 +14,10 @@ class DuplicateFieldIdException(name: String)
class RepeatingEnumValueException(name: String, value: Int)
extends ParseException("Repeating enum value in " + name + ": " + value)
+class UnionFieldInvalidNameException(union: String, field: String)
+ extends ParseException("Field " + field + " in union " + union + " is prohibited")
+
+
// warnings (non-severe errors). If the strict mode is on, Scrooge will throw these exceptions;
// otherwise it merely prints warnings.
class ParseWarning(reason: String, cause: Throwable)
@@ -295,10 +295,15 @@ class ThriftParser(
Struct(sid, sid.name, fixFieldIds(fields), comment, annotations)
}
+ private[this] val disallowedUnionFieldNames = Set("unknown_union_field", "unknownunionfield") map { _.toLowerCase }
+
lazy val union = structLike("union") ^^ {
case comment ~ sid ~ fields ~ annotations =>
val fields0 = fields.map {
- case f @ Field(_, _, _, _, _, r, _, _) if r == Requiredness.Default => f
+ case f @ Field(_, _, _, _, _, r, _, _) if r == Requiredness.Default =>
+ if (disallowedUnionFieldNames.contains(f.sid.name.toLowerCase)) {
+ throw new UnionFieldInvalidNameException(sid.name, f.sid.name)
+ } else f
case f @ _ =>
failOrWarn(UnionFieldRequirednessException(sid.name, f.sid.name, f.requiredness.toString))
f.copy(requiredness = Requiredness.Default)
@@ -574,6 +574,23 @@ class JavaGeneratorSpec extends JMockSpec with EvalHelper {
// no write test because it's not possible
}
+ "unknown field" should {
+ "read as unknown" in { _ =>
+ val prot = new TBinaryProtocol(new TMemoryBuffer(64))
+ val unionField = new NewUnionField(
+ 14653230,
+ new SomeInnerUnionStruct(26, "a_a")
+ )
+ val newUnion = UnionPostEvolution.newNewField(unionField)
+ UnionPostEvolution.encode(newUnion, prot)
+ val decoded = UnionPreEvolution.decode(prot)
+
+ // work around weird error when trying to reference java enums from scala.
+ // java.lang.AssertionError: thrift/java_test/UnionPreEvolution$AnotherName already declared as ch.epfl.lamp.fjbg.JInnerClassesAttribute$Entry@3ac8b10
+ decoded.setField.toString must be("UNKNOWN_UNION_VALUE")
+ }
+ }
+
"nested struct" should {
"read" in { cycle => import cycle._
val protocol = mock[TProtocol]
@@ -141,6 +141,23 @@ class ScalaGeneratorSpec extends JMockSpec with EvalHelper {
EnumUnion.decode(prot) must be(eUnion)
}
+ "encode-decode union with unknown field" in { _ =>
+ val prot = new TBinaryProtocol(new TMemoryBuffer(64))
+ val unionField = NewUnionField(
+ 14653230,
+ SomeInnerUnionStruct(26, "a_a")
+ )
+ val newUnion = UnionPostEvolution.NewField(unionField)
+ UnionPostEvolution.encode(newUnion, prot)
+ val decoded = UnionPreEvolution.decode(prot)
+ decoded.isInstanceOf[UnionPreEvolution.UnknownUnionField] must be(true)
+
+ val oldProt = new TBinaryProtocol(new TMemoryBuffer(64))
+ UnionPreEvolution.encode(decoded, oldProt)
+ val decodedNew = UnionPostEvolution.decode(oldProt)
+ decodedNew must be(UnionPostEvolution.NewField(unionField))
+ }
+
"be identified as an ENUM" in { _ =>
EnumStruct.NumberField.`type` must be(TType.ENUM)
}
@@ -214,6 +214,18 @@ enum Foo
Field(3, SimpleID("g"), "g", ReferenceType(Identifier("Glider")), None, Requiredness.Default)
), None, Map.empty))
}
+
+ "invalid field name" in {
+ intercept[UnionFieldInvalidNameException] {
+ parser.parse("""
+ union Fruit {
+ 1: Apple apple
+ 2: Banana banana
+ 3: UnknownFruit unknown_union_field
+ }
+ """, parser.definition)
+ }
+ }
}
"exception" in {
@@ -0,0 +1,24 @@
+namespace java thrift.test
+
+struct OldUnionField {
+ 1: i64 oldValue
+}
+
+struct SomeInnerUnionStruct {
+ 1: i32 anInt
+ 2: string aString
+}
+
+struct NewUnionField {
+ 1: i32 newValue
+ 2: SomeInnerUnionStruct innerStruct
+}
+
+union UnionPreEvolution {
+ 1: OldUnionField oldField
+}
+
+union UnionPostEvolution {
+ 1: OldUnionField oldField
+ 2: NewUnionField newField
+}

0 comments on commit 8a67b91

Please sign in to comment.