nextras / orm

Orm with clean object design, smart relationship loading and powerful collections.
https://nextras.org/orm
MIT License
310 stars 57 forks source link

SQL Server - duplicate data after persist #254

Closed dyamon-cz closed 6 years ago

dyamon-cz commented 6 years ago

Hello guys,

I'm currently testing Nextras/Orm with sqlsrv driver and I actually found issue immediately on beginning of my testing. Point is simple I've made just one entity and persist it into database, after look on data I can see two records of the entity what should have to be persisted only once.

I'm not sure if I did anything wrong but if I switch db to mysql everything works well. I'm using latest nette/web-project, Nextras/Orm dev-master, Nextras/Dbal dev-master and SQL Server 2012.

In my case is possible to find out this issue just with simple entity Book, id {primary} and name. In the database is identity(1,1) for primary key and varchar(50) for name.

Interesting is that nette debug-bar is showing only one insert.

debug-bar

But I'm getting two records in the database.

records

What is even more interesting and quite strange that when I put unique key on column name everything stay works well (no error that orm is trying to put two records to the table with unique key) and I even get just one record in the database.

Don't you have any idea what could be wrong?

Thank you very much!

PS: Sorry for screens having different names (Book1, Book2) I captured it in different stage of testing.

dyamon-cz commented 6 years ago

OK guys, is because Debug Panel for Tracy, SqlsrvDriver::query (in Nextras/Dbal) is called two times when one of those queries is EXPLAIN INSERT INTO [books] ([name]) VALUES ('Book 4'). That's also the reason why the tests couldn't discover this bug.

sorry, maybe will be better to move this issue under DBAL

dyamon-cz commented 6 years ago

This issue is possible to fix with configuration option for Nextras/Dbal panelQueryExplain: false

hrach commented 6 years ago

Hi, thanks for reporting! So the EXPLAIN INSERT INTO [books] ([name]) VALUES ('Book 4') actually inserts the row again?

dyamon-cz commented 6 years ago

Exactly, there is no such a thing like EXPLAIN in T-SQL syntax. So native client will throw error and execute rest of the query and because there is try{ } catch{ } in ConnectionPanel::getPanel there will be no exception.

hrach commented 6 years ago

🤦‍♂️

hrach commented 6 years ago

Is it somehow possible to disable this behavior? (E.g. do nothing when there is an error -> exception)

dyamon-cz commented 6 years ago

Here is the method...

public function getPanel(){

    $count = $this->count;
    $queries = $this->queries;
    $queries = array_map(function($row) {
        try {
            $row[4] = $this->doExplain ? $this->connection->getDriver()->query('EXPLAIN ' . $row['1'])->fetchAll() : null;
        } catch (\Throwable $e) {
            $row[4] = null;
            $row[3] = null; // rows count is also irrelevant
        }

        $row[1] = self::highlight($row[1]);
        return $row;
    }, $queries);
    $whitespaceExplain = $this->connection->getPlatform()->getName() === 'pgsql';

    ob_start();
    require __DIR__ . '/ConnectionPanel.panel.phtml';
    return ob_get_clean();
}

Because I'm not really sure what is going to happen with the data which are obtained from EXPLAIN query I don't know how this can be fixed and don't affect some intentional functionality. Like I said before there is a property doExplain so I have put panelQueryExplain: false to config of DBAL which completely skip that query execution. Even debug panel still works.

hrach commented 6 years ago

They are just rendered. I think this behavior must be caused by SQL server itself, or you you think it's just nextras' bug? I understood that as the problem of SQL server, which actually runs the query.

dyamon-cz commented 6 years ago

Oh yes that's actually quite wtf behavior of SQL server, you can try just simply run EXPLAIN INSERT INTO [books] ([name]) VALUES ('Book 4') in SQL Server Management Studio and you'll get this... eplain_query

SQL Server understand this execution as it would be two separate statements so you will get error for EXPLAIN but INSERT... is going to be executed anyway. This is like standard SQL Server behavior. I don't know about any way how to change it.

hrach commented 6 years ago

Ok :) Thanks for verification. Will fix. :)