Fix statement for getTableWhereClause method

[sinner]

If you have a role "postgres" in PostgreSQL that is described like this:

-- Role: postgres
CREATE ROLE postgres LOGIN
  ENCRYPTED PASSWORD 'md53175bce1d3201d16594cebf9d7eb3f9d'
  SUPERUSER INHERIT CREATEDB CREATEROLE REPLICATION;
ALTER ROLE postgres IN DATABASE sf_test
  SET search_path = "$user", public, access, geographic, rrhh;

At the time when I execute the statement (Part of $schema value in the line 348 without replace spaces):

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

I fetch this result:

{postgres," public"," access"," geographic"," rrhh"}

Look the space character that is on the start of each string.This error does not match any namespace to which the role has access. This is critical when you have PostgreSQL database and you work with schemas.

[deeky666]

@sinner Maybe duplicate of this PR #466?

[sinner]

Yes @lluisi, I fixed it, because is an error (Doctrine2 have many errors, seems that only works with MySQL but I love Symfony2 and well...). This error does that not match any namespace to which the role has access. This is critical when you have PostgreSQL database and you work with schemas.

Exactly, is not what I'm looking for... The "trim" function is not the solution and much less where you placed, You have even tested what you posted? I have placed the data there, you can make the test. The returned data is an PostgreSQL array, therefore make a trim on PHP side do nothing.

Also, I tried searching a function callback trim (native) for each array element that work with string_to_array on PostgreSQL like array_map on PHP but no luck. I think that is better solution.

@deeky666 I saw it after I commit this pull request, can be duplicate but this pass the Travis test, but the #466 no pass the Travis test instead. Also, the #466 assume that the role of user have more that one schema permissions and this can not be true ever.

[deeky666]

@sinner PR #466 also is strictly related to version 9.3, is that also true for this PR? According to the author the tests of the other PR fail because travis does not use 9.3 by default. But I cannot confirm that this is true by now. Is it possible that you can add a functional test that covers this issue? Because this line you changed looks rather messy and I would like to see what you expect in a test.

[sinner]

This code is tested on 9.1, 9.2 and 9.3 versions of PostgreSQL. Rather messy? Could you really see the commit? I added just a replacement of empty spaces:

replace(replace(setting,"$user",user),' ','')

and then, I could solve my problems.

The test for that commit is not feasible because the test is in the database. I don't understand what you want to see. I posted the example on the commit message.

I hope you can understand. Thank you very much for your help.

[deeky666]

@sinner I did not mean that what you did is messy ;) Sorry for the confusion. The line itself is just a little hard to read and understand if you do not have a deep insight into PostgreSQL. But this is not the point. The point is that we need a functional test for this to avoid regeressions in the future.

[sinner]

A PHP unit test for that commit is not feasible because the test is in the database, the query is bad formmed. The functional test was approved by Travis.

The "replace" function is supported from version 7.3 of PostgreSQL and and has not changed since then to version 9.3, this avoid any regressions in the future. Is just a replace SQL function...

I posted the example on the commit message.

If you have a role "postgres" in PostgreSQL that is described like this:

-- Role: postgres
CREATE ROLE postgres LOGIN
  ENCRYPTED PASSWORD 'md53175bce1d3201d16594cebf9d7eb3f9d'
  SUPERUSER INHERIT CREATEDB CREATEROLE REPLICATION;
ALTER ROLE postgres IN DATABASE sf_test
  SET search_path = "$user", public, access, geographic, rrhh;

At the time when I execute the statement (Part of $schema value in the line 348 without replace spaces):

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

we fetch this result:

{public," access"," geographic"," rrhh"} (PostgreSQL array)

Look the space character that is on the start of each string.This error does not match any namespace to which the role has access. This is critical when you have PostgreSQL database and you work with schemas. But if the query is changed by this:

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

we can fetch this result:

{public, access, geographic, rrhh} (PostgreSQL array)

I hope you can understand. Thank you very much for your help. If you can't, Can see this PR someone that have at least a "deep" insight into PostgreSQL?

And if you want a functional test let me see how make it a Database test on Github.

[deeky666]

@sinner I think you do not get what I mean ;) A test actually IS necessary IMO because the PostgreSQL didn't return any error on this issue before this PR. Only because it does not error now, too, does not mean that this issue is even covered by our testsuite. We need a test that would fail without your patch. This is what I mean with avoiding regressions ;)

