Skip to content

Enh #1604: CDbCommandBuilder supports multiple insertion #2285

Merged
merged 8 commits into from Apr 8, 2013

6 participants

@klimov-paul
Yii Software LLC member

Added method "CDbCommandBuilder::createMultipleInsertCommand()" to suport multiple insertion.
"CSqliteCommandBuilder" and "COciCommandBuilder" override "createMultipleInsertCommand()" implementing thier own syntax.

Resolves #1604.
Migrated from #2274

Has been tested for MySQL and SQLite.
No idea, if Implementation for Oracle actually works. It need to be tested.

@klimov-paul klimov-paul referenced this pull request Apr 2, 2013
Closed

Enh #1604: CDbCommand allows multiple insertion #2274

4 of 5 tasks complete
@samdark
Yii Software LLC member
samdark commented Apr 3, 2013

Can we extract common parts into method? Does it make sense to do so?

@klimov-paul
Yii Software LLC member

Perhaps, but in this case there will be several protected methods, which serve only as support for the "generateMultipleInserCommand()". On the other hand there is much code duplication at the present momemt.

I will think about refactoring.

klimov-paul added some commits Apr 3, 2013
@klimov-paul klimov-paul Method "CDbCommandBuilder::composeMultipleInsertCommand()" has been e…
…xtracted.
e167f01
@klimov-paul klimov-paul Doc comment tag @since 1.1.14 has been added to "CDbCommandBuilder::c…
…reateMultipleInsertCommand()"
59bced1
@klimov-paul
Yii Software LLC member

Method "CDbCommandBuilder::composeMultipleInsertCommand()" has been extracted to provide code reusage.
Still need test run for Oracle.

@resurtm
resurtm commented Apr 3, 2013

Still need test run for Oracle.

OK, i'm in process of testing it. I'll let you know when it'll be ready.

@klimov-paul
Yii Software LLC member

@resurtm, you may try to run unit test 'tests/framework/db/schema/COciCommandBuilderTest.php'.
Just do not forget to adjust config.

@resurtm
resurtm commented Apr 3, 2013

Results:

$ phpunit framework/db/schema/COciCommandBuilderTest.php
PHPUnit 3.7.18 by Sebastian Bergmann.

Configuration read from C:\_work\jetbrains\yii\tests\phpunit.xml

E

Time: 6 seconds, Memory: 4.00Mb

There was 1 error:

1) COciCommandBuilderTest::testMultipleInsert
CDbException: CDbCommand failed to execute the SQL statement: SQLSTATE[HY000]: G
eneral error: 1722 OCIStmtExecute: ORA-01722: invalid number
 (ext\pdo_oci\oci_statement.c:148). The SQL statement executed was: INSERT ALL I
NTO "types" ("int_col", "char_col", "char_col2", "float_col", "bool_col") VALUES
 (:int_col_0, :char_col_0, :char_col2_0, :float_col_0, :bool_col_0) INTO "types"
 ("int_col", "char_col", "char_col2", "float_col", "bool_col") VALUES (:int_col_
1, :char_col_1, NULL, :float_col_1, :bool_col_1) SELECT * FROM dual

C:\_work\jetbrains\yii\framework\db\CDbCommand.php:358
C:\_work\jetbrains\yii\tests\framework\db\schema\COciCommandBuilderTest.php:119
C:\_bin\php-5.4.3-Win32-VC9-x86\pear\phpunit:46

FAILURES!
Tests: 1, Assertions: 0, Errors: 1.

Feel free to request additional information, etc. Maybe via instant messaging it will be faster—you decide.

@klimov-paul
Yii Software LLC member

Oracle syntax checked successfully.

@klimov-paul
Yii Software LLC member

Need to perform some refactoring near unit tests.

@klimov-paul klimov-paul "CSqliteCommandBuilderTest" has been merged into "CSqliteTest".
"COciCommandBuilderTest" has been merged into "COciTest".
cec25ca
@klimov-paul
Yii Software LLC member

