Simpletest test_id assignment broken (critical on PostgreSQL)

I don't believe I have this issue.

Anyone else confirm?

I noticed the field name seemed to be weird some time ago, but forgot the check into it. Appears this was messed up after batch API patch.

  $schema['simpletest_test_id'] = array(
    'description' => t('Stores simpletest test IDs.'),
    'fields' => array(
      'test_id'  => array(
        'type' => 'serial',
        'not null' => TRUE,
        'description' => t('Primary Key: Unique simpletest ID.'),
      ),
    ),
    'primary key' => array('test_id'),
  );

has message_id instead, even though it was used and intended to be test_id as the field.

The following code demonstrates its use

db_query('INSERT INTO {simpletest_test_id} VALUES (default)');
$test_id = db_last_insert_id('simpletest_test_id', 'test_id');

The reason why the major, mysql users, didn't notice is due to the db_last_insert_id function. In mysql it looks like

function db_last_insert_id($table, $field) {
  return db_result(db_query('SELECT LAST_INSERT_ID()'));
}I noticed the field name seemed to be weird some time ago, but forgot the check into it. Appears this was messed up after batch API patch.

<?php
  $schema['simpletest_test_id'] = array(
    'description' => t('Stores simpletest test IDs.'),
    'fields' => array(
      'test_id'  => array(
        'type' => 'serial',
        'not null' => TRUE,
        'description' => t('Primary Key: Unique simpletest ID.'),
      ),
    ),
    'primary key' => array('test_id'),
  );

has message_id instead, even though it was used and intended to be test_id as the field.

The following code demonstrates its use

db_query('INSERT INTO {simpletest_test_id} VALUES (default)');
$test_id = db_last_insert_id('simpletest_test_id', 'test_id');

The reason why the major, mysql users, didn't notice is due to the db_last_insert_id function. In mysql it looks like

function db_last_insert_id($table, $field) {
  return db_result(db_query('SELECT LAST_INSERT_ID()'));
}

and doesn't use $field, where as in pgsql:

function db_last_insert_id($table, $field) {
  return db_result(db_query("SELECT CURRVAL('{" . db_escape_table($table) . "}_" . db_escape_table($field) . "_seq')"));
}

which does use the $field variable.

The patch fixes the schema.

Doesn't break tests as I just ran them all.

Use hook_update?

That's not how to do an update function. We want it to just change the name of the column, not re-install the whole table.

Since the entire table is the single field I would vote for destroying table (needs to be added) and re-creating it. Otherwise the documentation isn't clear on how to edit a field, it only shows how to add table or field. So based on that wouldn't we delete table and recreate? If so I'll just add the delete table to the patch.

Untested, but added table remove.

Unfortunately, removing the table will not work, as we can't reset the serial, since the {simpletest} table may already contain entries for lower numbers, which will mess up testing quite a bit.

Not sure if update is very useful in this case, but results can just be cleared to prevent issue. If this isn't "acceptable" then please come out and state what is commonly used. I think this solution is fine since this is a development version and the simpletest data should be fine to clear.

Just use db_change_field() please. That's certainly part of what it's meant for, renaming fields.

Clarifying the title and bumping to critical.

Here is a test case for this. Tested on MySQL only (passes), and when using a fixed $test_id (fails).

db_change_field
http://api.drupal.org/api/function/db_change_field/6

IMPORTANT NOTE: To maintain database portability, you have to explicitly recreate all indices and primary keys that are using the changed field.

  db_drop_primary_key($ret, 'simpletest_test_id');
  db_change_field($ret, 'simpletest_test_id', 'message_id', 'test_id',
                  array('type' => 'serial', 'not null' => TRUE),
                  array('primary key' => array('test_id')));

When running it that way the removal of primary key gives error since it is autoincrament.

This is somewhat of a waste of time considering the previous solution works and we don't have data we need to maintain so a "perfect" update is rather unnecessary.

@boombatower: fair enough. Does the droping/recreating solution works on PostgreSQL? :)

I don't know why it wouldn't, but someone with PostgreSQL to test would be appreciated. I'll comment the update function and role you test into patch and post it.

This patch contains cleaned up test and commented update function.

Won't it just run the schema hook which has the correct schema, it won't even run the update hooks?

And SimpleTest isn't in core as of 6.x.

I'm fairly new to writing updates, but it would seem counter intuitive if you had to update previous updates instead of just main schema.

According to my understanding of the update API that doesn't make sense. Just to confirm this I installed SimpleTest from fresh Drupal installation and the update hooks are never run.

Meaning that 7000 doesn't need to be changed since it is only used for people running the original schema to update to 7000 version, but on install it expects the hook_schema() to contain the 7000 format.

Same goes for 7001.

As for upgrading from SimpleTest 6.x that isn't an issue since once again there is no data that need to be ported and the 6.x-2.x version of SimpleTest is a copy of 7.x whereas the 6.x-1.x is different and there is no upgrade path, as none is needed.

Lastly an update isn't even useful here since this is dev version and simpletest data can simply be trashed as it doesn't need to be maintained. I don't think we should hold up this bug fix, since this works and is more than sufficient.

Let's not spend too much time on these update functions. By the time we release Drupal 7, Simpletest will probably be renamed to Testing anyway. We only need something that would work for now, during the development cycle.

Let's get this committed. We will change the world a little later.

I think 'id' should be written as 'ID'.

My bad, rerolled.

Hmm...text slips by the eye.

We do not provide HEAD-HEAD update functions.

(Minor comment: while we are at it, and when reviewing the code, it occurred to me that we don't really explain very well how IDs are used. I think we can improve the schema definition documentation as well as other places in the code.)

@chx: I thought that was the practice, but seeing the other update function in the file made me wonder. Following your comment I removed both update functions in this patch.

@Dries: Added some documentation to the schema and where the test_id is used to query the results.

Unfortunately, this test needs a re-roll. Should be a quick re-roll though.

Re-rolled.

Thanks for the re-roll boomba. Tested, reviewed and committed to CVS HEAD.

I'm getting notice: Undefined index: test_id all the time, I'm using latest built, did a new install .. but no luck
fwiw: MySQL : 5.0.51a , PHP: 5.2.6 , Apache/2.2.8
When I look into the sessions table I see 'test_id|N;' which well ain't good I think. I'll look into this further before I set this back to active.

Update:
Delete all the debug info here, it's simply my machine and cpu going wild whenever I start simpletest (even in batch mode). Tested the same distro setup on 2 other machines and everything went fine, phew!