From 2da86a36a5cb6e6963a8f960eb53a90f8533eaa4 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Thu, 2 Jan 2020 22:50:30 -0800 Subject: [PATCH 1/2] Remove the OCI8Connection::getExecuteMode() method The existing relationship between the connection and its statement violate the ISP principle. Instead of having access only to the execution mode of the connection, statements have access to the entire connection API. Having a method which is not defined in the driver connection interface makes it impossible to mark the method `final` (#3590). --- src/Driver/OCI8/ExecutionMode.php | 31 +++++++++++++++++++++++ src/Driver/OCI8/OCI8Connection.php | 26 ++++++------------- src/Driver/OCI8/OCI8Statement.php | 24 ++++++++++++------ tests/Driver/OCI8/ExecutionModeTest.php | 33 +++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 26 deletions(-) create mode 100644 src/Driver/OCI8/ExecutionMode.php create mode 100644 tests/Driver/OCI8/ExecutionModeTest.php diff --git a/src/Driver/OCI8/ExecutionMode.php b/src/Driver/OCI8/ExecutionMode.php new file mode 100644 index 00000000000..88c00216fda --- /dev/null +++ b/src/Driver/OCI8/ExecutionMode.php @@ -0,0 +1,31 @@ +isAutoCommitEnabled = true; + } + + public function disableAutoCommit(): void + { + $this->isAutoCommitEnabled = false; + } + + public function isAutoCommitEnabled(): bool + { + return $this->isAutoCommitEnabled; + } +} diff --git a/src/Driver/OCI8/OCI8Connection.php b/src/Driver/OCI8/OCI8Connection.php index c3022e180e7..23140a39ee8 100644 --- a/src/Driver/OCI8/OCI8Connection.php +++ b/src/Driver/OCI8/OCI8Connection.php @@ -23,7 +23,6 @@ use function sprintf; use function str_replace; -use const OCI_COMMIT_ON_SUCCESS; use const OCI_NO_AUTO_COMMIT; /** @@ -36,8 +35,8 @@ class OCI8Connection implements ConnectionInterface, ServerInfoAwareConnection /** @var resource */ protected $dbh; - /** @var int */ - protected $executeMode = OCI_COMMIT_ON_SUCCESS; + /** @var ExecutionMode */ + private $executionMode; /** * Creates a Connection to an Oracle Database using oci8 extension. @@ -67,7 +66,8 @@ public function __construct( throw OCI8Exception::fromErrorInfo(oci_error()); } - $this->dbh = $dbh; + $this->dbh = $dbh; + $this->executionMode = new ExecutionMode(); } /** @@ -107,7 +107,7 @@ public function requiresQueryForServerVersion() public function prepare(string $sql): DriverStatement { - return new Statement($this->dbh, $sql, $this); + return new Statement($this->dbh, $sql, $this->executionMode); } public function query(string $sql): ResultInterface @@ -154,22 +154,12 @@ public function lastInsertId($name = null) return (int) $result; } - /** - * Returns the current execution mode. - * - * @return int - */ - public function getExecuteMode() - { - return $this->executeMode; - } - /** * {@inheritdoc} */ public function beginTransaction() { - $this->executeMode = OCI_NO_AUTO_COMMIT; + $this->executionMode->disableAutoCommit(); return true; } @@ -183,7 +173,7 @@ public function commit() throw OCI8Exception::fromErrorInfo(oci_error($this->dbh)); } - $this->executeMode = OCI_COMMIT_ON_SUCCESS; + $this->executionMode->enableAutoCommit(); return true; } @@ -197,7 +187,7 @@ public function rollBack() throw OCI8Exception::fromErrorInfo(oci_error($this->dbh)); } - $this->executeMode = OCI_COMMIT_ON_SUCCESS; + $this->executionMode->enableAutoCommit(); return true; } diff --git a/src/Driver/OCI8/OCI8Statement.php b/src/Driver/OCI8/OCI8Statement.php index 180629667e5..2c0c31e3536 100644 --- a/src/Driver/OCI8/OCI8Statement.php +++ b/src/Driver/OCI8/OCI8Statement.php @@ -24,7 +24,9 @@ use const OCI_B_BIN; use const OCI_B_BLOB; +use const OCI_COMMIT_ON_SUCCESS; use const OCI_D_LOB; +use const OCI_NO_AUTO_COMMIT; use const OCI_TEMP_BLOB; use const PREG_OFFSET_CAPTURE; use const SQLT_CHR; @@ -42,8 +44,8 @@ class OCI8Statement implements StatementInterface /** @var resource */ protected $_sth; - /** @var OCI8Connection */ - protected $_conn; + /** @var ExecutionMode */ + private $executionMode; /** @var string[] */ protected $_paramMap = []; @@ -63,17 +65,17 @@ class OCI8Statement implements StatementInterface * @param resource $dbh The connection handle. * @param string $query The SQL query. */ - public function __construct($dbh, $query, OCI8Connection $conn) + public function __construct($dbh, $query, ExecutionMode $executionMode) { [$query, $paramMap] = self::convertPositionalToNamedPlaceholders($query); $stmt = oci_parse($dbh, $query); assert(is_resource($stmt)); - $this->_sth = $stmt; - $this->_dbh = $dbh; - $this->_paramMap = $paramMap; - $this->_conn = $conn; + $this->_sth = $stmt; + $this->_dbh = $dbh; + $this->_paramMap = $paramMap; + $this->executionMode = $executionMode; } /** @@ -299,7 +301,13 @@ public function execute($params = null): ResultInterface } } - $ret = @oci_execute($this->_sth, $this->_conn->getExecuteMode()); + if ($this->executionMode->isAutoCommitEnabled()) { + $mode = OCI_COMMIT_ON_SUCCESS; + } else { + $mode = OCI_NO_AUTO_COMMIT; + } + + $ret = @oci_execute($this->_sth, $mode); if (! $ret) { throw OCI8Exception::fromErrorInfo(oci_error($this->_sth)); } diff --git a/tests/Driver/OCI8/ExecutionModeTest.php b/tests/Driver/OCI8/ExecutionModeTest.php new file mode 100644 index 00000000000..ab041e0fb9b --- /dev/null +++ b/tests/Driver/OCI8/ExecutionModeTest.php @@ -0,0 +1,33 @@ +mode = new ExecutionMode(); + } + + public function testDefaultAutoCommitStatus(): void + { + self::assertTrue($this->mode->isAutoCommitEnabled()); + } + + public function testChangeAutoCommitStatus(): void + { + $this->mode->disableAutoCommit(); + self::assertFalse($this->mode->isAutoCommitEnabled()); + + $this->mode->enableAutoCommit(); + self::assertTrue($this->mode->isAutoCommitEnabled()); + } +} From 2dfe0afd7cd4d5755426a8b999cb431ec15fceea Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sun, 12 Jan 2020 11:52:47 -0800 Subject: [PATCH 2/2] Extract conversion of positional to named placeholder to an utility class --- psalm.xml | 2 +- .../ConvertPositionalToNamedPlaceholders.php | 162 ++++++++++++++++++ src/Driver/OCI8/OCI8Statement.php | 157 +---------------- ...vertPositionalToNamedPlaceholdersTest.php} | 64 +++++-- tests/Driver/OCI8/OCI8StatementTest.php | 52 ------ 5 files changed, 217 insertions(+), 220 deletions(-) create mode 100644 src/Driver/OCI8/ConvertPositionalToNamedPlaceholders.php rename tests/{UtilTest.php => Driver/OCI8/ConvertPositionalToNamedPlaceholdersTest.php} (60%) delete mode 100644 tests/Driver/OCI8/OCI8StatementTest.php diff --git a/psalm.xml b/psalm.xml index 7aafe8fe5d4..7628f4f0a65 100644 --- a/psalm.xml +++ b/psalm.xml @@ -29,7 +29,7 @@ This one is just too convoluted for Psalm to figure out, by its author's own admission --> - + diff --git a/src/Driver/OCI8/ConvertPositionalToNamedPlaceholders.php b/src/Driver/OCI8/ConvertPositionalToNamedPlaceholders.php new file mode 100644 index 00000000000..947c041a6c4 --- /dev/null +++ b/src/Driver/OCI8/ConvertPositionalToNamedPlaceholders.php @@ -0,0 +1,162 @@ +). + * + * Oracle does not support positional parameters, hence this method converts all + * positional parameters into artificially named parameters. Note that this conversion + * is not perfect. All question marks (?) in the original statement are treated as + * placeholders and converted to a named parameter. + * + * @internal This class is not covered by the backward compatibility promise + */ +final class ConvertPositionalToNamedPlaceholders +{ + /** + * @param string $statement The SQL statement to convert. + * + * @return mixed[] [0] => the statement value (string), [1] => the paramMap value (array). + * + * @throws OCI8Exception + */ + public function __invoke(string $statement): array + { + $fragmentOffset = $tokenOffset = 0; + $fragments = $paramMap = []; + $currentLiteralDelimiter = null; + + do { + if ($currentLiteralDelimiter === null) { + $result = $this->findPlaceholderOrOpeningQuote( + $statement, + $tokenOffset, + $fragmentOffset, + $fragments, + $currentLiteralDelimiter, + $paramMap + ); + } else { + $result = $this->findClosingQuote($statement, $tokenOffset, $currentLiteralDelimiter); + } + } while ($result); + + if ($currentLiteralDelimiter !== null) { + throw NonTerminatedStringLiteral::new($tokenOffset - 1); + } + + $fragments[] = substr($statement, $fragmentOffset); + $statement = implode('', $fragments); + + return [$statement, $paramMap]; + } + + /** + * Finds next placeholder or opening quote. + * + * @param string $statement The SQL statement to parse + * @param int $tokenOffset The offset to start searching from + * @param int $fragmentOffset The offset to build the next fragment from + * @param string[] $fragments Fragments of the original statement not containing placeholders + * @param string|null $currentLiteralDelimiter The delimiter of the current string literal + * or NULL if not currently in a literal + * @param string[] $paramMap Mapping of the original parameter positions to their named replacements + * + * @return bool Whether the token was found + */ + private function findPlaceholderOrOpeningQuote( + string $statement, + int &$tokenOffset, + int &$fragmentOffset, + array &$fragments, + ?string &$currentLiteralDelimiter, + array &$paramMap + ): bool { + $token = $this->findToken($statement, $tokenOffset, '/[?\'"]/'); + + if ($token === null) { + return false; + } + + if ($token === '?') { + $position = count($paramMap) + 1; + $param = ':param' . $position; + $fragments[] = substr($statement, $fragmentOffset, $tokenOffset - $fragmentOffset); + $fragments[] = $param; + $paramMap[$position] = $param; + $tokenOffset += 1; + $fragmentOffset = $tokenOffset; + + return true; + } + + $currentLiteralDelimiter = $token; + ++$tokenOffset; + + return true; + } + + /** + * Finds closing quote + * + * @param string $statement The SQL statement to parse + * @param int $tokenOffset The offset to start searching from + * @param string $currentLiteralDelimiter The delimiter of the current string literal + * + * @return bool Whether the token was found + */ + private function findClosingQuote( + string $statement, + int &$tokenOffset, + string &$currentLiteralDelimiter + ): bool { + $token = $this->findToken( + $statement, + $tokenOffset, + '/' . preg_quote($currentLiteralDelimiter, '/') . '/' + ); + + if ($token === null) { + return false; + } + + $currentLiteralDelimiter = null; + ++$tokenOffset; + + return true; + } + + /** + * Finds the token described by regex starting from the given offset. Updates the offset with the position + * where the token was found. + * + * @param string $statement The SQL statement to parse + * @param int $offset The offset to start searching from + * @param string $regex The regex containing token pattern + * + * @return string|null Token or NULL if not found + */ + private function findToken(string $statement, int &$offset, string $regex): ?string + { + if (preg_match($regex, $statement, $matches, PREG_OFFSET_CAPTURE, $offset) === 1) { + $offset = $matches[0][1]; + + return $matches[0][0]; + } + + return null; + } +} diff --git a/src/Driver/OCI8/OCI8Statement.php b/src/Driver/OCI8/OCI8Statement.php index 2c0c31e3536..6ef87d448e7 100644 --- a/src/Driver/OCI8/OCI8Statement.php +++ b/src/Driver/OCI8/OCI8Statement.php @@ -2,15 +2,12 @@ namespace Doctrine\DBAL\Driver\OCI8; -use Doctrine\DBAL\Driver\OCI8\Exception\NonTerminatedStringLiteral; use Doctrine\DBAL\Driver\OCI8\Exception\UnknownParameterIndex; use Doctrine\DBAL\Driver\Result as ResultInterface; use Doctrine\DBAL\Driver\Statement as StatementInterface; use Doctrine\DBAL\ParameterType; use function assert; -use function count; -use function implode; use function is_int; use function is_resource; use function oci_bind_by_name; @@ -18,9 +15,6 @@ use function oci_execute; use function oci_new_descriptor; use function oci_parse; -use function preg_match; -use function preg_quote; -use function substr; use const OCI_B_BIN; use const OCI_B_BLOB; @@ -28,7 +22,6 @@ use const OCI_D_LOB; use const OCI_NO_AUTO_COMMIT; use const OCI_TEMP_BLOB; -use const PREG_OFFSET_CAPTURE; use const SQLT_CHR; /** @@ -67,7 +60,7 @@ class OCI8Statement implements StatementInterface */ public function __construct($dbh, $query, ExecutionMode $executionMode) { - [$query, $paramMap] = self::convertPositionalToNamedPlaceholders($query); + [$query, $paramMap] = (new ConvertPositionalToNamedPlaceholders())($query); $stmt = oci_parse($dbh, $query); assert(is_resource($stmt)); @@ -78,154 +71,6 @@ public function __construct($dbh, $query, ExecutionMode $executionMode) $this->executionMode = $executionMode; } - /** - * Converts positional (?) into named placeholders (:param). - * - * Oracle does not support positional parameters, hence this method converts all - * positional parameters into artificially named parameters. Note that this conversion - * is not perfect. All question marks (?) in the original statement are treated as - * placeholders and converted to a named parameter. - * - * The algorithm uses a state machine with two possible states: InLiteral and NotInLiteral. - * Question marks inside literal strings are therefore handled correctly by this method. - * This comes at a cost, the whole sql statement has to be looped over. - * - * @param string $statement The SQL statement to convert. - * - * @return mixed[] [0] => the statement value (string), [1] => the paramMap value (array). - * - * @throws OCI8Exception - * - * @todo extract into utility class in Doctrine\DBAL\Util namespace - * @todo review and test for lost spaces. we experienced missing spaces with oci8 in some sql statements. - */ - public static function convertPositionalToNamedPlaceholders($statement) - { - $fragmentOffset = $tokenOffset = 0; - $fragments = $paramMap = []; - $currentLiteralDelimiter = null; - - do { - if (! $currentLiteralDelimiter) { - $result = self::findPlaceholderOrOpeningQuote( - $statement, - $tokenOffset, - $fragmentOffset, - $fragments, - $currentLiteralDelimiter, - $paramMap - ); - } else { - $result = self::findClosingQuote($statement, $tokenOffset, $currentLiteralDelimiter); - } - } while ($result); - - if ($currentLiteralDelimiter) { - throw NonTerminatedStringLiteral::new($tokenOffset - 1); - } - - $fragments[] = substr($statement, $fragmentOffset); - $statement = implode('', $fragments); - - return [$statement, $paramMap]; - } - - /** - * Finds next placeholder or opening quote. - * - * @param string $statement The SQL statement to parse - * @param string $tokenOffset The offset to start searching from - * @param int $fragmentOffset The offset to build the next fragment from - * @param string[] $fragments Fragments of the original statement not containing placeholders - * @param string|null $currentLiteralDelimiter The delimiter of the current string literal - * or NULL if not currently in a literal - * @param array $paramMap Mapping of the original parameter positions to their named replacements - * - * @return bool Whether the token was found - */ - private static function findPlaceholderOrOpeningQuote( - $statement, - &$tokenOffset, - &$fragmentOffset, - &$fragments, - &$currentLiteralDelimiter, - &$paramMap - ) { - $token = self::findToken($statement, $tokenOffset, '/[?\'"]/'); - - if ($token === null) { - return false; - } - - if ($token === '?') { - $position = count($paramMap) + 1; - $param = ':param' . $position; - $fragments[] = substr($statement, $fragmentOffset, $tokenOffset - $fragmentOffset); - $fragments[] = $param; - $paramMap[$position] = $param; - $tokenOffset += 1; - $fragmentOffset = $tokenOffset; - - return true; - } - - $currentLiteralDelimiter = $token; - ++$tokenOffset; - - return true; - } - - /** - * Finds closing quote - * - * @param string $statement The SQL statement to parse - * @param string $tokenOffset The offset to start searching from - * @param string $currentLiteralDelimiter The delimiter of the current string literal - * - * @return bool Whether the token was found - */ - private static function findClosingQuote( - $statement, - &$tokenOffset, - &$currentLiteralDelimiter - ) { - $token = self::findToken( - $statement, - $tokenOffset, - '/' . preg_quote($currentLiteralDelimiter, '/') . '/' - ); - - if ($token === null) { - return false; - } - - $currentLiteralDelimiter = false; - ++$tokenOffset; - - return true; - } - - /** - * Finds the token described by regex starting from the given offset. Updates the offset with the position - * where the token was found. - * - * @param string $statement The SQL statement to parse - * @param int $offset The offset to start searching from - * @param string $regex The regex containing token pattern - * - * @return string|null Token or NULL if not found - */ - private static function findToken($statement, &$offset, $regex) - { - if (preg_match($regex, $statement, $matches, PREG_OFFSET_CAPTURE, $offset) === 1) { - $offset = $matches[0][1]; - - return $matches[0][0]; - } - - return null; - } - /** * {@inheritdoc} */ diff --git a/tests/UtilTest.php b/tests/Driver/OCI8/ConvertPositionalToNamedPlaceholdersTest.php similarity index 60% rename from tests/UtilTest.php rename to tests/Driver/OCI8/ConvertPositionalToNamedPlaceholdersTest.php index 054caeada17..51323932d55 100644 --- a/tests/UtilTest.php +++ b/tests/Driver/OCI8/ConvertPositionalToNamedPlaceholdersTest.php @@ -1,16 +1,40 @@ convertPositionalToNamedPlaceholders = new ConvertPositionalToNamedPlaceholders(); + } + + /** + * @param mixed[] $expectedOutputParamsMap + * + * @dataProvider positionalToNamedPlaceholdersProvider + */ + public function testConvertPositionalToNamedParameters(string $inputSQL, string $expectedOutputSQL, array $expectedOutputParamsMap): void + { + [$statement, $params] = ($this->convertPositionalToNamedPlaceholders)($inputSQL); + + self::assertEquals($expectedOutputSQL, $statement); + self::assertEquals($expectedOutputParamsMap, $params); + } + /** * @return mixed[][] */ - public static function dataConvertPositionalToNamedParameters(): iterable + public static function positionalToNamedPlaceholdersProvider(): iterable { return [ [ @@ -67,15 +91,33 @@ public static function dataConvertPositionalToNamedParameters(): iterable } /** - * @param mixed[] $expectedOutputParamsMap - * - * @dataProvider dataConvertPositionalToNamedParameters + * @dataProvider nonTerminatedLiteralProvider */ - public function testConvertPositionalToNamedParameters(string $inputSQL, string $expectedOutputSQL, array $expectedOutputParamsMap): void + public function testConvertNonTerminatedLiteral(string $sql, string $expectedExceptionMessageRegExp): void { - [$statement, $params] = Statement::convertPositionalToNamedPlaceholders($inputSQL); + $this->expectException(OCI8Exception::class); + $this->expectExceptionMessageMatches($expectedExceptionMessageRegExp); + ($this->convertPositionalToNamedPlaceholders)($sql); + } - self::assertEquals($expectedOutputSQL, $statement); - self::assertEquals($expectedOutputParamsMap, $params); + /** + * @return array> + */ + public static function nonTerminatedLiteralProvider(): iterable + { + return [ + 'no-matching-quote' => [ + "SELECT 'literal FROM DUAL", + '/offset 7./', + ], + 'no-matching-double-quote' => [ + 'SELECT 1 "COL1 FROM DUAL', + '/offset 9./', + ], + 'incorrect-escaping-syntax' => [ + "SELECT 'quoted \\'string' FROM DUAL", + '/offset 23./', + ], + ]; } } diff --git a/tests/Driver/OCI8/OCI8StatementTest.php b/tests/Driver/OCI8/OCI8StatementTest.php deleted file mode 100644 index 77466f37f8b..00000000000 --- a/tests/Driver/OCI8/OCI8StatementTest.php +++ /dev/null @@ -1,52 +0,0 @@ -markTestSkipped('oci8 is not installed.'); - } - - parent::setUp(); - } - - /** - * @dataProvider nonTerminatedLiteralProvider - */ - public function testConvertNonTerminatedLiteral(string $sql, string $message): void - { - $this->expectException(OCI8Exception::class); - $this->expectExceptionMessageMatches($message); - OCI8Statement::convertPositionalToNamedPlaceholders($sql); - } - - /** - * @return array> - */ - public static function nonTerminatedLiteralProvider(): iterable - { - return [ - 'no-matching-quote' => [ - "SELECT 'literal FROM DUAL", - '/offset 7/', - ], - 'no-matching-double-quote' => [ - 'SELECT 1 "COL1 FROM DUAL', - '/offset 9/', - ], - 'incorrect-escaping-syntax' => [ - "SELECT 'quoted \\'string' FROM DUAL", - '/offset 23/', - ], - ]; - } -}