Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Fix parsing config('database.connections.pgsql.search_path') #41088

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

derekmd
Copy link
Contributor

@derekmd derekmd commented Feb 17, 2022

Fixes #41062

e.g., Config value 'test-db' was being parsed as set search_path to "test", "db" instead of set search_path to "test-db"

9.x renamed config('database.connections.pgsql.schema') to config('database.connections.pgsql.search_path') and introduced special handling if the config value is a string containing many segments. The originating PR has some examples. However the given PostgresConnector regex doesn't consider the full range of characters allowed in a schema or variable name - specifically '-' and accented characters.

Replace the 'search_path' regex alphanumeric-only allowlist of characters with a blocklist of delimiters when config('database.connections.pgsql.search_path') is a string value. Technically Postgres does allow our config delimiter characters (spaces, comma, quotes) in symbols so an array configuration can instead be used for such schema names. (However non-wrapping single/double quote characters in such array configs still aren't supported.)

return [
    'default' => env('DB_CONNECTION', 'mysql'),
    'connections' => [
        'pgsql' => [
            'search_path' => [
                "'public'",
                '"$user"',
                'containing commas, and spaces'
                // 'can\'t name schema with "inner quotes"'
            ],
set search_path to "public", "$user", "containing commas, and spaces"

  • Roll methods testPostgresSearchPathCommaSeparatedValueSupported() & testPostgresSearchPathVariablesSupported() into testPostgresSearchPathIsSet() with the fuller coverage provideSearchPaths() data set.
  • testPostgresSearchPathArraySupported() is repurposed to show config('database.connections.pgsql.schema') from versions < 9.x is used when the 'search_path' config key is absent.
  • Fix PostgresConnector::quoteSearchPath() parameter docblock since passing in a string value would throw an exception for being un-Countable. Its only use is being passed parseSearchPath()'s return value which is an array.

The given PostgresConnector regex doesn't consider the full range of
characters allowed in a schema or variable name - specifically '-' and
accented characters.

e.g., 'test-db' was being parsed as `set search_path to "test", "db"`
instead of `set search_path to "test-db"`

Replace the 'search_path' regex allowlist of characters with a blocklist
of  delimiters when config('database.connections.pgsql.search_path') is
a string value.

Technically Postgres _does_ allow our config delimiter characters
(spaces, comma, quotes) in symbols so an array configuration can
instead be used for such schema names. However single/double quote
characters in such array configs aren't supported.

---

* Roll methods testPostgresSearchPathCommaSeparatedValueSupported() &
  testPostgresSearchPathVariablesSupported() into
  testPostgresSearchPathIsSet() with the provideSearchPaths() data set.
* testPostgresSearchPathArraySupported() is repurposed to show
  config('database.connections.pgsql.schema') from versions < 9.x is
  used when the 'search_path' config key is absent.
* Fix PostgresConnector::quoteSearchPath() docblock since passing in a
  string value would throw an exception for being un-Countable. Its only
  use is being given parseSearchPath()'s return value which is an array.
@derekmd derekmd force-pushed the postgresql-parse-search-path-fix branch from e1a9467 to eff2a00 Compare February 17, 2022 21:51
@taylorotwell taylorotwell merged commit a8ce83a into laravel:9.x Feb 17, 2022
@taylorotwell
Copy link
Member

Thanks!

@derekmd derekmd deleted the postgresql-parse-search-path-fix branch February 17, 2022 21:57
derekmd added a commit to derekmd/framework that referenced this pull request Feb 24, 2022
….search_path')`

1. `PostgresBuilder::parseSchemaAndTable()`
   Needs a fallback to <= 8.x `config('database.connections.pgsql.schema')`
   when 9.x renamed `config('database.connections.pgsql.search_path')`
   is missing.
2. `PostgresBuilder::getAllTables()` + `getAllViews()` + `parseSchemaAndTable()`
   Apply the `parseSearchPath()` fixes applied to PostgresConnector:
   laravel#41088
3. `PostgresBuilder::parseSchemaAndTable()`
   Remove duplicate `$user` variable `config('database.connections.pgsql.username')`
   replacement handling already done by `parseSearchPath()`.
4. `DatabasePostgresBuilderTest`
   Add more test cases and use terse method names instead of extensive
   comments to concisely communicate each case.
derekmd added a commit to derekmd/framework that referenced this pull request Feb 24, 2022
….search_path')`

1. `PostgresBuilder::parseSchemaAndTable()`
   * Needs a fallback to <= 8.x `config('database.connections.pgsql.schema')`
     when 9.x renamed `config('database.connections.pgsql.search_path')`
     is missing.
   * Remove duplicate `$user` variable `config('database.connections.pgsql.username')`
     replacement handling already done by `parseSearchPath()`.
2. `PostgresBuilder::getAllTables()` + `getAllViews()` + `parseSchemaAndTable()`
   Apply the `parseSearchPath()` fixes applied to PostgresConnector:
   laravel#41088
3. `DatabasePostgresBuilderTest`
   Add more test cases and use terse method names instead of extensive
   comments to concisely communicate each case.
derekmd added a commit to derekmd/framework that referenced this pull request Feb 24, 2022
….search_path')`

1. `PostgresBuilder::parseSchemaAndTable()`
   * Needs a fallback to <= 8.x `config('database.connections.pgsql.schema')`
     when 9.x renamed `config('database.connections.pgsql.search_path')`
     is missing.
   * Remove duplicate `$user` variable `config('database.connections.pgsql.username')`
     replacement handling already done by `parseSearchPath()`.
2. `PostgresBuilder::getAllTables()` + `getAllViews()` + `parseSchemaAndTable()`
   Apply the `parseSearchPath()` fixes applied to PostgresConnector:
   laravel#41088
3. `DatabasePostgresBuilderTest`
   Add more test cases and use terse method names instead of extensive
   comments to concisely communicate each case.
derekmd added a commit to derekmd/framework that referenced this pull request Feb 24, 2022
….search_path')`

1. `PostgresBuilder::parseSchemaAndTable()`
   * Needs a fallback to <= 8.x `config('database.connections.pgsql.schema')`
     when 9.x renamed `config('database.connections.pgsql.search_path')`
     is missing.
   * Remove duplicate `$user` variable `config('database.connections.pgsql.username')`
     replacement handling already done by `parseSearchPath()`.
2. `PostgresBuilder::getAllTables()` + `getAllViews()` + `parseSchemaAndTable()`
   Apply the `parseSearchPath()` fixes applied to PostgresConnector:
   laravel#41088
3. `DatabasePostgresBuilderTest`
   Add more test cases and use terse method names instead of extensive
   comments to concisely communicate each case.
derekmd added a commit to derekmd/framework that referenced this pull request Feb 24, 2022
….search_path')`

1. `PostgresBuilder::parseSchemaAndTable()`
   * Needs a fallback to <= 8.x `config('database.connections.pgsql.schema')`
     when 9.x renamed `config('database.connections.pgsql.search_path')`
     is missing.
   * Remove duplicate `$user` variable `config('database.connections.pgsql.username')`
     replacement handling already done by `parseSearchPath()`.
2. `PostgresBuilder::getAllTables()` + `getAllViews()` + `parseSchemaAndTable()`
   Apply the `parseSearchPath()` fixes applied to PostgresConnector:
   laravel#41088
3. `DatabasePostgresBuilderTest`
   Add more test cases and use terse method names instead of extensive
   comments to concisely communicate each case.
taylorotwell pushed a commit that referenced this pull request Feb 24, 2022
….search_path')` (#41215)

1. `PostgresBuilder::parseSchemaAndTable()`
   * Needs a fallback to <= 8.x `config('database.connections.pgsql.schema')`
     when 9.x renamed `config('database.connections.pgsql.search_path')`
     is missing.
   * Remove duplicate `$user` variable `config('database.connections.pgsql.username')`
     replacement handling already done by `parseSearchPath()`.
2. `PostgresBuilder::getAllTables()` + `getAllViews()` + `parseSchemaAndTable()`
   Apply the `parseSearchPath()` fixes applied to PostgresConnector:
   #41088
3. `DatabasePostgresBuilderTest`
   Add more test cases and use terse method names instead of extensive
   comments to concisely communicate each case.
taylorotwell pushed a commit to illuminate/database that referenced this pull request Feb 24, 2022
….search_path')` (#41215)

1. `PostgresBuilder::parseSchemaAndTable()`
   * Needs a fallback to <= 8.x `config('database.connections.pgsql.schema')`
     when 9.x renamed `config('database.connections.pgsql.search_path')`
     is missing.
   * Remove duplicate `$user` variable `config('database.connections.pgsql.username')`
     replacement handling already done by `parseSearchPath()`.
2. `PostgresBuilder::getAllTables()` + `getAllViews()` + `parseSchemaAndTable()`
   Apply the `parseSearchPath()` fixes applied to PostgresConnector:
   laravel/framework#41088
3. `DatabasePostgresBuilderTest`
   Add more test cases and use terse method names instead of extensive
   comments to concisely communicate each case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Laravel 9, Postgres can't change search_path to valid schema
2 participants