Issue 123 Database Verification Errors On Setup - edmozley/freeitsm GitHub Wiki

Issue #123 โ€” three errors when running Database Verification

Reported by chris18890 ยท Fixed in #1413 and #1414 ยท Affects every installation up to that point

Three errors on System โ†’ Database Verification, on an upgrade and on a fresh install alike:

knowledge_gap_tickets:         Failed to create table: SQLSTATE[42000] 1072
                               Key column 'id' doesn't exist in table
knowledge_gap_cluster_tickets: Failed to create table: SQLSTATE[42000] 1072
                               Key column 'id' doesn't exist in table
Schema column drift:           ticket_audit.analyst_id
                               (freeitsm.sql has INT NULL, Verification expects INT NOT NULL)

Two separate causes. The second one is much worse than it looks.

1 and 2 โ€” the table builder assumes every table has an id

db_verify.php creates a missing table from the $schema array, and finishes the statement with a primary key it works out like this:

if ($tableName === 'knowledge_article_tags')      { PRIMARY KEY (article_id, tag_id) }
elseif ($tableName === 'task_tag_map')            { PRIMARY KEY (task_id, tag_id) }
elseif (isset($primaryKeys[$tableName]))          { PRIMARY KEY ($primaryKeys[$tableName]) }
else                                              { PRIMARY KEY (id) }          // โ† here

Neither knowledge-gap table has an id column. knowledge_gap_tickets is keyed by ticket_id โ€” one row per ticket, which is the point of it โ€” and knowledge_gap_cluster_tickets is a join table keyed by (cluster_id, ticket_id). Both fell through to the last branch, and MySQL refused a key on a column that does not exist.

๐Ÿ”‘ The warning was already there, and was not enough

The $primaryKeys map carries this comment, written when two earlier tables did exactly the same thing:

โš ๏ธ A new table whose PK is not literally id MUST be listed here.

Two more tables were added without being listed. A comment cannot enforce anything, and this one had already failed once before it failed again.

The shape was part of the problem: a composite key had to be written in two places โ€” null in the map, and then the table named again in the builder's if/elseif. Two edits to add one table, with the second one silent if you forget it. $primaryKeys now takes a string or an array and the builder has one rule:

$pk     = $primaryKeys[$tableName] ?? 'id';
$pkCols = is_array($pk) ? $pk : [$pk];
$colDefs[] = 'PRIMARY KEY (`' . implode('`, `', $pkCols) . '`)';

The check that should have existed

Every table in $schema with no id column and no entry in $primaryKeys will fail this way. That is a static question, so it can simply be asked:

foreach ($schema as $table => $cols) {
    if (array_key_exists('id', $cols))        continue;
    if (isset($primaryKeys[$table]))          continue;
    echo "would fail to create: $table\n";
}

Run across all 268 tables it named exactly the two in the report, and nothing else. Worth running whenever a table is added.

3 โ€” ๐Ÿ”ด not a warning. A fresh install brought back a bug that was already fixed

The drift note looked cosmetic. It was not.

The schema is declared in three places, and they answer different questions:

Where What it is for
database/freeitsm.sql builds a new database
includes/db_verify_schema.php ($schema) the columns Verification creates and compares
a migration block in api/system/db_verify.php changes an existing column in place

#1391 made ticket_audit.analyst_id nullable, because a workflow writes history entries that no analyst made. It updated the first and the third. It did not update the second.

And the migration runs before the tables are created โ€” line 223 against line 289. So on a fresh install:

  1. the migration looks for ticket_audit.analyst_id, finds no such table yet, and does nothing;
  2. the table is then created from $schema โ€” as INT NOT NULL;
  3. nothing else in that run touches it.

Proved by building ticket_audit from $schema alone in a scratch database and running the workflow engine's own INSERT, verbatim:

BEFORE  (a) FRESH install : ๐Ÿ”ด FAILED โ€” 1048 Column 'analyst_id' cannot be null
AFTER   (a) FRESH install : INSERT OK, analyst_id stored as NULL

So anyone installing FreeITSM fresh got GH #120 back โ€” a workflow could not add a note to a ticket โ€” until they happened to run Database Verification a second time, at which point the table existed and the migration corrected it. An existing installation was never affected, which is why it went unnoticed.

๐Ÿ”‘ A schema change is not done until all three declarations agree. The drift guard exists precisely to say so, it was saying so, and it was read as a tidiness warning rather than as a report that new installs were being built wrong.

How it was verified

  • The static audit above: 0 of 268 tables would now fail to create, down from 2.
  • Both tables dropped and recreated through the real endpoint, coming back with PRIMARY KEY (ticket_id) and PRIMARY KEY (cluster_id, ticket_id) exactly as freeitsm.sql declares them. The three existing rows were copied out first and restored after.
  • A full run afterwards: 268 tables, 0 errors, 0 updates โ€” idempotent, and the drift note gone.
  • The #120 regression check run both ways: on a fresh schema built from $schema, and against the live database inside a transaction that was rolled back (161 audit rows before and after).

Related