Skip to content

Commit

Permalink
Update field_adapter extension as per revised wrapper design
Browse files Browse the repository at this point in the history
Summary:
Facebook :
Design doc - https://fb.quip.com/mzaaAOaErmEN

Revised design -
- struct will only have a single getter method to get the wrapped object
- wrapper will have thrift only methods that can be used during serialization.
- No separate adapter class

In this diff change the hhvm serialization extension as per the revised field wrapper design .

Old FieldAdapter implementation -
```
class MyStruct {
   MyWrapper<int> $field;
   public function getWrapped_field():MyWrapper<int> { ... }
   public function setWrapped_field(MyWrapper<int>):void { ... }
   public function get_field():int { ... }
   public function set_field(int):void { ... }
}

class MyAdapter { ... }
class MyWrapper { ... }
```
Revised design
```
class MyStruct {
   MyWrapper<int> $field;
   public function get_field():MyWrapper<int> { ... }

}

class MyWrapper {
   public function getValue_DO_NOT_USE_THRIFT_INTERNAL() { ... }
   public function setValue_DO_NOT_USE_THRIFT_INTERNAL($val) { ... }
   .....
 }
```

#forcetdhashing

Reviewed By: andrii-korotkov

Differential Revision: D34590769

fbshipit-source-id: da6a1c956cc9a2bfbd4ccba492540423ce0e58c5
  • Loading branch information
rmakheja authored and facebook-github-bot committed Mar 10, 2022
1 parent 65b1c95 commit c559c6b
Show file tree
Hide file tree
Showing 12 changed files with 211 additions and 204 deletions.
10 changes: 5 additions & 5 deletions hphp/runtime/ext/thrift/binary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 5 additions & 5 deletions hphp/runtime/ext/thrift/compact.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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));
Expand Down
4 changes: 2 additions & 2 deletions hphp/runtime/ext/thrift/config.cmake
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

///////////////////////////////////////////////////////////////////////////////
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ namespace HPHP {
namespace thrift {
///////////////////////////////////////////////////////////////////////////////

Class* getFieldAdapter(const Array& spec);

void
setThriftType(Variant value, const Object& obj, const String& fieldName);

Expand Down
8 changes: 5 additions & 3 deletions hphp/runtime/ext/thrift/spec-holder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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());
Expand Down Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion hphp/runtime/ext/thrift/spec-holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
165 changes: 0 additions & 165 deletions hphp/test/slow/thrift/hack_field_adapter.php

This file was deleted.

Loading

0 comments on commit c559c6b

Please sign in to comment.