Add Upsert/UpsertBatch to builder

[sclubricants]

This PR adds Upsert and UpsertBatch methods to builder.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[sclubricants]

The duplicate code can be resolved by refactoring.

The insertBatch() method can point to the new executeBatch() method.

The setInsertBatch() method can point to the new setBatch() method.

Also a small adjustment to _insertBatch() method will be to use the new getValues() method.

I didn't want to change them yet so you guys could see what is going on here before the change.

[iRedds]

1. CI4 follows PSR-12 so you need to limit lines to 80 (120) characters (https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/styleguide.md#lines).
2. I recommend wrapping SQL code in tests in Nowdoc (or Heredoc). This will make it easier to read. It also seems to me that it is worth separating the SQL generation tests into separate methods for each driver.
3. It is better to separate life tests and SQL generation tests, according to the same principle as it is now in the repository.
4. Need tests for exceptions thrown in batchExecute().
5. I think the _upsertBatch() method of the SQLite and Postgre drivers should throw an exception if the $constraints variable is empty. Otherwise, the method becomes a duplicate of insertBatch()

[MGatner]

Very nice work on this feature, much easier to review after splitting apart. @iRedds gave a great review so I will refrain, but I'm here watching.

[sclubricants]

1. CI4 follows PSR-12 so you need to limit lines to 80 (120) characters
2. I recommend wrapping SQL code in tests in Nowdoc (or Heredoc). This will make it easier to read. It also seems to me that it is worth separating the SQL generation tests into separate methods for each driver.
3. It is better to separate life tests and SQL generation tests, according to the same principle as it is now in the repository.
4. Need tests for exceptions thrown in batchExecute().
5. I think the _upsertBatch() method of the SQLite and Postgre drivers should throw an exception if the $constraints variable is empty. Otherwise, the method becomes a duplicate of insertBatch()

@iRedds

1. Done
2. Nowdoc Done - I don't see separating all that beneficial. I feel this would add unnecessary duplication. You know which one fails by which test fails.
3. I'm not sure what your talking about here. There are some tests at tests\system\Database\Builder but these tests use mock connection and do not test all the databases. Also, I'm testing the method getCompiledUpsert() and not explicitly running testMode(). Another issue is that the query generated is predicated on the results of index queries.. so a live test is needed.
4. I have now refactored the insertBatch() method to use executeBatch(). The existing tests should provide code coverage now.
5. Done - I was back and forth as to this matter. You settled it. The same could be said for Mysqli but then you would force an extra query on all the calls to check if a constraint exists for the table and the included fields.

[iRedds]

In the context of individual SQL code generation tests, see how the class is implemented https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Database/Live/OCI8/LastInsertIDTest.php
SQL for Postgre and SQLite is the same. SQL for Oracle and MSSQL are similar for the most part. Therefore, it is possible to confuse what exactly caused the error.
I think we need to hear the opinion of the maintainers on this issue.

MySQL does not depend on indexes (constraint) in the same way as other DBMSs for upsert queries. Honestly, I don’t even know if it makes sense in other DBMSs to perform search operations for intexes with an empty $constraints variable.

You need to add a description to the documentation and changelog.

[sclubricants]

If the test "PHPUnit / PHP 8.1 - Postgre (pull_request)" fails the test then you know that the issue is with Postgre. Or if you are running locally whichever database assigned as "tests".

The live/driver directory is for tests which have no counterpart in the other drivers. In this case the same call returns a value for all of them.

I don’t even know if it makes sense in other DBMSs to perform search operations for intexes with an empty $constraints variable.

My original conception was to have all the DBMSs work in the same way as MySQL. However some of them don't allow using multiple constraints at one time. So I changed to pick up the first constraint in order.. primary keys first and then unique indexes. Originally I didn't have the option to manually set constraints but felt it was a must so I added this feature. For most purposes the auto method of assigning a constraint works quite well though and simplifies writing code.

[sclubricants]

A question for you guys:

I created the method setBatch() which is a copy of setInsertBatch(). The reasoning was to disassociate the method from insert to be used with upsert or both insert and upsert. The way the codes sits now setInsertBatch() is an alias of setBatch().

So currently you could do:

$this->db->table('user')->setBatch($userData)->upsertBatch();

The question is, we could forgo the setBatch() method and just use the setInsertBatch() method with upserts.

$this->db->table('user')->setInsertBatch($userData)->upsertBatch();

What do you guys think?

[sclubricants]

Here is a sneak peak at my next installment.. should you guys accept this PR.

        // upsert from query rather than data set
        $fromQuery = $db->table('user_loggin')
            ->select('users.id, MAX(user_loggin.login_date) AS last_login')
            ->join('users','users.id = user_loggin.userid','inner')
            ->groupBy('users.id');
            
        $builder = $db->table('users')
            ->fromQuery($fromQuery)
            ->onConstraint('id')
            ->updateFields('last_login')
            ->upsert();

        // same can work with inserts
        $builder = $db->table('table_name')
            ->fromQuery($fromQuery)
            ->ignore(true)
            ->insert();
            
        // can work with raw SQL too
        $fromQuery = <<<'SQL'
                SELECT users.id, MAX(user_loggin.login_date) AS last_login
                FROM user_login
                INNER JOIN users ON users.id = user_login.userid
                GROUP BY users.id
            SQL;
        
        $builder = $db->table('users')
            ->fromQuery($fromQuery)
            ->onConstraint('id')
            ->updateFields('last_login')
            ->upsert();

[iRedds]

What do you guys think?

To be honest, the setBatch() method has a very general name structure. If you look at the surface and do not refer to the documentation, then you can assume that this method can be used for any batch changes to the database.
That is, a developer may natively think that the following code will work:

$builder->setBatch($data)->updateBatch();

I think it would be more logical to see the method name as setUpsertBatch().

At the same time, the setInsertBatch() method explicitly defines the behavior of batch insertion.
Which can also lead to misunderstanding.

And if I had to choose between the options you suggested, I would choose the third one.

$builder->set(data); // ->insert()/update()/upsert() Both for single insert/update and batch.

[sclubricants]

The set() method already works. It doesnt work for multiple records. Hense the setBatch() as its sister function.

I agree with setUpsertBatch(). I guess I was thinking to consolidate them but that would cause other issues.

It would be nice if set() did them all.

[sclubricants]

@lonnieezell @paulbalandan @MGatner @michalsn @samsonasik @kenjis
Any comments on this PR?

[sclubricants]

@kenjis I screwed this up trying to fix a conflict. Any idea of what to do?

[kenjis]

What command did you run?

[sclubricants]

I was using github desktop. I ran rebase UpsertUpsertBatch on 4.3

[sclubricants]

It was suppose to add like 11 commits on top of 4.3

[kenjis]

I've pushed my backup of your branch in my repository:
https://github.com/kenjis/CodeIgniter4/commits/UpsertUpsertBatch

[sclubricants]

So how do I use your backup?

[kenjis]

I recommend you create a new branch as a backup before running destructive commands:

$ git branch UpsertUpsertBatch.bk

Fetch my repository:

$ git remote add kenjis [email protected]:kenjis/CodeIgniter4.git
$ git fetch kenjis

Remove the existing branch:

$ git switch develop
$ git branch -D UpsertUpsertBatch

Checkout:

$ git branch UpsertUpsertBatch kenjis/UpsertUpsertBatch
$ git switch UpsertUpsertBatch

[sclubricants]

$ git fetch kenjis
[email protected]: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

[sclubricants]

I have the files. I'll just create a new branch and new PR.

[kenjis]

If you don't use SSH, use HTTPS link.

$ git remote add kenjis https://github.com/kenjis/CodeIgniter4.git