From 20fc31c8c9887df2298631de57b3cbfad5bb99c3 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Mon, 7 Dec 2020 21:05:35 -0500 Subject: [PATCH 1/4] "Normalize" the search_path parsing behavior Primarily, these changes "normalize" the search_path parsing behavior such that the new parseSearchPath() method returns the same result whether the search_path input is an array (with one or more schemas), a string with one schema, or a string of comma-separated schemas. This method's presence makes it much simpler to retrieve the search_path as configured on the connection and use it for more complex schema-related grammar construction. --- .../Database/Connectors/PostgresConnector.php | 33 ++++++++++++++----- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/Illuminate/Database/Connectors/PostgresConnector.php b/src/Illuminate/Database/Connectors/PostgresConnector.php index 113039f763c7..f02f414ef5de 100755 --- a/src/Illuminate/Database/Connectors/PostgresConnector.php +++ b/src/Illuminate/Database/Connectors/PostgresConnector.php @@ -94,12 +94,35 @@ protected function configureTimezone($connection, array $config) protected function configureSearchPath($connection, $config) { if (isset($config['search_path'])) { - $searchPath = $this->formatSearchPath($config['search_path']); + $searchPath = $this->formatSearchPath( + $this->parseSearchPath($config['search_path']) + ); $connection->prepare("set search_path to {$searchPath}")->execute(); } } + /** + * Parse the search_path. + * + * @param string|array $searchPath + * @return array + */ + protected function parseSearchPath($searchPath) + { + if (is_string($searchPath)) { + preg_match_all('/[a-zA-z0-9$]{1,}/i', $searchPath, $matches); + + $searchPath = $matches[0]; + } + + array_walk($searchPath, function(&$schema) { + $schema = trim($schema, '\'"'); + }); + + return $searchPath; + } + /** * Format the search path for the DSN. * @@ -108,13 +131,7 @@ protected function configureSearchPath($connection, $config) */ protected function formatSearchPath($searchPath) { - if (is_array($searchPath)) { - $searchPath = '"'.implode('", "', $searchPath).'"'; - } - - preg_match_all('/[a-zA-z0-9$]{1,}/i', $searchPath, $matches); - - return '"'.implode('", "', $matches[0]).'"'; + return count($searchPath) === 1 ? '"' . $searchPath[0] . '"' : '"'.implode('", "', $searchPath).'"'; } /** From 76933a32c3fb36335cabf194e534b12d919ccf47 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Mon, 7 Dec 2020 21:17:10 -0500 Subject: [PATCH 2/4] Fix PostgreSQL object reference parsing The manner in which object references were parsed in certain scenarios caused methods such as hasTable() to return incorrect results. Among other issues, the underlying SQL grammar omitted the "table_catalog" (i.e., database) in the WHERE clause, which caused inaccurate results in certain cases, e.g., the method returned true incorrectly because a schema and table with the same name exist in a *different database*. --- .../Schema/Grammars/PostgresGrammar.php | 4 +- .../Database/Schema/PostgresBuilder.php | 70 +++++++++++++++---- 2 files changed, 58 insertions(+), 16 deletions(-) diff --git a/src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php b/src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php index 204beecea3b2..f6450c1b41c9 100755 --- a/src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php +++ b/src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php @@ -42,7 +42,7 @@ class PostgresGrammar extends Grammar */ public function compileTableExists() { - return "select * from information_schema.tables where table_schema = ? and table_name = ? and table_type = 'BASE TABLE'"; + return "select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'"; } /** @@ -52,7 +52,7 @@ public function compileTableExists() */ public function compileColumnListing() { - return 'select column_name from information_schema.columns where table_schema = ? and table_name = ?'; + return 'select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?'; } /** diff --git a/src/Illuminate/Database/Schema/PostgresBuilder.php b/src/Illuminate/Database/Schema/PostgresBuilder.php index 9d10e9077ec9..78fc744b1c43 100755 --- a/src/Illuminate/Database/Schema/PostgresBuilder.php +++ b/src/Illuminate/Database/Schema/PostgresBuilder.php @@ -12,12 +12,12 @@ class PostgresBuilder extends Builder */ public function hasTable($table) { - [$schema, $table] = $this->parseSchemaAndTable($table); + [$database, $schema, $table] = $this->parseSchemaAndTable($table); $table = $this->connection->getTablePrefix().$table; return count($this->connection->select( - $this->grammar->compileTableExists(), [$schema, $table] + $this->grammar->compileTableExists(), [$database, $schema, $table] )) > 0; } @@ -143,35 +143,77 @@ public function getAllTypes() */ public function getColumnListing($table) { - [$schema, $table] = $this->parseSchemaAndTable($table); + [$database, $schema, $table] = $this->parseSchemaAndTable($table); $table = $this->connection->getTablePrefix().$table; $results = $this->connection->select( - $this->grammar->compileColumnListing(), [$schema, $table] + $this->grammar->compileColumnListing(), [$database, $schema, $table] ); return $this->connection->getPostProcessor()->processColumnListing($results); } /** - * Parse the table name and extract the schema and table. + * Parse the search_path. * - * @param string $table + * @param string|array $searchPath * @return array */ - protected function parseSchemaAndTable($table) + protected function parseSearchPath($searchPath) { - $table = explode('.', $table); + if (is_string($searchPath)) { + preg_match_all('/[a-zA-z0-9$]{1,}/i', $searchPath, $matches); - if (is_array($searchPath = $this->connection->getConfig('search_path'))) { - if (in_array($table[0], $searchPath)) { - return [array_shift($table), implode('.', $table)]; - } + $searchPath = $matches[0]; + } + + array_walk($searchPath, function(&$schema) { + $schema = trim($schema, '\'"'); + }); + + return $searchPath; + } + + /** + * Parse the database object reference and extract the database, schema, and + * table. + * + * @param string $reference + * @return array + */ + protected function parseSchemaAndTable($reference) + { + $searchPath = $this->parseSearchPath( + $this->connection->getConfig('search_path') ?: 'public' + ); + + $parts = explode('.', $reference); + + // Use the connection's configured database by default. + + $database = $this->connection->getConfig('database'); + + // If the reference contains a database name, use that instead. + + if (count($parts) === 3) { + $database = $parts[0]; + + array_shift($parts); + } + + // Use the first schema in the search_path by default. + + $schema = $searchPath[0]; + + // If the reference contains a schema, use that instead. + + if (count($parts) === 2) { + $schema = $parts[0]; - $schema = head($searchPath); + array_shift($parts); } - return [$schema ?: 'public', implode('.', $table)]; + return [$database, $schema, $parts[0]]; } } From c630b8ec7c50725d45eaaddb1bd1d45370fa4a85 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Mon, 7 Dec 2020 22:31:15 -0500 Subject: [PATCH 3/4] Apply Style-CI fixes --- src/Illuminate/Database/Connectors/PostgresConnector.php | 4 ++-- src/Illuminate/Database/Schema/PostgresBuilder.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Illuminate/Database/Connectors/PostgresConnector.php b/src/Illuminate/Database/Connectors/PostgresConnector.php index f02f414ef5de..d74a00dfc7b5 100755 --- a/src/Illuminate/Database/Connectors/PostgresConnector.php +++ b/src/Illuminate/Database/Connectors/PostgresConnector.php @@ -116,7 +116,7 @@ protected function parseSearchPath($searchPath) $searchPath = $matches[0]; } - array_walk($searchPath, function(&$schema) { + array_walk($searchPath, function (&$schema) { $schema = trim($schema, '\'"'); }); @@ -131,7 +131,7 @@ protected function parseSearchPath($searchPath) */ protected function formatSearchPath($searchPath) { - return count($searchPath) === 1 ? '"' . $searchPath[0] . '"' : '"'.implode('", "', $searchPath).'"'; + return count($searchPath) === 1 ? '"'.$searchPath[0].'"' : '"'.implode('", "', $searchPath).'"'; } /** diff --git a/src/Illuminate/Database/Schema/PostgresBuilder.php b/src/Illuminate/Database/Schema/PostgresBuilder.php index 78fc744b1c43..c95423166928 100755 --- a/src/Illuminate/Database/Schema/PostgresBuilder.php +++ b/src/Illuminate/Database/Schema/PostgresBuilder.php @@ -168,7 +168,7 @@ protected function parseSearchPath($searchPath) $searchPath = $matches[0]; } - array_walk($searchPath, function(&$schema) { + array_walk($searchPath, function (&$schema) { $schema = trim($schema, '\'"'); }); From 7128e69832141b86271c65c27bc4f1c0e99af443 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Tue, 8 Dec 2020 17:07:49 -0500 Subject: [PATCH 4/4] Add tests to cover any added or modified code --- .../Database/DatabasePostgresBuilderTest.php | 209 ++++++++++++++++++ .../DatabasePostgresSchemaGrammarTest.php | 14 ++ 2 files changed, 223 insertions(+) create mode 100644 tests/Database/DatabasePostgresBuilderTest.php diff --git a/tests/Database/DatabasePostgresBuilderTest.php b/tests/Database/DatabasePostgresBuilderTest.php new file mode 100644 index 000000000000..e20e24d1213f --- /dev/null +++ b/tests/Database/DatabasePostgresBuilderTest.php @@ -0,0 +1,209 @@ +getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn(null); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileTableExists')->andReturn("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'"); + $connection->shouldReceive('select')->with("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'", ['laravel', 'public', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $builder = $this->getBuilder($connection); + + $builder->hasTable('foo'); + } + + /** + * Ensure that when the reference is unqualified (i.e., does not contain a + * database name or a schema), and the first schema in the search_path is + * NOT the default ('public'), the database specified on the connection is + * used, and the first schema in the search_path is used. + */ + public function testWhenSearchPathNotEmptyHasTableWithUnqualifiedSchemaReferenceIsCorrect() + { + $connection = $this->getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn('myapp,public'); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileTableExists')->andReturn("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'"); + $connection->shouldReceive('select')->with("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'", ['laravel', 'myapp', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $builder = $this->getBuilder($connection); + + $builder->hasTable('foo'); + } + + /** + * Ensure that when the reference is qualified only with a schema, that + * the database specified on the connection is used, and the specified + * schema is used, even if it is not within the search_path. + */ + public function testWhenSchemaNotInSearchPathHasTableWithQualifiedSchemaReferenceIsCorrect() + { + $connection = $this->getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn('public'); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileTableExists')->andReturn("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'"); + $connection->shouldReceive('select')->with("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'", ['laravel', 'myapp', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $builder = $this->getBuilder($connection); + + $builder->hasTable('myapp.foo'); + } + + /** + * Ensure that when the reference is qualified with a database AND a schema, + * and the database is NOT the database configured for the connection, the + * specified database is used instead. + */ + public function testWhenDatabaseNotDefaultHasTableWithFullyQualifiedReferenceIsCorrect() + { + $connection = $this->getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn('public'); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileTableExists')->andReturn("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'"); + $connection->shouldReceive('select')->with("select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'", ['mydatabase', 'myapp', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $builder = $this->getBuilder($connection); + + $builder->hasTable('mydatabase.myapp.foo'); + } + + /** + * Ensure that when the reference is unqualified (i.e., does not contain a + * database name or a schema), and the search_path is empty, the database + * specified on the connection is used, and the default schema ('public') + * is used. + */ + public function testWhenSearchPathEmptyGetColumnListingWithUnqualifiedReferenceIsCorrect() + { + $connection = $this->getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn(null); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileColumnListing')->andReturn('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?'); + $connection->shouldReceive('select')->with('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?', ['laravel', 'public', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $processor = m::mock(PostgresProcessor::class); + $connection->shouldReceive('getPostProcessor')->andReturn($processor); + $processor->shouldReceive('processColumnListing')->andReturn(['some_column']); + $builder = $this->getBuilder($connection); + + $builder->getColumnListing('foo'); + } + + /** + * Ensure that when the reference is unqualified (i.e., does not contain a + * database name or a schema), and the first schema in the search_path is + * NOT the default ('public'), the database specified on the connection is + * used, and the first schema in the search_path is used. + */ + public function testWhenSearchPathNotEmptyGetColumnListingWithUnqualifiedSchemaReferenceIsCorrect() + { + $connection = $this->getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn('myapp,public'); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileColumnListing')->andReturn('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?'); + $connection->shouldReceive('select')->with('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?', ['laravel', 'myapp', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $processor = m::mock(PostgresProcessor::class); + $connection->shouldReceive('getPostProcessor')->andReturn($processor); + $processor->shouldReceive('processColumnListing')->andReturn(['some_column']); + $builder = $this->getBuilder($connection); + + $builder->getColumnListing('foo'); + } + + /** + * Ensure that when the reference is qualified only with a schema, that + * the database specified on the connection is used, and the specified + * schema is used, even if it is not within the search_path. + */ + public function testWhenSchemaNotInSearchPathGetColumnListingWithQualifiedSchemaReferenceIsCorrect() + { + $connection = $this->getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn('public'); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileColumnListing')->andReturn('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?'); + $connection->shouldReceive('select')->with('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?', ['laravel', 'myapp', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $processor = m::mock(PostgresProcessor::class); + $connection->shouldReceive('getPostProcessor')->andReturn($processor); + $processor->shouldReceive('processColumnListing')->andReturn(['some_column']); + $builder = $this->getBuilder($connection); + + $builder->getColumnListing('myapp.foo'); + } + + /** + * Ensure that when the reference is qualified with a database AND a schema, + * and the database is NOT the database configured for the connection, the + * specified database is used instead. + */ + public function testWhenDatabaseNotDefaultGetColumnListingWithFullyQualifiedReferenceIsCorrect() + { + $connection = $this->getConnection(); + $connection->shouldReceive('getConfig')->with('search_path')->andReturn('public'); + $grammar = m::mock(PostgresGrammar::class); + $connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar); + $grammar->shouldReceive('compileColumnListing')->andReturn('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?'); + $connection->shouldReceive('select')->with('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?', ['mydatabase', 'myapp', 'foo'])->andReturn(['countable_result']); + $connection->shouldReceive('getTablePrefix'); + $connection->shouldReceive('getConfig')->with('database')->andReturn('laravel'); + $processor = m::mock(PostgresProcessor::class); + $connection->shouldReceive('getPostProcessor')->andReturn($processor); + $processor->shouldReceive('processColumnListing')->andReturn(['some_column']); + $builder = $this->getBuilder($connection); + + $builder->getColumnListing('mydatabase.myapp.foo'); + } + + protected function getConnection() + { + return m::mock(Connection::class); + } + + protected function getBuilder($connection) + { + return new PostgresBuilder($connection); + } + + protected function getGrammar() + { + return new PostgresGrammar; + } +} diff --git a/tests/Database/DatabasePostgresSchemaGrammarTest.php b/tests/Database/DatabasePostgresSchemaGrammarTest.php index 052197fd08c4..f99728474107 100755 --- a/tests/Database/DatabasePostgresSchemaGrammarTest.php +++ b/tests/Database/DatabasePostgresSchemaGrammarTest.php @@ -998,6 +998,20 @@ public function testDropAllTypesEscapesTableNames() $this->assertSame('drop type "alpha","beta","gamma" cascade', $statement); } + public function testCompileTableExists() + { + $statement = $this->getGrammar()->compileTableExists(); + + $this->assertSame('select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = \'BASE TABLE\'', $statement); + } + + public function testCompileColumnListing() + { + $statement = $this->getGrammar()->compileColumnListing(); + + $this->assertSame('select column_name from information_schema.columns where table_catalog = ? and table_schema = ? and table_name = ?', $statement); + } + protected function getConnection() { return m::mock(Connection::class);