Selection->getPrimary() is not sufficiently specific

[temistokles]

Selection->getPrimary returns name of primary key (in my case id for all the tables).

This is also true when joining tables - e.g. with where table2.column = ?.

Unfortunatelly when joining very large tables (hundreads of thousands of rows, hundread columns) the order of returned columns is indeterminate in MySQL. Although observed here, this might not be the only case when this happens.

The resulting behaviour is the primary key from table2 is considered instead of table1. This has fatal results - not all the rows are returned and they might get damaged when being updated.

The easy way to fix this seemed to be:

1. not using PKs in Statement->execute, unfortunatelly this breaks referencing (row->table2->table2row).
2. just using <table name>.id instead of just id in Selection->getPrimary, but this breaks referencing as well

[Unlink]

I'm not sure, but i think that problem is not in nette database. You probably use it wrong.
Principle of ndbt is, you select only from one table, there aren't joins, only joins for conditions.

So, if you want to use custom ->select() you should know, what are you doing and specify alias for primary key from another table.

IMHO it would be weird, if i have selection for table1 and there is id whitch came from another table..

[temistokles]

table1 having PK id and table2 having PK id

The use is:
$connection->table('table1')->where(['table2.parameter1' => 'some value'])

If parameter1 chooses rows from table2 having non-unique table2.id, the query returns only so many rows how many unique values of table2.id exist. Internaly these values are stored based on PK from table2, not table1, which is wrong.

The database query itself gets rows from table1, these rows are correctly retrieved, but they get overwritten in https://github.com/nette/database/blob/master/src/Database/Table/Selection.php#L509.

[Unlink]

Can you prepare steps, how to reproduce or write falling test?

[temistokles]

I proposed the scenario in the pull request. The issue seems to be at select('*'), without it, the query returns the right number of rows.

[Unlink]

I think that ->select('*') is wrong construction, but problem is not directly in nette database. In PDO if you use PDO::FETCH_ASSOC mode, you get assoc array indexed by columns names so if there are duplicate columns names, newest rewrites oldest.

I think that we could add exception when you select columns with duplicate names to avoid this strange behaviour.
Something like this

if ($this->pdoStatement->columnCount() != count($row)) {
    throw ...
}

[temistokles]

Well this is one way - let user know there would be a problem. Unfortunatelly this approach does not guarantee the exception occured because of using select('*').

In other words, the exception would just say You probably did something wrong, maybe used select(*)?. I don't think this would help a user that much - because it does not provide the solution.

During my later debugging I discovered that select('table1.*)works as expected! Moreover it does not affect the ability to referencetable2and get its data. Maybe a magical "fix" when using just*` is better than magical exception?

[temistokles]

Another point of view - maybe we could just detect the situation when tables share column names (one would overwrite the other) and if this happens, simply use the PK of primary table (in this case not just id, but table1.id).

[Unlink]

Yes, but what if someone use select('table1.*, table2.id').
I also thinking, that it can automaticaly add aliased primary keys at end of query (as you suggest table1.id) and create rows using that keys but i'm not sure, if this can't break something.

I think, that exception is good, when user gets less columns than how it wants.
Maybee it can also says, whitch columns are duplicate st. like Your resultset contains duplicate columns: 'table1.id', 'table2.id'

[temistokles]

I think placing <primary table>.<PK> as last column in select, renamed to e.g. AS netteDatabasePrimaryRowKey would be an elegant solution. Yes this can break things - mainly queries that considered the current behavior "normal".

[Unlink]

I agree, but i don't know what else it can break.
As i suggested exception, something like that is alredy implemented in ResultSet:
https://github.com/nette/database/blob/master/src/Database/ResultSet.php#L263 but selection use result set only for create statement so this check not apply.

Select primary key sepparatly can also prevent from wrong use of ActiveRow when you can update or delete wrong row, if you use st. like this: select('table1.*, table2.id')