From 3c387ea7e5fa31520ddbe05cab01a655c97b3b24 Mon Sep 17 00:00:00 2001 From: michaelbausor Date: Fri, 15 Feb 2019 16:16:42 -0800 Subject: [PATCH] PHP: Exclude repeated and map fields from normalization in constructor (#5723) * Exclude repeated and map fields from normalization * Remove erroneous comments * Remove unnecessary check for map type * Add support for repeated/map fields, add tests * Fix wrapper message in repeated/map fields in array constructor * Address PR comments * Removed unused code * Update docs --- php/ext/google/protobuf/message.c | 110 +++++++++++++++++- php/src/Google/Protobuf/Internal/Message.php | 55 +++++++-- .../proto/test_wrapper_type_setters.proto | 4 + php/tests/wrapper_type_setters_test.php | 84 +++++++++++++ 4 files changed, 240 insertions(+), 13 deletions(-) diff --git a/php/ext/google/protobuf/message.c b/php/ext/google/protobuf/message.c index ba3b11b5a1..a957f26717 100644 --- a/php/ext/google/protobuf/message.c +++ b/php/ext/google/protobuf/message.c @@ -294,6 +294,57 @@ void build_class_from_descriptor( // PHP Methods // ----------------------------------------------------------------------------- +static bool is_wrapper_msg(const upb_msgdef* m) { + upb_wellknowntype_t type = upb_msgdef_wellknowntype(m); + return type >= UPB_WELLKNOWN_DOUBLEVALUE && + type <= UPB_WELLKNOWN_BOOLVALUE; +} + +static void append_wrapper_message( + zend_class_entry* subklass, RepeatedField* intern, zval* value TSRMLS_DC) { + MessageHeader* submsg; + const upb_fielddef* field; +#if PHP_MAJOR_VERSION < 7 + zval* val = NULL; + MAKE_STD_ZVAL(val); + ZVAL_OBJ(val, subklass->create_object(subklass TSRMLS_CC)); + repeated_field_push_native(intern, &val); + submsg = UNBOX(MessageHeader, val); +#else + zend_object* obj = subklass->create_object(subklass TSRMLS_CC); + repeated_field_push_native(intern, &obj); + submsg = (MessageHeader*)((char*)obj - XtOffsetOf(MessageHeader, std)); +#endif + custom_data_init(subklass, submsg PHP_PROTO_TSRMLS_CC); + + field = upb_msgdef_itof(submsg->descriptor->msgdef, 1); + layout_set(submsg->descriptor->layout, submsg, field, value TSRMLS_CC); +} + +static void set_wrapper_message_as_map_value( + zend_class_entry* subklass, zval* map, zval* key, zval* value TSRMLS_DC) { + MessageHeader* submsg; + const upb_fielddef* field; +#if PHP_MAJOR_VERSION < 7 + zval* val = NULL; + MAKE_STD_ZVAL(val); + ZVAL_OBJ(val, subklass->create_object(subklass TSRMLS_CC)); + map_field_handlers->write_dimension( + map, key, val TSRMLS_CC); + submsg = UNBOX(MessageHeader, val); +#else + zval val; + zend_object* obj = subklass->create_object(subklass TSRMLS_CC); + ZVAL_OBJ(&val, obj); + map_field_handlers->write_dimension(map, key, &val TSRMLS_CC); + submsg = (MessageHeader*)((char*)obj - XtOffsetOf(MessageHeader, std)); +#endif + custom_data_init(subklass, submsg PHP_PROTO_TSRMLS_CC); + + field = upb_msgdef_itof(submsg->descriptor->msgdef, 1); + layout_set(submsg->descriptor->layout, submsg, field, value TSRMLS_CC); +} + void Message_construct(zval* msg, zval* array_wrapper) { TSRMLS_FETCH(); zend_class_entry* ce = Z_OBJCE_P(msg); @@ -336,14 +387,38 @@ void Message_construct(zval* msg, zval* array_wrapper) { HashPosition subpointer; zval subkey; void* memory; + bool is_wrapper = false; + zend_class_entry* subklass = NULL; + const upb_msgdef* mapentry = upb_fielddef_msgsubdef(field); + const upb_fielddef *value_field = upb_msgdef_itof(mapentry, 2); + + if (upb_fielddef_issubmsg(value_field)) { + const upb_msgdef* submsgdef = upb_fielddef_msgsubdef(value_field); + upb_wellknowntype_t type = upb_msgdef_wellknowntype(submsgdef); + is_wrapper = is_wrapper_msg(submsgdef); + + if (is_wrapper) { + PHP_PROTO_HASHTABLE_VALUE subdesc_php = get_def_obj(submsgdef); + Descriptor* subdesc = UNBOX_HASHTABLE_VALUE(Descriptor, subdesc_php); + subklass = subdesc->klass; + } + } + for (zend_hash_internal_pointer_reset_ex(subtable, &subpointer); php_proto_zend_hash_get_current_data_ex(subtable, (void**)&memory, &subpointer) == SUCCESS; zend_hash_move_forward_ex(subtable, &subpointer)) { zend_hash_get_current_key_zval_ex(subtable, &subkey, &subpointer); - map_field_handlers->write_dimension( - submap, &subkey, - CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC); + if (is_wrapper && + Z_TYPE_P(CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory)) != IS_OBJECT) { + set_wrapper_message_as_map_value( + subklass, submap, &subkey, + CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC); + } else { + map_field_handlers->write_dimension( + submap, &subkey, + CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC); + } zval_dtor(&subkey); } } else if (upb_fielddef_isseq(field)) { @@ -354,13 +429,36 @@ void Message_construct(zval* msg, zval* array_wrapper) { CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)value)); HashPosition subpointer; void* memory; + bool is_wrapper = false; + zend_class_entry* subklass = NULL; + + if (upb_fielddef_issubmsg(field)) { + const upb_msgdef* submsgdef = upb_fielddef_msgsubdef(field); + upb_wellknowntype_t type = upb_msgdef_wellknowntype(submsgdef); + is_wrapper = is_wrapper_msg(submsgdef); + + if (is_wrapper) { + PHP_PROTO_HASHTABLE_VALUE subdesc_php = get_def_obj(submsgdef); + Descriptor* subdesc = UNBOX_HASHTABLE_VALUE(Descriptor, subdesc_php); + subklass = subdesc->klass; + } + } + for (zend_hash_internal_pointer_reset_ex(subtable, &subpointer); php_proto_zend_hash_get_current_data_ex(subtable, (void**)&memory, &subpointer) == SUCCESS; zend_hash_move_forward_ex(subtable, &subpointer)) { - repeated_field_handlers->write_dimension( - subarray, NULL, - CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC); + if (is_wrapper && + Z_TYPE_P(CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory)) != IS_OBJECT) { + RepeatedField* intern = UNBOX(RepeatedField, subarray); + append_wrapper_message( + subklass, intern, + CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC); + } else { + repeated_field_handlers->write_dimension( + subarray, NULL, + CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC); + } } } else if (upb_fielddef_issubmsg(field)) { const upb_msgdef* submsgdef = upb_fielddef_msgsubdef(field); diff --git a/php/src/Google/Protobuf/Internal/Message.php b/php/src/Google/Protobuf/Internal/Message.php index 2c41c7aa75..1ff2dc9a67 100644 --- a/php/src/Google/Protobuf/Internal/Message.php +++ b/php/src/Google/Protobuf/Internal/Message.php @@ -976,9 +976,12 @@ class Message * ]); * ``` * + * This method will trigger an error if it is passed data that cannot + * be converted to the correct type. For example, a StringValue field + * must receive data that is either a string or a StringValue object. + * * @param array $array An array containing message properties and values. * @return null. - * @throws \Exception Invalid data. */ protected function mergeFromArray(array $array) { @@ -990,22 +993,61 @@ class Message 'Invalid message property: ' . $key); } $setter = $field->getSetter(); - if ($field->isWrapperType()) { - self::normalizeToMessageType($value, $field->getMessageType()->getClass()); + if ($field->isMap()) { + $valueField = $field->getMessageType()->getFieldByName('value'); + if (!is_null($valueField) && $valueField->isWrapperType()) { + self::normalizeArrayElementsToMessageType($value, $valueField->getMessageType()->getClass()); + } + } elseif ($field->isWrapperType()) { + $class = $field->getMessageType()->getClass(); + if ($field->isRepeated()) { + self::normalizeArrayElementsToMessageType($value, $class); + } else { + self::normalizeToMessageType($value, $class); + } } $this->$setter($value); } } + /** + * Tries to normalize the elements in $value into a provided protobuf + * wrapper type $class. If $value is any type other than array, we do + * not do any conversion, and instead rely on the existing protobuf + * type checking. If $value is an array, we process each element and + * try to convert it to an instance of $class. + * + * @param mixed $value The array of values to normalize. + * @param string $class The expected wrapper class name + */ + private static function normalizeArrayElementsToMessageType(&$value, $class) + { + if (!is_array($value)) { + // In the case that $value is not an array, we do not want to + // attempt any conversion. Note that this includes the cases + // when $value is a RepeatedField of MapField. In those cases, + // we do not need to convert the elements, as they should + // already be the correct types. + return; + } else { + // Normalize each element in the array. + foreach ($value as $key => &$elementValue) { + self::normalizeToMessageType($elementValue, $class); + } + } + } + /** * Tries to normalize $value into a provided protobuf wrapper type $class. * If $value is any type other than an object, we attempt to construct an * instance of $class and assign $value to it using the setValue method * shared by all wrapper types. * + * This method will raise an error if it receives a type that cannot be + * assigned to the wrapper type via setValue. + * * @param mixed $value The value to normalize. * @param string $class The expected wrapper class name - * @throws \Exception If $value cannot be converted to a wrapper type */ private static function normalizeToMessageType(&$value, $class) { @@ -1022,10 +1064,9 @@ class Message $value = $msg; return; } catch (\Exception $exception) { - throw new \Exception( + trigger_error( "Error normalizing value to type '$class': " . $exception->getMessage(), - $exception->getCode(), - $exception + E_USER_ERROR ); } } diff --git a/php/tests/proto/test_wrapper_type_setters.proto b/php/tests/proto/test_wrapper_type_setters.proto index e1e3309c54..41ca7f3f31 100644 --- a/php/tests/proto/test_wrapper_type_setters.proto +++ b/php/tests/proto/test_wrapper_type_setters.proto @@ -19,4 +19,8 @@ message TestWrapperSetters { google.protobuf.DoubleValue double_value_oneof = 10; google.protobuf.StringValue string_value_oneof = 11; } + + repeated google.protobuf.StringValue repeated_string_value = 12; + + map map_string_value = 13; } diff --git a/php/tests/wrapper_type_setters_test.php b/php/tests/wrapper_type_setters_test.php index 3d09c9a8c2..5509a175a1 100644 --- a/php/tests/wrapper_type_setters_test.php +++ b/php/tests/wrapper_type_setters_test.php @@ -225,4 +225,88 @@ class WrapperTypeSettersTest extends TestBase [TestWrapperSetters::class, BytesValue::class, 'bytes_value', 'getBytesValue', "nine"], ]; } + + /** + * @dataProvider constructorWithRepeatedWrapperTypeDataProvider + */ + public function testConstructorWithRepeatedWrapperType($wrapperField, $getter, $value) + { + $actualInstance = new TestWrapperSetters([$wrapperField => $value]); + foreach ($actualInstance->$getter() as $key => $actualWrapperValue) { + $actualInnerValue = $actualWrapperValue->getValue(); + $expectedElement = $value[$key]; + if (is_object($expectedElement) && is_a($expectedElement, '\Google\Protobuf\StringValue')) { + $expectedInnerValue = $expectedElement->getValue(); + } else { + $expectedInnerValue = $expectedElement; + } + $this->assertEquals($expectedInnerValue, $actualInnerValue); + } + } + + public function constructorWithRepeatedWrapperTypeDataProvider() + { + $sv7 = new StringValue(['value' => 'seven']); + $sv8 = new StringValue(['value' => 'eight']); + + $testWrapperSetters = new TestWrapperSetters(); + $testWrapperSetters->setRepeatedStringValue([$sv7, $sv8]); + $repeatedField = $testWrapperSetters->getRepeatedStringValue(); + + return [ + ['repeated_string_value', 'getRepeatedStringValue', []], + ['repeated_string_value', 'getRepeatedStringValue', [$sv7]], + ['repeated_string_value', 'getRepeatedStringValue', [$sv7, $sv8]], + ['repeated_string_value', 'getRepeatedStringValue', ['seven']], + ['repeated_string_value', 'getRepeatedStringValue', [7]], + ['repeated_string_value', 'getRepeatedStringValue', [7.7]], + ['repeated_string_value', 'getRepeatedStringValue', ['seven', 'eight']], + ['repeated_string_value', 'getRepeatedStringValue', [$sv7, 'eight']], + ['repeated_string_value', 'getRepeatedStringValue', ['seven', $sv8]], + ['repeated_string_value', 'getRepeatedStringValue', $repeatedField], + ]; + } + + /** + * @dataProvider constructorWithMapWrapperTypeDataProvider + */ + public function testConstructorWithMapWrapperType($wrapperField, $getter, $value) + { + $actualInstance = new TestWrapperSetters([$wrapperField => $value]); + foreach ($actualInstance->$getter() as $key => $actualWrapperValue) { + $actualInnerValue = $actualWrapperValue->getValue(); + $expectedElement = $value[$key]; + if (is_object($expectedElement) && is_a($expectedElement, '\Google\Protobuf\StringValue')) { + $expectedInnerValue = $expectedElement->getValue(); + } elseif (is_object($expectedElement) && is_a($expectedElement, '\Google\Protobuf\Internal\MapEntry')) { + $expectedInnerValue = $expectedElement->getValue()->getValue(); + } else { + $expectedInnerValue = $expectedElement; + } + $this->assertEquals($expectedInnerValue, $actualInnerValue); + } + } + + public function constructorWithMapWrapperTypeDataProvider() + { + $sv7 = new StringValue(['value' => 'seven']); + $sv8 = new StringValue(['value' => 'eight']); + + $testWrapperSetters = new TestWrapperSetters(); + $testWrapperSetters->setMapStringValue(['key' => $sv7, 'key2' => $sv8]); + $mapField = $testWrapperSetters->getMapStringValue(); + + return [ + ['map_string_value', 'getMapStringValue', []], + ['map_string_value', 'getMapStringValue', ['key' => $sv7]], + ['map_string_value', 'getMapStringValue', ['key' => $sv7, 'key2' => $sv8]], + ['map_string_value', 'getMapStringValue', ['key' => 'seven']], + ['map_string_value', 'getMapStringValue', ['key' => 7]], + ['map_string_value', 'getMapStringValue', ['key' => 7.7]], + ['map_string_value', 'getMapStringValue', ['key' => 'seven', 'key2' => 'eight']], + ['map_string_value', 'getMapStringValue', ['key' => $sv7, 'key2' => 'eight']], + ['map_string_value', 'getMapStringValue', ['key' => 'seven', 'key2' => $sv8]], + ['map_string_value', 'getMapStringValue', $mapField], + ]; + } }