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

Already on GitHub? Sign in to your account

[DDC-2110] fixed problem with existing columns (postgresql) #305

Merged
merged 3 commits into from May 1, 2013

Conversation

Projects
None yet
6 participants
Contributor

MatthiasLohr commented Apr 15, 2013

Fixed http://www.doctrine-project.org/jira/browse/DDC-2110. While selecting the configured search_path, "$user" is not replaced with the name of the current user. This commit will do the replacing.

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-495

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius Ocramius and 1 other commented on an outdated diff Apr 15, 2013

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
@@ -280,7 +280,7 @@ private function getTableWhereClause($table, $classAlias = 'c', $namespaceAlias
list($schema, $table) = explode(".", $table);
$schema = "'" . $schema . "'";
} else {
- $schema = "ANY(string_to_array((select setting from pg_catalog.pg_settings where name = 'search_path'),','))";
+ $schema = "ANY(string_to_array((select replace(setting,'\"\$user\"',(SELECT user)) from pg_catalog.pg_settings where name = 'search_path'),','))";
@Ocramius

Ocramius Apr 15, 2013

Owner

This is unsafe IMO. Shouldn't you escape it? Also, please provide a test, since this is a fix

@MatthiasLohr

MatthiasLohr Apr 15, 2013

Contributor

What should be escaped? The username?

Test: I'm not sure how to write a test for that, because there are a couple of requirements for that. I tested it on my own platform where i found this bug.

@Ocramius

Ocramius Apr 15, 2013

Owner

@MatthiasLohr you don't really need to write a test that uses the db - checking the generated SQL is fine. For escaping, yes, please don't (ever) use string concatenation. If this could work with parameter binding, consider doing it that way.

@Ocramius

Ocramius Apr 15, 2013

Owner

Nevermind on the parameter binding. Didn't know that $user is pgsql syntax.

MatthiasLohr added some commits Apr 15, 2013

@MatthiasLohr MatthiasLohr removed unnecessary select 4baadaf
@MatthiasLohr MatthiasLohr new tests for PostgreSqlPlatform
- added test for getListTableColumnsSQL
- added test for getListTableForeignKeysSQL
- added test for getListTableIndexesSQL
810bf4c
Owner

Ocramius commented Apr 19, 2013

@guilhermeblanco ping on this one (especially about using a pattern in the test instead of checking the exact generated string)

Owner

guilhermeblanco commented Apr 19, 2013

@MatthiasLohr IIRC, Postgres also allows using:

replace(setting, :user, (SELECT user)) from pg_catalog.pg_settings where name = 'search_path')

And even:

replace(setting, $1, (SELECT user)) from pg_catalog.pg_settings where name = 'search_path')

Can you confirm them to me?

Owner

beberlei commented Apr 20, 2013

Tests on just the SQL return are bad. Please write actual functional tests for this functionality, the SQL is internal and should not matter to achieve a goal.

Contributor

MatthiasLohr commented Apr 21, 2013

@beberlei, not sure how to implement the tests you want. In the discussion with @Ocramius he said this kind of tests would be enough and i don't have the time for experimenting which type of tests would pass your merge criteria.
Furthermore, there was no test for these functions before, so if it's more important to have proper tests than a bug fixed, i would like to ask you to write the needed tests.

I think a bug fix is better than no bug fix and bad tests are better than no tests.

Contributor

MatthiasLohr commented Apr 21, 2013

@guilhermeblanco: First of all, (SELECT user) can be replaced with a simple user (my fault, see 4baadaf).

SELECT replace(setting, '"$user"', user) FROM pg_catalog.pg_settings WHERE name = 'search_path';

returns mlohr,public.

SELECT replace(setting, :user, user) FROM pg_catalog.pg_settings WHERE name = 'search_path';

returns

ERROR:  syntax error at or near ":"
LINE 1: SELECT replace(setting, :user, user) FROM pg_catalog.pg_sett...
SELECT replace(setting, $1, user) FROM pg_catalog.pg_settings WHERE name = 'search_path';

returns

ERROR:  there is no parameter $1
LINE 1: SELECT replace(setting, $1, user) FROM pg_catalog.pg_setting...

So, no, i can't confirm that.

$1 can be used in prepared statements AFAIK, but this query isn't one.

@beberlei beberlei merged commit 810bf4c into doctrine:master May 1, 2013

1 check passed

default The Travis build passed
Details

Bug still exists, I have added a comment at http://www.doctrine-project.org/jira/browse/DBAL-510

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment