diff --git a/hphp/runtime/ext/thrift/binary.cpp b/hphp/runtime/ext/thrift/binary.cpp index 62d9d932ab015..00731000d0ac9 100644 --- a/hphp/runtime/ext/thrift/binary.cpp +++ b/hphp/runtime/ext/thrift/binary.cpp @@ -26,7 +26,7 @@ #include "hphp/runtime/ext/reflection/ext_reflection.h" #include "hphp/runtime/ext/std/ext_std_classobj.h" #include "hphp/runtime/ext/thrift/adapter.h" -#include "hphp/runtime/ext/thrift/field_adapter.h" +#include "hphp/runtime/ext/thrift/field_wrapper.h" #include "hphp/runtime/ext/thrift/ext_thrift.h" #include "hphp/runtime/ext/thrift/spec-holder.h" #include "hphp/runtime/ext/thrift/transport.h" @@ -410,7 +410,7 @@ void binary_deserialize_slow(const Object& zthis, if (const auto* fieldspec = getFieldSlow(spec, fieldno)) { if (ttypes_are_compatible(ttype, fieldspec->type)) { Variant rv = binary_deserialize(ttype, transport, *fieldspec, options); - if (fieldspec->field_adapter) { + if (fieldspec->isWrapped) { setThriftType(rv, zthis, StrNR(fieldspec->name)); } else { zthis->o_set(StrNR(fieldspec->name), rv, zthis->getClassName()); @@ -480,7 +480,7 @@ Object binary_deserialize_struct(const String& clsName, } auto index = cls->propSlotToIndex(i); auto value = binary_deserialize(fieldType, transport, fields[i], options); - if (fields[i].field_adapter) { + if (fields[i].isWrapped) { setThriftType(value, dest, StrNR(fields[i].name)); } else { tvSet(*value.asTypedValue(), objProps->at(index)); @@ -615,7 +615,7 @@ void binary_serialize_slow(const FieldSpec& field, INC_TPC(thrift_write_slow); StrNR fieldName(field.name); Variant fieldVal; - if (field.field_adapter) { + if (field.isWrapped) { fieldVal = getThriftType(obj, fieldName); } else { fieldVal = obj->o_get(fieldName, true, obj->getClassName()); @@ -643,7 +643,7 @@ void binary_serialize_struct(const Object& obj, PHPOutputTransport& transport) { auto index = cls->propSlotToIndex(slot); VarNR fieldWrapper(objProps->at(index).tv()); Variant fieldVal; - if (fields[slot].field_adapter) { + if (fields[slot].isWrapped) { fieldVal = getThriftType(obj, StrNR(fields[slot].name)); } else { fieldVal = fieldWrapper; diff --git a/hphp/runtime/ext/thrift/compact.cpp b/hphp/runtime/ext/thrift/compact.cpp index 5ead719a2e61a..7b0192694315f 100644 --- a/hphp/runtime/ext/thrift/compact.cpp +++ b/hphp/runtime/ext/thrift/compact.cpp @@ -28,7 +28,7 @@ #include "hphp/runtime/ext/collections/ext_collections-vector.h" #include "hphp/runtime/ext/reflection/ext_reflection.h" #include "hphp/runtime/ext/thrift/adapter.h" -#include "hphp/runtime/ext/thrift/field_adapter.h" +#include "hphp/runtime/ext/thrift/field_wrapper.h" #include "hphp/runtime/ext/thrift/spec-holder.h" #include "hphp/runtime/ext/thrift/transport.h" #include "hphp/runtime/ext/thrift/util.h" @@ -246,7 +246,7 @@ struct CompactWriter { INC_TPC(thrift_write_slow); StrNR fieldName(field.name); Variant fieldVal; - if (field.field_adapter) { + if (field.isWrapped) { fieldVal = getThriftType(obj, fieldName); } else { fieldVal = obj->o_get(fieldName, true, obj->getClassName()); @@ -292,7 +292,7 @@ struct CompactWriter { fieldInfo.prop = &prop[slot]; fieldInfo.fieldNum = fields[slot].fieldNum; - if (fields[slot].field_adapter) { + if (fields[slot].isWrapped) { auto value = getThriftType(obj, StrNR(fields[slot].name)); writeField(value, fields[slot], fieldType, fieldInfo); } else { @@ -666,7 +666,7 @@ struct CompactReader { if (typesAreCompatible(fieldType, fieldSpec->type)) { readComplete = true; Variant fieldValue = readField(*fieldSpec, fieldType); - if (fieldSpec->field_adapter) { + if (fieldSpec->isWrapped) { setThriftType(fieldValue, dest, StrNR(fieldSpec->name)); } else { dest->o_set( @@ -732,7 +732,7 @@ struct CompactReader { } auto index = cls->propSlotToIndex(slot); auto value = readField(fields[slot], fieldType); - if (fields[slot].field_adapter) { + if (fields[slot].isWrapped) { setThriftType(value, dest, StrNR(fields[slot].name)); } else { tvSet(*value.asTypedValue(), objProp->at(index)); diff --git a/hphp/runtime/ext/thrift/config.cmake b/hphp/runtime/ext/thrift/config.cmake index aea0f54a99f54..c486002426abe 100644 --- a/hphp/runtime/ext/thrift/config.cmake +++ b/hphp/runtime/ext/thrift/config.cmake @@ -1,14 +1,14 @@ HHVM_DEFINE_EXTENSION("thrift" SOURCES adapter.cpp - field_adapter.cpp + field_wrapper.cpp binary.cpp compact.cpp ext_thrift.cpp spec-holder.cpp HEADERS adapter.h - field_adapter.h + field_wrapper.h util.h ext_thrift.h spec-holder.h diff --git a/hphp/runtime/ext/thrift/field_adapter.cpp b/hphp/runtime/ext/thrift/field_wrapper.cpp similarity index 64% rename from hphp/runtime/ext/thrift/field_adapter.cpp rename to hphp/runtime/ext/thrift/field_wrapper.cpp index 23dd98338c738..0db97f118dac5 100644 --- a/hphp/runtime/ext/thrift/field_adapter.cpp +++ b/hphp/runtime/ext/thrift/field_wrapper.cpp @@ -27,32 +27,38 @@ namespace HPHP { namespace thrift { /////////////////////////////////////////////////////////////////////////////// -const StaticString s_fieldAdapter("field_adapter"); - -Class* getFieldAdapter(const Array& spec) { - const auto adapterName = spec.lookup(s_fieldAdapter, AccessFlags::Key); - if (isNullType(adapterName.type())) { - return nullptr; - } - const auto adapterNameString = tvCastToString(adapterName); - const auto adapter = Class::load(adapterNameString.get()); - if (!adapter) { +void setThriftType(Variant value, const Object& obj, const String& fieldName) { + auto getter_name = "get_" + fieldName; + auto field_val = + obj->o_invoke_few_args(getter_name, RuntimeCoeffects::pure(), 0); + if (field_val.isObject()) { + auto wrapped_obj = field_val.toObject(); + wrapped_obj->o_invoke_few_args( + "setValue_DO_NOT_USE_THRIFT_INTERNAL", + RuntimeCoeffects::write_this_props(), + 1, + value); + } else { thrift_error( - folly::sformat("adapter class {} doesn't exist", adapterNameString), + folly::sformat("Method {} returned a non-object.", getter_name), ERR_INVALID_DATA); } - return adapter; -} - -void -setThriftType(Variant value, const Object& obj, const String& fieldName) { - auto setter_name = "set_" + fieldName; - obj->o_invoke_few_args(setter_name, RuntimeCoeffects::defaults(), 1,value); } Variant getThriftType(const Object& obj, const String& fieldName) { auto getter_name = "get_" + fieldName; - return obj->o_invoke_few_args(getter_name, RuntimeCoeffects::defaults(), 0); + auto field_val = + obj->o_invoke_few_args(getter_name, RuntimeCoeffects::pure(), 0); + if (field_val.isObject()) { + auto wrapped_obj = field_val.toObject(); + return wrapped_obj->o_invoke_few_args( + "getValue_DO_NOT_USE_THRIFT_INTERNAL", + RuntimeCoeffects::write_this_props(), + 0); + } + thrift_error( + folly::sformat("Method {} returned a non-object.", getter_name), + ERR_INVALID_DATA); } /////////////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/ext/thrift/field_adapter.h b/hphp/runtime/ext/thrift/field_wrapper.h similarity index 97% rename from hphp/runtime/ext/thrift/field_adapter.h rename to hphp/runtime/ext/thrift/field_wrapper.h index 66fa31d061070..06a127992f1a8 100644 --- a/hphp/runtime/ext/thrift/field_adapter.h +++ b/hphp/runtime/ext/thrift/field_wrapper.h @@ -25,8 +25,6 @@ namespace HPHP { namespace thrift { /////////////////////////////////////////////////////////////////////////////// -Class* getFieldAdapter(const Array& spec); - void setThriftType(Variant value, const Object& obj, const String& fieldName); diff --git a/hphp/runtime/ext/thrift/spec-holder.cpp b/hphp/runtime/ext/thrift/spec-holder.cpp index c17cb9a6b46b4..455cb0b9a2ca8 100644 --- a/hphp/runtime/ext/thrift/spec-holder.cpp +++ b/hphp/runtime/ext/thrift/spec-holder.cpp @@ -23,7 +23,7 @@ #include "hphp/runtime/ext/collections/ext_collections-set.h" #include "hphp/runtime/ext/collections/ext_collections-vector.h" #include "hphp/runtime/ext/thrift/adapter.h" -#include "hphp/runtime/ext/thrift/field_adapter.h" +#include "hphp/runtime/ext/thrift/field_wrapper.h" #include "hphp/runtime/ext/thrift/util.h" #include "hphp/util/hash-map.h" @@ -46,7 +46,8 @@ const StaticString s_vtype("vtype"), s_etype("etype"), s_format("format"), - s_SPEC("SPEC"); + s_SPEC("SPEC"), + s_isWrapped("is_wrapped"); Array get_tspec(const Class* cls) { auto lookup = cls->clsCnsGet(s_SPEC.get()); @@ -246,7 +247,8 @@ FieldSpec FieldSpec::compile(const Array& fieldSpec, bool topLevel) { break; } field.adapter = getAdapter(fieldSpec); - field.field_adapter = getFieldAdapter(fieldSpec); + field.isWrapped = tvCastToBoolean( + fieldSpec.lookup(s_isWrapped, AccessFlags::Key)); return field; } diff --git a/hphp/runtime/ext/thrift/spec-holder.h b/hphp/runtime/ext/thrift/spec-holder.h index b205bff91a935..f2e932d77aa17 100644 --- a/hphp/runtime/ext/thrift/spec-holder.h +++ b/hphp/runtime/ext/thrift/spec-holder.h @@ -47,7 +47,7 @@ struct FieldSpec { return StrNR(className_); } Class* adapter{}; - Class* field_adapter{}; + bool isWrapped{}; // true if the field has field_wrapper annotation bool noTypeCheck{}; // If this field doesn't need type checking // (conservatively). static FieldSpec compile(const Array& fieldSpec, bool topLevel); diff --git a/hphp/test/slow/thrift/hack_field_adapter.php b/hphp/test/slow/thrift/hack_field_adapter.php deleted file mode 100644 index 50b34d680463e..0000000000000 --- a/hphp/test/slow/thrift/hack_field_adapter.php +++ /dev/null @@ -1,165 +0,0 @@ - {} -interface IThriftFieldAdapter {} - -final class MyFieldWrapper implements IThriftFieldWrapper{ - public function __construct( - public T $value, - public int $fieldId, - public IThriftStruct $struct, - )[] {} - - public function setValue(T $value)[write_props]: void { - $this->value = $value; - } - - public function getValue()[]: T { - return $this->value; - } -} - -final class MyFieldAdapter implements IThriftFieldAdapter{ - public static function fromThrift( - TThriftType $thrift_obj, - int $field_id, - IThriftStruct $struct, - )[]: MyFieldWrapper { - return new MyFieldWrapper($thrift_obj, $field_id, $struct); - } - - public static function toThrift( - IThriftFieldWrapper $hack_obj, - )[]: TThriftType { - $hack_obj as MyFieldWrapper<_>; - return $hack_obj->getValue(); - } - - public static function assign( - IThriftFieldWrapper $hack_obj, - TThriftType $value, - )[write_props]: void { - $hack_obj as MyFieldWrapper<_>; - $hack_obj->setValue($value); - } - - public static function assignWrapped( - IThriftFieldWrapper $from, - IThriftFieldWrapper $to, - )[write_props]: void { - $from as MyFieldWrapper<_>; - $to as MyFieldWrapper<_>; - $from->setValue($to->getValue()); - } -} - - -class OuterStruct implements IThriftStruct{ - const SPEC = darray[ - 1 => darray[ - 'var' => 'value', - 'type' => \TType::I32, - 'field_adapter' => MyFieldAdapter::class, - ] - ]; - private ?\MyFieldWrapper $value; - - public function getWrapped_value()[]: \MyFieldWrapper { - return $this->value as nonnull; - } - - public function setWrapped_value(\MyFieldWrapper $value)[write_props]: void { - \MyFieldAdapter::assignWrapped($this->value as nonnull, $value); - } - - public function get_value()[]: ?int { - return \MyFieldAdapter::toThrift($this->value as nonnull); - } - - public function set_value(?int $value)[write_props]: void { - \MyFieldAdapter::assign($this->value as nonnull, $value); - } - - public function __construct()[] { - $this->value = \MyFieldAdapter::fromThrift(null, 1, $this); - } - - public static function withDefaultValues()[]: this { - return new static(); - } - - public function print(): void { - echo "----OuterStruct----\n"; - echo "\t\t value = "; - echo $this->get_value(); - echo "\n"; - } -} - -// This class is identical to OuterStruct but with all the adapters removed. -// It's used to "peek" at the actual serialized data without adapters getting -// in the way. -class OuterStructNoAdapter { - const SPEC = darray[ - 1 => darray[ - 'var' => 'value', - 'type' => \TType::I32, - ], - ]; - public ?int $value = null; - public function __construct()[] {} - public static function withDefaultValues()[]: this { - return new static(); - } - - public function print(): void { - echo "----OuterStructNoAdapter----\n"; - echo "\t\t value = "; - echo $this->value; - echo "\n"; - } -} - -function getStruct()[write_props] { - $v = new OuterStruct(); - $v->set_value(42); - return $v; -} - -function testBinary() { - $p = new DummyProtocol(); - $v = getStruct(); - $v->print(); - thrift_protocol_write_binary($p, 'foomethod', 1, $v, 20, true); - var_dump(md5($p->getTransport()->buff)); - $new_value = thrift_protocol_read_binary($p, 'OuterStruct', true); - $new_value->print(); - // Peek at what the serialized data actually looks like. - $p->getTransport()->pos = 0; - $new_value = thrift_protocol_read_binary($p, 'OuterStructNoAdapter', true); - $new_value->print(); -} - -function testCompact() { - $p = new DummyProtocol(); - $v = getStruct(); - $v->print(); - thrift_protocol_write_compact($p, 'foomethod', 2, $v, 20); - var_dump(md5($p->getTransport()->buff)); - $new_value = thrift_protocol_read_compact($p, 'OuterStruct'); - $new_value->print(); - - // Peek at what the serialized data actually looks like. - $p->getTransport()->pos = 0; - $new_value = thrift_protocol_read_compact($p, 'OuterStructNoAdapter'); - $new_value->print(); -} - -<<__EntryPoint>> -function main_forward_compatibility() { - require 'common.inc'; - echo "--- binary ---\n"; - testBinary(); - echo "--- compact ---\n"; - testCompact(); -} diff --git a/hphp/test/slow/thrift/hack_field_wrapper.php b/hphp/test/slow/thrift/hack_field_wrapper.php new file mode 100644 index 0000000000000..48d5d077bf19e --- /dev/null +++ b/hphp/test/slow/thrift/hack_field_wrapper.php @@ -0,0 +1,166 @@ + { + protected function __construct( + protected TThriftType $value, + protected int $fieldId, + protected TStruct $struct, + )[] {} + + final public function getValue_DO_NOT_USE_THRIFT_INTERNAL()[]: TThriftType { + return $this->value; + } + + final public function setValue_DO_NOT_USE_THRIFT_INTERNAL( + TThriftType $value, + )[write_props]: void { + $this->value = $value; + } + + final public static function fromThrift_DO_NOT_USE_THRIFT_INTERNAL( + TThriftType $value, + int $field_id, + TStruct $parent, + )[]: this { + return new static($value, $field_id, $parent); + } + + abstract public static function genToThrift( + this $value, + ): Awaitable; + + abstract public static function genFromThrift( + TThriftType $value, + int $field_id, + TStruct $parent, + ): Awaitable; + + abstract public function genUnwrap(): Awaitable; + abstract public function genWrap(TThriftType $value): Awaitable; +} + +final class MyFieldWrapper extends IThriftFieldWrapper{ + <<__Override>> + public static async function genToThrift( + this $value, + ): Awaitable { + return await $value->genUnwrap(); + } + + <<__Override>> + public static async function genFromThrift( + TThriftType $value, + int $field_id, + TStruct $parent, + ): Awaitable{ + return new static($value,$field_id,$parent); + } + + <<__Override>> + public async function genUnwrap(): Awaitable { + return $this->value; + } + + <<__Override>> + public async function genWrap(TThriftType $value): Awaitable { + $this->value = $value; + } +} + +class OuterStruct implements IThriftStruct{ + const SPEC = darray[ + 1 => darray[ + 'var' => 'value', + 'type' => \TType::I32, + 'is_wrapped' => true, + ] + ]; + private ?\MyFieldWrapper $value; + + public function get_value()[]: \MyFieldWrapper { + return $this->value as nonnull; + } + + public function __construct()[] { + $this->value = \MyFieldWrapper::fromThrift_DO_NOT_USE_THRIFT_INTERNAL(null, 1, $this); + } + + public static function withDefaultValues()[]: this { + return new static(); + } + + public async function print(): Awaitable { + echo "----OuterStruct----\n"; + echo "\t\t value = "; + echo await $this->get_value()->genUnwrap(); + echo "\n"; + } +} + +// This class is identical to OuterStruct but with all the adapters removed. +// It's used to "peek" at the actual serialized data without adapters getting +// in the way. +class OuterStructNoWrappedFields { + const SPEC = darray[ + 1 => darray[ + 'var' => 'value', + 'type' => \TType::I32, + ], + ]; + public ?int $value = null; + public function __construct()[] {} + public static function withDefaultValues()[]: this { + return new static(); + } + + public function print(): void { + echo "----OuterStructNoWrappedFields----\n"; + echo "\t\t value = "; + echo $this->value; + echo "\n"; + } +} + +async function getStruct(){ + $v = new OuterStruct(); + await $v->get_value()->genWrap(42); + return $v; +} + +async function testBinary() { + $p = new DummyProtocol(); + $v = await getStruct(); + $v->print(); + thrift_protocol_write_binary($p, 'foomethod', 1, $v, 20, true); + var_dump(md5($p->getTransport()->buff)); + $new_value = thrift_protocol_read_binary($p, 'OuterStruct', true); + await $new_value->print(); + // Peek at what the serialized data actually looks like. + $p->getTransport()->pos = 0; + $new_value = thrift_protocol_read_binary($p, 'OuterStructNoWrappedFields', true); + $new_value->print(); +} + +async function testCompact() { + $p = new DummyProtocol(); + $v = await getStruct(); + $v->print(); + thrift_protocol_write_compact($p, 'foomethod', 2, $v, 20); + var_dump(md5($p->getTransport()->buff)); + $new_value = thrift_protocol_read_compact($p, 'OuterStruct'); + await $new_value->print(); + + // Peek at what the serialized data actually looks like. + $p->getTransport()->pos = 0; + $new_value = thrift_protocol_read_compact($p, 'OuterStructNoWrappedFields'); + $new_value->print(); +} + +<<__EntryPoint>> +async function main_forward_compatibility() { + require 'common.inc'; + echo "--- binary ---\n"; + await testBinary(); + echo "--- compact ---\n"; + await testCompact(); +} diff --git a/hphp/test/slow/thrift/hack_field_adapter.php.expect b/hphp/test/slow/thrift/hack_field_wrapper.php.expect similarity index 80% rename from hphp/test/slow/thrift/hack_field_adapter.php.expect rename to hphp/test/slow/thrift/hack_field_wrapper.php.expect index e87da29484fb7..dbf6dd5797ed2 100644 --- a/hphp/test/slow/thrift/hack_field_adapter.php.expect +++ b/hphp/test/slow/thrift/hack_field_wrapper.php.expect @@ -4,7 +4,7 @@ string(32) "26cc06895b4bb7b431b2580f9443bb12" ----OuterStruct---- value = 42 -----OuterStructNoAdapter---- +----OuterStructNoWrappedFields---- value = 42 --- compact --- ----OuterStruct---- @@ -12,5 +12,5 @@ string(32) "26cc06895b4bb7b431b2580f9443bb12" string(32) "633119f2b60fec192ff10759cf557234" ----OuterStruct---- value = 42 -----OuterStructNoAdapter---- +----OuterStructNoWrappedFields---- value = 42 diff --git a/hphp/test/slow/thrift/hack_field_adapter.php.hphp_opts b/hphp/test/slow/thrift/hack_field_wrapper.php.hphp_opts similarity index 100% rename from hphp/test/slow/thrift/hack_field_adapter.php.hphp_opts rename to hphp/test/slow/thrift/hack_field_wrapper.php.hphp_opts diff --git a/hphp/test/slow/thrift/hack_field_adapter.php.opts b/hphp/test/slow/thrift/hack_field_wrapper.php.opts similarity index 100% rename from hphp/test/slow/thrift/hack_field_adapter.php.opts rename to hphp/test/slow/thrift/hack_field_wrapper.php.opts