From 616e68ecc1c997c8b4c22a708cc9f6605a329bef Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Fri, 10 Mar 2017 13:42:59 -0800 Subject: Repeated/Map field setter should accept a regular PHP array (#2817) Accept regular PHP array for repeated/map setter. Existing map/repeated field will be swapped by a clean map/repeated field. Then, elements in the array will be added to the map/repeated field. All elements will be type checked before adding. See #2686 for detail. --- php/ext/google/protobuf/array.c | 20 ++-- php/ext/google/protobuf/map.c | 27 +++-- php/ext/google/protobuf/protobuf.h | 12 ++- php/ext/google/protobuf/storage.c | 6 +- php/ext/google/protobuf/type_check.c | 116 ++++++++++++++++++++-- php/src/Google/Protobuf/Internal/GPBUtil.php | 68 ++++++++++--- php/src/Google/Protobuf/Internal/MapField.php | 24 +++++ php/tests/generated_class_test.php | 95 ++++++++++++++++++ src/google/protobuf/compiler/php/php_generator.cc | 28 +++++- 9 files changed, 355 insertions(+), 41 deletions(-) diff --git a/php/ext/google/protobuf/array.c b/php/ext/google/protobuf/array.c index 63bb6d0a..ab2eaf90 100644 --- a/php/ext/google/protobuf/array.c +++ b/php/ext/google/protobuf/array.c @@ -225,8 +225,17 @@ void repeated_field_push_native(RepeatedField *intern, void *value TSRMLS_DC) { zend_hash_next_index_insert(ht, (void **)value, size, NULL); } +void repeated_field_create_with_field(zend_class_entry *ce, + const upb_fielddef *field, + zval **repeated_field TSRMLS_DC) { + upb_fieldtype_t type = upb_fielddef_type(field); + const zend_class_entry *msg_ce = field_type_class(field TSRMLS_CC); + repeated_field_create_with_type(ce, type, msg_ce, repeated_field); +} + void repeated_field_create_with_type(zend_class_entry *ce, - const upb_fielddef *field, + upb_fieldtype_t type, + const zend_class_entry* msg_ce, zval **repeated_field TSRMLS_DC) { MAKE_STD_ZVAL(*repeated_field); Z_TYPE_PP(repeated_field) = IS_OBJECT; @@ -235,13 +244,8 @@ void repeated_field_create_with_type(zend_class_entry *ce, RepeatedField *intern = zend_object_store_get_object(*repeated_field TSRMLS_CC); - intern->type = upb_fielddef_type(field); - if (intern->type == UPB_TYPE_MESSAGE) { - const upb_msgdef *msg = upb_fielddef_msgsubdef(field); - zval *desc_php = get_def_obj(msg); - Descriptor *desc = zend_object_store_get_object(desc_php TSRMLS_CC); - intern->msg_ce = desc->klass; - } + intern->type = type; + intern->msg_ce = msg_ce; MAKE_STD_ZVAL(intern->array); repeated_field_array_init(intern->array, intern->type, 0 ZEND_FILE_LINE_CC); diff --git a/php/ext/google/protobuf/map.c b/php/ext/google/protobuf/map.c index ab98879d..4a3829d6 100644 --- a/php/ext/google/protobuf/map.c +++ b/php/ext/google/protobuf/map.c @@ -146,6 +146,11 @@ static zend_function_entry map_field_methods[] = { ZEND_FE_END }; +// Forward declare static functions. + +static bool map_field_write_dimension(zval *object, zval *key, + zval *value TSRMLS_DC); + // ----------------------------------------------------------------------------- // MapField creation/desctruction // ----------------------------------------------------------------------------- @@ -183,6 +188,7 @@ void map_field_init(TSRMLS_D) { map_field_handlers = PEMALLOC(zend_object_handlers); memcpy(map_field_handlers, zend_get_std_object_handlers(), sizeof(zend_object_handlers)); + map_field_handlers->write_dimension = map_field_write_dimension; map_field_handlers->get_gc = map_field_get_gc; } @@ -235,7 +241,18 @@ void map_field_free(void *object TSRMLS_DC) { efree(object); } -void map_field_create_with_type(zend_class_entry *ce, const upb_fielddef *field, +void map_field_create_with_field(zend_class_entry *ce, const upb_fielddef *field, + zval **map_field TSRMLS_DC) { + const upb_fielddef *key_field = map_field_key(field); + const upb_fielddef *value_field = map_field_value(field); + map_field_create_with_type( + ce, upb_fielddef_type(key_field), upb_fielddef_type(value_field), + field_type_class(value_field TSRMLS_CC), map_field); +} + +void map_field_create_with_type(zend_class_entry *ce, upb_fieldtype_t key_type, + upb_fieldtype_t value_type, + const zend_class_entry *msg_ce, zval **map_field TSRMLS_DC) { MAKE_STD_ZVAL(*map_field); Z_TYPE_PP(map_field) = IS_OBJECT; @@ -245,11 +262,9 @@ void map_field_create_with_type(zend_class_entry *ce, const upb_fielddef *field, Map* intern = (Map*)zend_object_store_get_object(*map_field TSRMLS_CC); - const upb_fielddef *key_field = map_field_key(field); - const upb_fielddef *value_field = map_field_value(field); - intern->key_type = upb_fielddef_type(key_field); - intern->value_type = upb_fielddef_type(value_field); - intern->msg_ce = field_type_class(value_field TSRMLS_CC); + intern->key_type = key_type; + intern->value_type = value_type; + intern->msg_ce = msg_ce; } static void map_field_free_element(void *object) { diff --git a/php/ext/google/protobuf/protobuf.h b/php/ext/google/protobuf/protobuf.h index 28cea95c..2287f7e6 100644 --- a/php/ext/google/protobuf/protobuf.h +++ b/php/ext/google/protobuf/protobuf.h @@ -296,6 +296,7 @@ PHP_METHOD(Util, checkBool); PHP_METHOD(Util, checkString); PHP_METHOD(Util, checkBytes); PHP_METHOD(Util, checkMessage); +PHP_METHOD(Util, checkMapField); PHP_METHOD(Util, checkRepeatedField); // ----------------------------------------------------------------------------- @@ -349,7 +350,11 @@ const upb_fielddef* map_entry_key(const upb_msgdef* msgdef); const upb_fielddef* map_entry_value(const upb_msgdef* msgdef); zend_object_value map_field_create(zend_class_entry *ce TSRMLS_DC); -void map_field_create_with_type(zend_class_entry *ce, const upb_fielddef *field, +void map_field_create_with_field(zend_class_entry *ce, const upb_fielddef *field, + zval **map_field TSRMLS_DC); +void map_field_create_with_type(zend_class_entry *ce, upb_fieldtype_t key_type, + upb_fieldtype_t value_type, + const zend_class_entry *msg_ce, zval **map_field TSRMLS_DC); void map_field_free(void* object TSRMLS_DC); void* upb_value_memory(upb_value* v); @@ -392,9 +397,12 @@ struct RepeatedFieldIter { long position; }; -void repeated_field_create_with_type(zend_class_entry* ce, +void repeated_field_create_with_field(zend_class_entry* ce, const upb_fielddef* field, zval** repeated_field TSRMLS_DC); +void repeated_field_create_with_type(zend_class_entry* ce, upb_fieldtype_t type, + const zend_class_entry* msg_ce, + zval** repeated_field TSRMLS_DC); // Return the element at the index position from the repeated field. There is // not restriction on the type of stored elements. void *repeated_field_index_native(RepeatedField *intern, int index TSRMLS_DC); diff --git a/php/ext/google/protobuf/storage.c b/php/ext/google/protobuf/storage.c index 1b239ee3..af7c292f 100644 --- a/php/ext/google/protobuf/storage.c +++ b/php/ext/google/protobuf/storage.c @@ -517,12 +517,12 @@ void layout_init(MessageLayout* layout, void* storage, *oneof_case = ONEOF_CASE_NONE; } else if (is_map_field(field)) { zval_ptr_dtor(property_ptr); - map_field_create_with_type(map_field_type, field, property_ptr TSRMLS_CC); + map_field_create_with_field(map_field_type, field, property_ptr TSRMLS_CC); DEREF(memory, zval**) = property_ptr; } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) { zval_ptr_dtor(property_ptr); - repeated_field_create_with_type(repeated_field_type, field, - property_ptr TSRMLS_CC); + repeated_field_create_with_field(repeated_field_type, field, + property_ptr TSRMLS_CC); DEREF(memory, zval**) = property_ptr; } else { native_slot_init(upb_fielddef_type(field), memory, property_ptr); diff --git a/php/ext/google/protobuf/type_check.c b/php/ext/google/protobuf/type_check.c index d12d0025..718682ac 100644 --- a/php/ext/google/protobuf/type_check.c +++ b/php/ext/google/protobuf/type_check.c @@ -51,6 +51,13 @@ ZEND_BEGIN_ARG_INFO_EX(arg_check_repeated, 0, 0, 2) ZEND_ARG_INFO(0, klass) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_INFO_EX(arg_check_map, 0, 0, 3) + ZEND_ARG_INFO(1, val) + ZEND_ARG_INFO(0, key_type) + ZEND_ARG_INFO(0, value_type) + ZEND_ARG_INFO(0, klass) +ZEND_END_ARG_INFO() + static zend_function_entry util_methods[] = { PHP_ME(Util, checkInt32, arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) PHP_ME(Util, checkUint32, arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) @@ -63,6 +70,7 @@ static zend_function_entry util_methods[] = { PHP_ME(Util, checkString, arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) PHP_ME(Util, checkBytes, arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) PHP_ME(Util, checkMessage, arg_check_message, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) + PHP_ME(Util, checkMapField, arg_check_map, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) PHP_ME(Util, checkRepeatedField, arg_check_repeated, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) ZEND_FE_END @@ -411,20 +419,112 @@ PHP_METHOD(Util, checkRepeatedField) { zval* val; long type; const zend_class_entry* klass = NULL; - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "Ol|C", &val, - repeated_field_type, &type, &klass) == FAILURE) { + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "zl|C", &val, &type, + &klass) == FAILURE) { return; } - RepeatedField *intern = - (RepeatedField *)zend_object_store_get_object(val TSRMLS_CC); - if (to_fieldtype(type) != intern->type) { + if (Z_TYPE_P(val) == IS_ARRAY) { + HashTable* table = Z_ARRVAL_P(val); + HashPosition pointer; + void* memory; + zval* repeated_field; + + repeated_field_create_with_type(repeated_field_type, to_fieldtype(type), + klass, &repeated_field TSRMLS_CC); + RepeatedField* intern = + (RepeatedField*)zend_object_store_get_object(repeated_field TSRMLS_CC); + + for (zend_hash_internal_pointer_reset_ex(table, &pointer); + zend_hash_get_current_data_ex(table, (void**)&memory, &pointer) == + SUCCESS; + zend_hash_move_forward_ex(table, &pointer)) { + repeated_field_handlers->write_dimension(repeated_field, NULL, *(zval**)memory); + } + + Z_DELREF_P(repeated_field); + RETURN_ZVAL(repeated_field, 1, 0); + + } else if (Z_TYPE_P(val) == IS_OBJECT) { + if (!instanceof_function(Z_OBJCE_P(val), repeated_field_type TSRMLS_CC)) { + zend_error(E_USER_ERROR, "Given value is not an instance of %s.", + repeated_field_type->name); + return; + } + RepeatedField* intern = + (RepeatedField*)zend_object_store_get_object(val TSRMLS_CC); + if (to_fieldtype(type) != intern->type) { + zend_error(E_USER_ERROR, "Incorrect repeated field type."); + return; + } + if (klass != NULL && intern->msg_ce != klass) { + zend_error(E_USER_ERROR, + "Expect a repeated field of %s, but %s is given.", klass->name, + intern->msg_ce->name); + return; + } + RETURN_ZVAL(val, 1, 0); + } else { zend_error(E_USER_ERROR, "Incorrect repeated field type."); return; } - if (klass != NULL && intern->msg_ce != klass) { - zend_error(E_USER_ERROR, "Expect a repeated field of %s, but %s is given.", - klass->name, intern->msg_ce->name); + +} + +PHP_METHOD(Util, checkMapField) { + zval* val; + long key_type, value_type; + const zend_class_entry* klass = NULL; + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "zll|C", &val, &key_type, + &value_type, &klass) == FAILURE) { + return; + } + + if (Z_TYPE_P(val) == IS_ARRAY) { + HashTable* table = Z_ARRVAL_P(val); + HashPosition pointer; + zval key, *map_field; + void* value; + + map_field_create_with_type(map_field_type, to_fieldtype(key_type), + to_fieldtype(value_type), klass, + &map_field TSRMLS_CC); + Map* intern = + (Map*)zend_object_store_get_object(map_field TSRMLS_CC); + + for (zend_hash_internal_pointer_reset_ex(table, &pointer); + zend_hash_get_current_data_ex(table, (void**)&value, &pointer) == + SUCCESS; + zend_hash_move_forward_ex(table, &pointer)) { + zend_hash_get_current_key_zval_ex(table, &key, &pointer); + map_field_handlers->write_dimension(map_field, &key, *(zval**)value); + } + + Z_DELREF_P(map_field); + RETURN_ZVAL(map_field, 1, 0); + } else if (Z_TYPE_P(val) == IS_OBJECT) { + if (!instanceof_function(Z_OBJCE_P(val), map_field_type TSRMLS_CC)) { + zend_error(E_USER_ERROR, "Given value is not an instance of %s.", + map_field_type->name); + return; + } + Map* intern = (Map*)zend_object_store_get_object(val TSRMLS_CC); + if (to_fieldtype(key_type) != intern->key_type) { + zend_error(E_USER_ERROR, "Incorrect map field key type."); + return; + } + if (to_fieldtype(value_type) != intern->value_type) { + zend_error(E_USER_ERROR, "Incorrect map field value type."); + return; + } + if (klass != NULL && intern->msg_ce != klass) { + zend_error(E_USER_ERROR, "Expect a map field of %s, but %s is given.", + klass->name, intern->msg_ce->name); + return; + } + RETURN_ZVAL(val, 1, 0); + } else { + zend_error(E_USER_ERROR, "Incorrect map field type."); return; } } diff --git a/php/src/Google/Protobuf/Internal/GPBUtil.php b/php/src/Google/Protobuf/Internal/GPBUtil.php index ba1d2eb3..0e66ae6f 100644 --- a/php/src/Google/Protobuf/Internal/GPBUtil.php +++ b/php/src/Google/Protobuf/Internal/GPBUtil.php @@ -34,6 +34,7 @@ namespace Google\Protobuf\Internal; use Google\Protobuf\Internal\GPBType; use Google\Protobuf\Internal\RepeatedField; +use Google\Protobuf\Internal\MapField; class GPBUtil { @@ -175,19 +176,60 @@ class GPBUtil public static function checkRepeatedField(&$var, $type, $klass = null) { - if (!$var instanceof RepeatedField) { - trigger_error("Expect repeated field.", E_USER_ERROR); - } - if ($var->getType() != $type) { - trigger_error( - "Expect repeated field of different type.", - E_USER_ERROR); - } - if ($var->getType() === GPBType::MESSAGE && - $var->getClass() !== $klass) { - trigger_error( - "Expect repeated field of different message.", - E_USER_ERROR); + if (!$var instanceof RepeatedField && !is_array($var)) { + trigger_error("Expect array.", E_USER_ERROR); + } + if (is_array($var)) { + $tmp = new RepeatedField($type, $klass); + foreach ($var as $value) { + $tmp[] = $value; + } + return $tmp; + } else { + if ($var->getType() != $type) { + trigger_error( + "Expect repeated field of different type.", + E_USER_ERROR); + } + if ($var->getType() === GPBType::MESSAGE && + $var->getClass() !== $klass) { + trigger_error( + "Expect repeated field of different message.", + E_USER_ERROR); + } + return $var; + } + } + + public static function checkMapField(&$var, $key_type, $value_type, $klass = null) + { + if (!$var instanceof MapField && !is_array($var)) { + trigger_error("Expect dict.", E_USER_ERROR); + } + if (is_array($var)) { + $tmp = new MapField($key_type, $value_type, $klass); + foreach ($var as $key => $value) { + $tmp[$key] = $value; + } + return $tmp; + } else { + if ($var->getKeyType() != $key_type) { + trigger_error( + "Expect map field of key type.", + E_USER_ERROR); + } + if ($var->getValueType() != $value_type) { + trigger_error( + "Expect map field of value type.", + E_USER_ERROR); + } + if ($var->getValueType() === GPBType::MESSAGE && + $var->getValueClass() !== $klass) { + trigger_error( + "Expect map field of different value message.", + E_USER_ERROR); + } + return $var; } } diff --git a/php/src/Google/Protobuf/Internal/MapField.php b/php/src/Google/Protobuf/Internal/MapField.php index 14ee7ebe..68c10c08 100644 --- a/php/src/Google/Protobuf/Internal/MapField.php +++ b/php/src/Google/Protobuf/Internal/MapField.php @@ -203,6 +203,30 @@ class MapField implements \ArrayAccess, \IteratorAggregate, \Countable $this->klass = $klass; } + /** + * @ignore + */ + public function getKeyType() + { + return $this->key_type; + } + + /** + * @ignore + */ + public function getValueType() + { + return $this->value_type; + } + + /** + * @ignore + */ + public function getValueClass() + { + return $this->klass; + } + /** * Return the element at the given key. * diff --git a/php/tests/generated_class_test.php b/php/tests/generated_class_test.php index 7f8567b8..4c3bca2d 100644 --- a/php/tests/generated_class_test.php +++ b/php/tests/generated_class_test.php @@ -6,6 +6,7 @@ require_once('test_base.php'); require_once('test_util.php'); use Google\Protobuf\Internal\RepeatedField; +use Google\Protobuf\Internal\MapField; use Google\Protobuf\Internal\GPBType; use Foo\TestEnum; use Foo\TestMessage; @@ -526,6 +527,26 @@ class GeneratedClassTest extends TestBase $this->assertSame($repeated_int32, $m->getRepeatedInt32()); } + public function testRepeatedFieldViaArray() + { + $m = new TestMessage(); + + $arr = array(); + $m->setRepeatedInt32($arr); + $this->assertSame(0, count($m->getRepeatedInt32())); + + $arr = array(1, 2.1, "3"); + $m->setRepeatedInt32($arr); + $this->assertTrue($m->getRepeatedInt32() instanceof RepeatedField); + $this->assertSame("Google\Protobuf\Internal\RepeatedField", + get_class($m->getRepeatedInt32())); + $this->assertSame(3, count($m->getRepeatedInt32())); + $this->assertSame(1, $m->getRepeatedInt32()[0]); + $this->assertSame(2, $m->getRepeatedInt32()[1]); + $this->assertSame(3, $m->getRepeatedInt32()[2]); + $this->assertFalse($arr instanceof RepeatedField); + } + /** * @expectedException PHPUnit_Framework_Error */ @@ -568,6 +589,80 @@ class GeneratedClassTest extends TestBase $m->setRepeatedMessage($repeated_message); } + ######################################################### + # Test map field. + ######################################################### + + public function testMapField() + { + $m = new TestMessage(); + + $map_int32_int32 = new MapField(GPBType::INT32, GPBType::INT32); + $m->setMapInt32Int32($map_int32_int32); + $this->assertSame($map_int32_int32, $m->getMapInt32Int32()); + } + + public function testMapFieldViaArray() + { + $m = new TestMessage(); + + $dict = array(); + $m->setMapInt32Int32($dict); + $this->assertSame(0, count($m->getMapInt32Int32())); + + $dict = array(5 => 5, 6.1 => 6.1, "7" => "7"); + $m->setMapInt32Int32($dict); + $this->assertTrue($m->getMapInt32Int32() instanceof MapField); + $this->assertSame(3, count($m->getMapInt32Int32())); + $this->assertSame(5, $m->getMapInt32Int32()[5]); + $this->assertSame(6, $m->getMapInt32Int32()[6]); + $this->assertSame(7, $m->getMapInt32Int32()[7]); + $this->assertFalse($dict instanceof MapField); + } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testMapFieldWrongTypeFail() + { + $m = new TestMessage(); + $a = 1; + $m->setMapInt32Int32($a); + } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testMapFieldWrongObjectFail() + { + $m = new TestMessage(); + $m->setMapInt32Int32($m); + } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testMapFieldWrongRepeatedTypeFail() + { + $m = new TestMessage(); + + $map_uint32_uint32 = new MapField(GPBType::UINT32, GPBType::UINT32); + $m->setMapInt32Int32($map_uint32_uint32); + } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testMapFieldWrongRepeatedMessageClassFail() + { + $m = new TestMessage(); + + $map_int32_message = new MapField(GPBType::INT32, + GPBType::MESSAGE, + TestMessage::class); + $m->setMapInt32Message($map_int32_message); + } + ######################################################### # Test oneof field. ######################################################### diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc index ec9a2365..7f35d712 100644 --- a/src/google/protobuf/compiler/php/php_generator.cc +++ b/src/google/protobuf/compiler/php/php_generator.cc @@ -439,9 +439,31 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor, // Type check. if (field->is_map()) { + const Descriptor* map_entry = field->message_type(); + const FieldDescriptor* key = map_entry->FindFieldByName("key"); + const FieldDescriptor* value = map_entry->FindFieldByName("value"); + printer->Print( + "$arr = GPBUtil::checkMapField($var, " + "\\Google\\Protobuf\\Internal\\GPBType::^key_type^, " + "\\Google\\Protobuf\\Internal\\GPBType::^value_type^", + "key_type", ToUpper(key->type_name()), + "value_type", ToUpper(value->type_name())); + if (value->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) { + printer->Print( + ", \\^class_name^);\n", + "class_name", + MessageName(value->message_type(), is_descriptor) + "::class"); + } else if (value->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) { + printer->Print( + ", ^class_name^);\n", + "class_name", + EnumName(value->enum_type(), is_descriptor) + "::class"); + } else { + printer->Print(");\n"); + } } else if (field->is_repeated()) { printer->Print( - "GPBUtil::checkRepeatedField($var, " + "$arr = GPBUtil::checkRepeatedField($var, " "\\Google\\Protobuf\\Internal\\GPBType::^type^", "type", ToUpper(field->type_name())); if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) { @@ -480,6 +502,10 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor, printer->Print( "$this->writeOneof(^number^, $var);\n", "number", IntToString(field->number())); + } else if (field->is_repeated()) { + printer->Print( + "$this->^name^ = $arr;\n", + "name", field->name()); } else { printer->Print( "$this->^name^ = $var;\n", -- cgit v1.2.3