propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 396 forks source link

Propel diff ignores first column when auto-generating indexes #1837

Open Lkopo opened 2 years ago

Lkopo commented 2 years ago

Version: Propel2.0.0-beta1 DB engine: MySQL (MariaDB)

When table is created as in docs: http://propelorm.org/documentation/04-relationships.html#many-to-many-relationships the index is created only for the second column. The first one is ignored.

We are using Propel2 ORM and I have noticed that when I generate propel diff migration file, it always tries to delete index which doesn't exist. An example based on the docs above when such thing happens:

By debugging Propel code, I tried to understand how the logic of generating diffs for migration is working. I must admit, it was a challenge. To simplify it, Propel creates 2 databases (objects), one based on schema XML files & another one based on real DB tables. It also tries to generate indexes automatically if they are not specified. As it's in Propel\Generator\Model\Table#addExtraIndices() it gets the list of columns in some order. The first column is simply ignored:

# class Propel\Generator\Model\Table

    ....

    protected function collectIndexedColumns($indexName, $columns, &$collectedIndexes)
    {
        $indexedColumns = [];
        foreach ($columns as $column) {
            $indexedColumns[] = $column;
            $indexedColumnsHash = $this->getColumnList($indexedColumns); // for first column the hash is just the column name
            if (!isset($collectedIndexes[$indexedColumnsHash])) {
                $collectedIndexes[$indexedColumnsHash] = [];
            }
            $collectedIndexes[$indexedColumnsHash][] = $indexName;
        }
    }

    ....

    # inside addExtraIndices() method lines 473-478, $_indices was used as third argument to collectIndexedColumns() method:

        foreach ($this->foreignKeys as $foreignKey) {
            $localColumns = $foreignKey->getLocalColumnObjects();
            $localColumnsHash = $this->getColumnList($localColumns); // first column has its hash same as column name
            if (empty($localColumns) || isset($_indices[$localColumnsHash])) {
                continue; // execution will fall here therefore no index for first column is created
            }

At the end the table ends up with just one index. Problem might occur when there is different order of the table columns for database generated based on XML schema files & database generated based on DB tables. This eventually leads to that database generated based on DB tables would have two indexes even though it's not true. Why I mentioned 2 indexes if the example says only one is generated? For database based on DB tables, first indexes are fetched from DB table. Then the process to generate indexes is rerun again (now with the code execution as above). This leads to two indexes in case the columns order is different. And this creates the situation Propel tries to delete the non-existing index everytime Propel diff is generated.

To conclude it, I don't think the order of the column is a problem in here but the fact, the first column is always ignored if we are talking about index generation. In case I am missing something that ignoring the first column is on purpose, then the order of columns would have to be ensured all the time.

dereuromark commented 2 years ago

Do you have an idea for a fix in mind? If you could PR that, this would be awesome

We plan to make a new beta release very soon.

Lkopo commented 2 years ago

Actually I don’t as I don’t have it confirmed whether skipping first column for Index is intentional. Maybe it is for tables with just one primary key (as they are indexed automatically) but also M:N table with composite primary key is affected.