Unxpected behaviour with $this->db->update_batch() and $this->db->insert_batch()

[eluser]JustinWyllie[/eluser]
Hi

In my database config file I have set db_debug to false.

If a call to $this->db->update() fails CI doesn't output an error page - I just check for the return value of ->update() being false and handle it myself. This is exactly what I want.

However with $this->db->update_batch() I am finding that the return value from the function is null. It is even null if it succeeds. This is not what I want as I can't check for an error.

I would add; on testing, the behaviour for $this->db->insert_batch() is different. This function returns true in either case; whether it succeeds or not.

Clearly this behaviour is not consistent and for that reason alone is a bug no?

In summary:

$this->db->update_batch() has no return value whether it succeeds or fails.
$this->db->insert_batch() returns true whether it succeeds or fails.

This is against mysql 5.0.37 and using the mysqli dbdriver.

Also for information, the practical solution to my problem seems to be to check for
$this->db->_error_message();


Thanks

--JustinWyllie

[eluser]Aken[/eluser]
You bring up a valid concern. Let me explain a little bit about what happens in those two methods, though.

Both batch methods split up the queries into groups of 100 rows. So if you have 550 rows of data to insert or update, six queries will be run. While it's certainly possible to track if any of those queries do fail, it is a little misleading to say that the entire method failed, when only one actual query didn't perform correctly.

For things like this, you'll probably want to be using transactions anyway, if you don't want to commit all the changes unless they all work. But people don't always take that precaution.

I agree that something could be done to help make the returned values more consistent with their single brethren, just not sure of what. I suggest creating an issue on Github, maybe even a pull request with your own solution suggestion, and see what kind of ideas and feedback come from it.

[eluser]JustinWyllie[/eluser]
Thanks.

Following what you've explained I tested update_batch() with the obvious case of an error in the first batch of 100 and not in the second. (I'm not 100% certain the batches are in fact per one hundred; I would have to check). In this case _error_message() was clear - presumably the second batch was ok so that is the final result after the call to update_batch even though the first batch of 100 failed and will have produced an error_message.

My suggestion then to use _error_message is wrong. The correct way to use update_batch is as you suggest to use transactions:

Code:

$this->db->trans_start();
$this->db->update_batch('table', $data, 'keyfield');
$this->db->trans_complete();

if ($this->db->trans_status() === FALSE)
{
     return false;
}
else
{
    return true;
}

I wonder if the reason this isn't in the documentation for update_batch is because CI is trying to support people who don't have transaction enabled databases?

Given that it works as you suggest with transactions and I have InnoBD tables my motivation to look into setting a possible return value (a string of 'ok', 'failed' and 'partial' perhaps?) is not high. And - what good would it do them?


Finally, as a footnote, I also found another anomaly. Tested with update_batch only. If the error is in the naming of the key field the call produces a database error page even if db_debug is off in config. I agree this is a very unlikely scenario.

[eluser]Aken[/eluser]
Can you give a specific use case (aka code) for the update_batch error anomaly? I don't want to try and reproduce it, lol.

[eluser]JustinWyllie[/eluser]

Code:

//in config/database.php
$db['default']['db_debug'] = FALSE;

Code:

create table test (
    keyid int not null auto_increment primary key,
    field varchar(255)
);
insert into test values(null, 'test1');

Code:

//field name is wrong; result is no change to db, no response output as expected.
$test = array(
  array('keyid'=>1, 'fieldXXX'=>'test2')
);
$this->db->trans_start();
$this->db->update_batch('test', $test, 'keyid');
$this->db->trans_complete();

Code:

// name of key field for update is wrong; result is no change to db, get the Standard
//Database error page with 'One or more rows submitted for batch updating
//is missing the specified index.'
$test = array(
  array('keyidXXX'=>1, 'field'=>'test2')
);
$this->db->trans_start();
$this->db->update_batch('test', $test, 'keyid');
$this->db->trans_complete();

I think it is totally trivial.

What is actually happening is this:

in system/database/DB_active_rec.php update_batch() calls set_update_batch() to set up the keys for the update. set_update_batch() has a little routine which sets keys/values. It finds that the there is no index in the data array matching the 3rd param originally passed to update_batch and produces an error:

Code:

if ($index_set == FALSE)
{
   return $this->display_error('db_batch_missing_index');
}

There is no check for :

Code:

if ($this->db_debug)

hence an error whatever the db config setting.

If there was a check for db_debug and if it was allowed to continue without an error $ar_set would not be set and in update_batch the check for an empty array $this->ar_set would work and update_batch() would return false.

So it looks like there is a fix to be made here:

in function set_update_batch():

Code:

//add check for db_debug and only set ar_set if it can be set
if (($index_set == FALSE)  )
{
   if ($this->db_debug) {
       return $this->display_error('db_batch_missing_index');
   }
}
else
{
$this->ar_set[] = $clean;

}

It would need someome to check that over I think.

--Justin