Now should be ready for the merging.

@samdark
Yii Software LLC member
samdark commented Apr 4, 2013

Looks good to me (just checked structure, not really tested).

@klimov-paul klimov-paul merged commit c6b10c5 into yiisoft:master Apr 8, 2013
@klimov-paul klimov-paul was assigned Apr 8, 2013
@slavcodev

Please make composeMultipleInsertCommand public!

Usage

$cmd = $builder->composeMultipleInsertCommand($tableName, $values, array(
  'main'=>'INSERT INTO {{tableName}} ({{columnInsertNames}}) VALUES {{rowInsertValues}} ON DUPLICATE KEY UPDATE',
));
@creocoder

@slavcodev Yes, good idea.

@slavcodev

@klimov-paul , @resurtm please welcome back :)

@klimov-paul
Yii Software LLC member

Please make composeMultipleInsertCommand public!

Why?

@slavcodev

Because this method not change the component state, it only generate SQL.
Is not necessary protected it.
Finaly I'm write usage example in previous message.

@klimov-paul
Yii Software LLC member

@slavcodev, here is a procedure: open a separated issue, describe your use case and reasons why method "composeMultipleInsertCommand()" should be published.
We will continue our debates there.

@slavcodev

@klimov-paul my propose is not new issue, is help to make your PR better.
I think that is unnecessary to encapsulate the method that does not change the state of the object, but only converts the input given in a different format

@creocoder

@klimov-paul This is thread about you PR, so i think debates should be there. Also i think that method composeMultipleInsertCommand() is excess.

@creocoder

@klimov-paul Also if its really needed, how to make sql queries like INSERT IGNORE ..., INSERT ... ON DUBLICATE KEY UPDATE without making this method public?

@samdark
Yii Software LLC member
samdark commented Apr 10, 2013

@creocoder composeMultipleInsertCommand is not to duplicate the same code for Oracle and SQLite.

@creocoder

@samdark Yes, got it. But how about custom INSERT templates at user level?

@klimov-paul
Yii Software LLC member

Method “CDbCommandBuilder::composeMultipleInsertCommand()” has been created by necessity to avoid duplicating same code for different multiple insert syntax, which SQLite and Oracle use.
Now you wish to convert necessity into a feature.
The original issue was about support of multiple insertions by the single query. This issue is resolved by this pull request. Allowing the composition of SQL queries by given patterns is totally another issue. That is why I propose to track it separately.
But fine, let’s do this here.

Here is my opinion on this matter:

  • Framework supports only the most common SQL expressions. It can NOT cover all possible variations. If you wish to create your own custom SQL command you can do it on your own.
  • “CDbCommandBuilder::composeMultipleInsertCommand()” is a service method, it is not a part of public interface. Patterns at this method ware created to support different syntax, which different database engines maintain. “CDb*” classes introduce an abstraction layer for ANY database engine, which means it public interface functions should be CROSSENGINE. While there are different syntax for multiple insertions for different db engines, allowing invokes of “INSERT ... ON DUBLICATE KEY UPDATE” breaks the cross engine feature.
  • If we start to support vary placeholders at some command, next we will add them to ALL commands.
  • If you wish to create an multiple insertions command at your own syntax, you can extend “CMysqlDbCommadBuilder” and override method “createMultipleInsertCommand()” or create a new one. There is nothing, which prevent this.
@samdark
Yii Software LLC member
samdark commented Apr 11, 2013

Agree with @klimov-paul. Adding the feature to one method and not allowing to use it in all other methods will just make API inconsistent.

@creocoder

@klimov-paul Yes, now everything is clear, excellent explanation. Totally agree with you.

@creocoder

@slavcodev If you want custom templates i think its good idea to extend CMysqlDbCommadBuilder and implement methods like createMultipleInsertIgnoreCommand(), createMultipleInsertOnDublicateKeyUpdateCommand(), etc.

@kiaplayer

@klimov-paul Should method createMultipleInsertCommand present in CDbCommand?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.