Modified subquery in PostgreSqlPlatform->getListTableForeignKeysSQL()

[jwilson-cee]

Fixed cardinality issue where subquery could return more than than one row in getListTableForeignKeysSQL function

Q	A
Type	bug
BC Break	no
Fixed issues	#950

Summary

The cardinality error would happen when the schema wasn't prefixed in the $table parameter. As the subquery in the WHERE = clause would return more than one result if there was the same table name in two different schemas. (This is specific to postgres only.)

This adds:

• LIMIT 1 to the subquery of the WHERE = (SELECT ...) clause
• A clearer parameter name $tableWithSchema and doc blocks to convey what's expected for this function
• A deprecation message for when the schema is not included

[greg0ire]

Please format your commit message according to these rules :

• The commit message subject must be less than 50 characters and tell us what you did.
• The commit message body should tell us why you did it. It is optional but highly recommended.

Bad example :

Fixed bug #989 by removing call to defraculate()

Good example:

Remove call to defraculate()

Calling this function caused a bug because it interferes 
with calls to getSchmeckles().
Fixes #989

More details in CONTRIBUTING.md

[Ocramius]

Change looks good, but I'm wondering if there is a way to test this patch?

[jwilson-cee]

Change looks good, but I'm wondering if there is a way to test this patch?

In order to reproduce this error you need to have a postgres database that has two schemas that have a table with the same name:

TestDB
    ->SchemaName1
        ->TestTable
    ->SchemaName2
        ->TestTable

Then to trigger the error, fetch a column type from the schema:
Schema::getColumnType('TestTable', 'id')

[Ocramius]

Yes, and that needs to be in our automation test suite with the other tests

[jwilson-cee]

Yes, and that needs to be in our automation test suite with the other tests

@Ocramius, you asked a question and I gave answer. What's the purpose of this comment? Do you want me to write a test for this?

[Ocramius]

Yes, in @doctrine/doctrinecore we don't merge fixes without tests, unless they fix an already failing test

[jwilson-cee]

Ok, I'm giving up on this. You guys can take it or leave it.

I can't get it to pass some of your code checks.

The code sniffer in travis is complaining about type hinting array items, I can't find any examples of type hinting array items. And it's complaining about the in_array() function, of which cannot find any documentation or reference to this error Function in_array() should not be referenced via a fallback global name, but via a use statement. (SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFallbackGlobalName.

Also getting an error from appveyor about it not being able to find a driver. Not sure what to do about that, as I can get the phpunit test to pass locally.

[Ocramius]

Function in_array() should not be referenced via a fallback global name, but via a use statement.

This means that at the top of the file, after the other use statements, there should be a use function in_array;

I can't find any examples of type hinting array items.

The syntax is something like @var Foo[] or @param Foo[] $paramName or @return Foo[]

[andrew-demb]

Seems like travis failures is not related to PR.
Can we retry checks, or merge it?

[jwilson-cee]

Seems like travis failures is not related to PR.
Can we retry checks, or merge it? - @andrew-demb

I believe this PR passes all the checks. I think it is being blocked by @morozov 's Change Request comment?
https://github.com/doctrine/dbal/pull/3422/files/0a55ce69a634009624f30f11971770ffbeb42099
(you may need to click on "Show Conversation" to see the discussion)

Not interested.

[morozov]

With all due respect, I implore you to stop beating around the bush with this question of schema specificity, and please let this PR be merged.

I dismissed my request for changes. Please have someone else review the patch.

[greg0ire]

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

1. git rebase -i origin/master, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
2. A window will show up with many lines, replace pick with fixup on every line but the first one
3. Close your editor, git should do its magic, and you should end up with one commit
4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

[greg0ire]

There are some conflicts

[greg0ire]

@jwilson-cee jwilson-cee requested a review from greg0ire 4 hours ago

Github still complains about conflicts… can you please rebase?

[greg0ire]

The build is failing because of coding style issues. You can try running vendor/bin/phpcbf to fix them.

[jwilson-cee]

The build is failing because of coding style issues. You can try running vendor/bin/phpcbf to fix them.

I keep getting this error when trying to run phpcbf:
ERROR: Referenced sniff "Doctrine" does not exist

[Ocramius]

@jwilson-cee run a composer install again: I think this depends on whether post-install scripts were run or not

[jwilson-cee]

@jwilson-cee run a composer install again: I think this depends on whether post-install scripts were run or not

Thanks, but getting the same error after re-running composer install. I'll just manually fix the code style.

[greg0ire]

This is starting to look really good!

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

1. git rebase -i origin/master, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
2. A window will show up with many lines, replace pick with fixup on every line but the first one
3. Close your editor, git should do its magic, and you should end up with one commit
4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

When doing so, please make sure to follow the contributing guide regarding the commit message. This PR is non-trivial, and that means the commit message should be high quality.

[greg0ire]

LGTM

[greg0ire]

• retarget needed
• remove LIMIT 1
• throw a meaningful exception if and only if there is a crash

Looks like we still have to find an agreement on what to do

[greg0ire]

We're debating this internally, and will do the necessarily retargets for you when we agree. We're in the middle of changing workflows for Doctrine, hence the confusion, sorry for this.

[jwilson-cee]

@greg0ire
I'm going to close this PR, unless you prefer otherwise.

My reasoning is that I've dug into all the getListTable...SQL() functions and I've discovered that all of them return SQL that will generate a list of all objects for a table name that is in multiple schemas (the getListTableForeignKeysSQL is the only one that crashes because of the = operator). I'll provide all the SQL for this that you can run to see for yourself.

Because of this discovery, I believe it might be better to move this discussion to the #950 issue thread than continue in here, as this may warrant further discussion on the direction you want to take for this. And, because of this, it will probably be cleaner to start with a new branch, and let this one die.

If you'd rather keep it in this branch/PR, I'll keep this open and put my findings (the SQL test code) in this comment thread.

Thanks

[greg0ire]

You can start afresh if you think it's best. I'm AFK this weekend though, so don't expect much more from me until Tuesday.

[jwilson-cee]

Closing this PR as the fix for this needs further discussion (see issue #950)