[deeky666]

@sinner Have a look at the PostgreSQL functional test case. You should add the test there.

[sinner]

Didn't return any error on this issue before this PR because you don't have test with PostgreSQL working with schemas. Doctrine2 only use a public schema. I can not believe you have not noticed this before.

In any case the tests should be in PostgreSQL Platform test case.

But I can't make a test on PHP when the change is on a SQL query. Can you undestand? :) Believe me, do not have advanced knowledge in PostgreSQL to understand the change on PR :)

[deeky666]

@sinner After having a closer insight into this issue I now understand that it is pretty difficult to test this as it depends on the search_path setting which cannot be changed dynamically that easy in the testsuite so that we could create a failing test. So this might be okay without a test. I'll leave that up to @beberlei.
But I have another question. As you are now replacing all space in the search_path string, what about schema names that contain spaces like "foo bar" (don't know if this is possible at all). In my example it would be converted to "foobar", wouldn't it?

[sinner]

@deeky666 Although you're right and this is possible in databases like PostgreSQL, but is not a standard, in fact the PostgreSQL documentation recommends not using reserved words, start with underscore or lera, avoid points, spaces and characters special in the names of database objects. For more

But even so, the example you just gave me not generate this query, and the name of the scheme remains intact "foo bar" because for example a table named "myTable" have the entire name "foo bar"."myTable" an then it would this block:

if (strpos($table, ".") !== false) {
    list($schema, $table) = explode(".", $table);
    $schema = "'" . $schema . "'";
}

therefore, the part of query contained in the $whereClause variable would somthing like

... sc.relname = "myTable" AND sn.nspname = "foo bar"

But it is a very good point you speak and refers to another of my pull requests No dot on Class Names, because as before I said, Doctrine2 does not support database schemas postgreSQL in their reverse engineering processes and if you use schemas the entities (PHP Classes) generated has dots in their names, if in addition some object names as the name of a schema or the name of a table had a space to go through the EntityGenerate the entity has (with my referenced pull request) a proper name, but not the correct name of the table and should be enclosed in double quotes all these names that have spaces, dots, capitalization, among other things...

But this is just blah blah blah, I think that Doctrine2 should not engage something that is not a standard and is also frowned upon.

But what is important is that if I use the reverse engineering processes, the entity is generated with a valid name for a PHP Class. No dot on Class Names

[deeky666]

@sinner I get your point and you are right. But what I was concerned about is schema names in search_path that contain spaces. You know those that would be evaluated from pg_catalog.pg_settings in the getTableWhereClause method. As I said I don't know if that is even possible but according to your comment it is and so it should be considered somehow. I know this is a very rare edge case but we're all developers and know how stupid people are and know that someone will use it and complain about it ;) Otherwise speaking of reserved keywords if nobody would use it, we wouldn't have such a mess with getting things right in those cases :)

[sinner]

@deeky666 Really, Read you what I wrote? I have already tried locally and run fine.

If I get a search_path with spaces like that the example you just gave me, the name of the search_path (schema) remains intact "foo bar" because for example a table named "myTable" have the entire name "foo bar"."myTable" an then it would this block:

if (strpos($table, ".") !== false) {
    list($schema, $table) = explode(".", $table);
    $schema = "'" . $schema . "'";
}

therefore, the part of query contained in the $whereClause variable would somthing like

... sc.relname = "myTable" AND sn.nspname = "foo bar"

The replace function is never executed.

Can understand? :) Pay attention, And again, the Doctrine works with PostgreSQL is really poor :)

[sinner]

@deeky666 And although my pull request solves your problem spaces search_path, Anyway ... Doctrine does not support PostgreSQL schemas and you worry if the search_path (used to determine the schemes to which a PostgreSQL user has access) has a space? You should go step by step, do not you think?

[deeky666]

@sinner Okay let's come to an end here. I just wanted to throw some considerations around. You provided good feedback on this and therefore the PR is fine for me. I would have liked to have a functional test for this but this seems to be hard to achieve. I give the "okay" from my side and @beberlei will merge if he thinks so, too.
Thank you very much for this contribution!

[beberlei]

@sinner You mention "many errors", can you elaborate on some more problems you have found? We would like to fix them of course.

[beberlei]

Merged fix into 2.4 and 2.3 release branches, http://www.doctrine-project.org/jira/browse/DBAL-766