From 577f407df96ffe15177b5435ba99db56ae0129d8 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Wed, 23 Jul 2014 19:04:12 +0200 Subject: [PATCH] THRIFT-2632 add "validate" option to generate read/write validation code Client: PHP Patch: Stig Bakken & Jens Geyer Modifications made to the original pull request: - moved TestValidators.* to lib/php/test - created new TestValidators.thrift to house the UnionOfStrings union - modified makefiles accordingly This closes #159 --- compiler/cpp/src/generate/t_php_generator.cc | 115 ++++++++++++++- lib/php/test/Makefile.am | 16 ++- lib/php/test/Test/Thrift/TestValidators.php | 142 +++++++++++++++++++ lib/php/test/TestValidators.thrift | 28 ++++ 4 files changed, 295 insertions(+), 6 deletions(-) create mode 100644 lib/php/test/Test/Thrift/TestValidators.php create mode 100644 lib/php/test/TestValidators.thrift diff --git a/compiler/cpp/src/generate/t_php_generator.cc b/compiler/cpp/src/generate/t_php_generator.cc index 1eb759caa..2d8fe00df 100644 --- a/compiler/cpp/src/generate/t_php_generator.cc +++ b/compiler/cpp/src/generate/t_php_generator.cc @@ -70,6 +70,9 @@ class t_php_generator : public t_oop_generator { iter = parsed_options.find("oop"); oop_ = (iter != parsed_options.end()); + iter = parsed_options.find("validate"); + validate_ = (iter != parsed_options.end()); + iter = parsed_options.find("nsglobal"); if(iter != parsed_options.end()) { nsglobal_ = iter->second; @@ -118,6 +121,12 @@ class t_php_generator : public t_oop_generator { void generate_php_struct_reader(std::ofstream& out, t_struct* tstruct); void generate_php_struct_writer(std::ofstream& out, t_struct* tstruct); void generate_php_function_helpers(t_function* tfunction); + void generate_php_struct_required_validator(ofstream& out, t_struct* tstruct, std::string method_name, bool write_mode); + void generate_php_struct_read_validator(ofstream& out, t_struct* tstruct); + void generate_php_struct_write_validator(ofstream& out, t_struct* tstruct); + bool needs_php_write_validator(t_struct* tstruct); + bool needs_php_read_validator(t_struct* tstruct); + int get_php_num_required_fields(const vector& fields, bool write_mode); void generate_php_type_spec(std::ofstream &out, t_type* t); void generate_php_struct_spec(std::ofstream &out, t_struct* tstruct); @@ -363,6 +372,11 @@ class t_php_generator : public t_oop_generator { */ bool oop_; + /** + * Whether to generate validator code + */ + bool validate_; + /** * Global namespace for PHP 5.3 */ @@ -817,6 +831,12 @@ void t_php_generator::generate_php_struct_definition(ofstream& out, generate_php_struct_reader(out, tstruct); generate_php_struct_writer(out, tstruct); + if (needs_php_read_validator(tstruct)) { + generate_php_struct_read_validator(out, tstruct); + } + if (needs_php_write_validator(tstruct)) { + generate_php_struct_write_validator(out, tstruct); + } indent_down(); out << @@ -837,8 +857,15 @@ void t_php_generator::generate_php_struct_reader(ofstream& out, scope_up(out); if (oop_) { - indent(out) << "return $this->_read('" << tstruct->get_name() << "', self::$_TSPEC, $input);" << endl; + if (needs_php_read_validator(tstruct)) { + indent(out) << "$tmp = $this->_read('" << tstruct->get_name() << "', self::$_TSPEC, $input);" << endl; + indent(out) << "$this->_validateForRead();" << endl; + indent(out) << "return $tmp;" << endl; + } else { + indent(out) << "return $this->_read('" << tstruct->get_name() << "', self::$_TSPEC, $input);" << endl; + } scope_down(out); + out << endl; return; } @@ -936,6 +963,11 @@ void t_php_generator::generate_php_struct_reader(ofstream& out, "$xfer += $input->readStructEnd();" << endl; } + if (needs_php_read_validator(tstruct)) { + indent(out) << + "$this->_validateForRead();" << endl; + } + indent(out) << "return $xfer;" << endl; @@ -963,9 +995,14 @@ void t_php_generator::generate_php_struct_writer(ofstream& out, } indent_up(); + if (needs_php_write_validator(tstruct)) { + indent(out) << "$this->_validateForWrite();" << endl; + } + if (oop_) { indent(out) << "return $this->_write('" << tstruct->get_name() << "', self::$_TSPEC, $output);" << endl; scope_down(out); + out << endl; return; } @@ -1038,9 +1075,78 @@ void t_php_generator::generate_php_struct_writer(ofstream& out, indent() << "return $xfer;" << endl; indent_down(); - out << - indent() << "}" << endl << - endl; + out << indent() << "}" << endl << endl; +} + +void t_php_generator::generate_php_struct_read_validator(ofstream& out, + t_struct* tstruct) { + generate_php_struct_required_validator(out, tstruct, "_validateForRead", false); +} + +void t_php_generator::generate_php_struct_write_validator(ofstream& out, + t_struct* tstruct) { + generate_php_struct_required_validator(out, tstruct, "_validateForWrite", true); +} + +void t_php_generator::generate_php_struct_required_validator(ofstream& out, + t_struct* tstruct, + std::string method_name, + bool write_mode) { + indent(out) << + "private function " << method_name << "() {" << endl; + indent_up(); + + const vector& fields = tstruct->get_members(); + + if (fields.size() > 0) { + vector::const_iterator f_iter; + + for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) { + t_field* field = (*f_iter); + if (field->get_req() == t_field::T_REQUIRED || + (field->get_req() == t_field::T_OPT_IN_REQ_OUT && write_mode)) { + indent(out) << + "if ($this->" << field->get_name() << " === null) {" << endl; + indent_up(); + indent(out) << + "throw new TProtocolException('Required field " << + tstruct->get_name() << "." << field->get_name() << " is unset!');" << endl; + indent_down(); + indent(out) << + "}" << endl; + } + } + } + + indent_down(); + indent(out) << "}" << endl << endl; +} + +int t_php_generator::get_php_num_required_fields(const vector& fields, + bool write_mode) { + int num_req = 0; + + if (fields.size() > 0) { + vector::const_iterator f_iter; + for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) { + if ((*f_iter)->get_req() == t_field::T_REQUIRED || + ((*f_iter)->get_req() == t_field::T_OPT_IN_REQ_OUT && write_mode)) { + ++num_req; + } + } + } + return num_req; +} + +bool t_php_generator::needs_php_write_validator(t_struct* tstruct) { + return (validate_ && + !tstruct->is_union() && + get_php_num_required_fields(tstruct->get_members(), true) > 0); +} + +bool t_php_generator::needs_php_read_validator(t_struct* tstruct) { + return (validate_ && + (get_php_num_required_fields(tstruct->get_members(), false) > 0)); } /** @@ -2577,5 +2683,6 @@ THRIFT_REGISTER_GENERATOR(php, "PHP", " oop: Generate PHP with object oriented subclasses\n" " rest: Generate PHP REST processors\n" " nsglobal=NAME: Set global namespace\n" +" validate: Generate PHP validator methods\n" ) diff --git a/lib/php/test/Makefile.am b/lib/php/test/Makefile.am index 1292b818a..a529d8c28 100755 --- a/lib/php/test/Makefile.am +++ b/lib/php/test/Makefile.am @@ -19,15 +19,27 @@ THRIFT = $(top_builddir)/compiler/cpp/thrift -stubs: ../../../test/ThriftTest.thrift +stubs: ../../../test/ThriftTest.thrift TestValidators.thrift mkdir -p ./packages $(THRIFT) --gen php -r --out ./packages ../../../test/ThriftTest.thrift + mkdir -p ./packages/phpv + mkdir -p ./packages/phpvo + $(THRIFT) --gen php:validate -r --out ./packages/phpv TestValidators.thrift + $(THRIFT) --gen php:validate,oop -r --out ./packages/phpvo TestValidators.thrift + +check-validator: stubs + php Test/Thrift/TestValidators.php + php Test/Thrift/TestValidators.php -oop +check-protocol: stubs if HAVE_PHPUNIT -check: stubs $(PHPUNIT) --log-junit=phpunit.xml Test/Thrift/Protocol/TestTJSONProtocol.php $(PHPUNIT) --log-junit=phpunit.xml Test/Thrift/Protocol/TestBinarySerializer.php endif + +check: stubs \ + check-protocol \ + check-validator clean-local: $(RM) -r ./packages diff --git a/lib/php/test/Test/Thrift/TestValidators.php b/lib/php/test/Test/Thrift/TestValidators.php new file mode 100644 index 000000000..41e95fb25 --- /dev/null +++ b/lib/php/test/Test/Thrift/TestValidators.php @@ -0,0 +1,142 @@ +registerNamespace('Thrift', __DIR__ . '/../../../lib'); +$loader->registerDefinition('ThriftTest', __DIR__ . '/../../packages/' . $GEN_DIR); +$loader->registerDefinition('TestValidators', __DIR__ . '/../../packages/' . $GEN_DIR); +$loader->register(); + +// Would be nice to have PHPUnit here, but for now just hack it. + +set_exception_handler(function($e) { + my_assert(false, "Unexpected exception caught: " . $e->getMessage()); +}); + +set_error_handler(function($errno, $errmsg) { + my_assert(false, "Unexpected PHP error: " . $errmsg); +}); + +// Empty structs should not have validators +assert_has_no_read_validator('ThriftTest\EmptyStruct'); +assert_has_no_write_validator('ThriftTest\EmptyStruct'); + +// Bonk has only opt_in_req_out fields +{ + assert_has_no_read_validator('ThriftTest\Bonk'); + assert_has_a_write_validator('ThriftTest\Bonk'); + { + // Check that we can read an empty object + $bonk = new \ThriftTest\Bonk(); + $transport = new TMemoryBuffer("\000"); + $protocol = new TBinaryProtocol($transport); + $bonk->read($protocol); + } + { + // ...but not write an empty object + $bonk = new \ThriftTest\Bonk(); + $transport = new TMemoryBuffer(); + $protocol = new TBinaryProtocol($transport); + assert_protocol_exception_thrown(function() use ($bonk, $protocol) { $bonk->write($protocol); }, + 'Bonk was able to write an empty object'); + } +} + +// StructA has a single required field +{ + assert_has_a_read_validator('ThriftTest\StructA'); + assert_has_a_write_validator('ThriftTest\StructA'); + { + // Check that we are not able to write StructA with a missing required field + $structa = new \ThriftTest\StructA(); + $transport = new TMemoryBuffer(); + $protocol = new TBinaryProtocol($transport); + assert_protocol_exception_thrown(function() use ($structa, $protocol) { $structa->write($protocol); }, + 'StructA was able to write an empty object'); + } + { + // Check that we are able to read and write a message with a good StructA + $transport = new TMemoryBuffer(base64_decode('CwABAAAAA2FiYwA=')); + $protocol = new TBinaryProtocol($transport); + $structa = new \ThriftTest\StructA(); + $structa->read($protocol); + $structa->write($protocol); + } +} + +// Unions should not get write validators +assert_has_no_write_validator('TestValidators\UnionOfStrings'); + +function assert_has_a_read_validator($class) { + my_assert(has_read_validator_method($class), + $class . ' class should have a read validator'); +} + +function assert_has_no_read_validator($class) { + my_assert(!has_read_validator_method($class), + $class . ' class should not have a read validator'); +} + +function assert_has_a_write_validator($class) { + my_assert(has_write_validator_method($class), + $class . ' class should have a write validator'); +} + +function assert_has_no_write_validator($class) { + my_assert(!has_write_validator_method($class), + $class . ' class should not have a write validator'); +} + +function assert_protocol_exception_thrown($callable, $message) { + try { + call_user_func($callable); + my_assert(false, $message); + } catch (TProtocolException $e) { + } +} + +function has_write_validator_method($class) { + $rc = new \ReflectionClass($class); + return $rc->hasMethod('_validateForWrite'); +} + +function has_read_validator_method($class) { + $rc = new \ReflectionClass($class); + return $rc->hasMethod('_validateForRead'); +} + +function my_assert($something, $message) { + if (!$something) { + fwrite(STDERR, basename(__FILE__) . " FAILED: $message\n"); + exit(1); + } +} diff --git a/lib/php/test/TestValidators.thrift b/lib/php/test/TestValidators.thrift new file mode 100644 index 000000000..f994a9e57 --- /dev/null +++ b/lib/php/test/TestValidators.thrift @@ -0,0 +1,28 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +namespace php TestValidators + +include "../../../test/ThriftTest.thrift" + +union UnionOfStrings { + 1: string aa; + 2: string bb; +} +