From 48c4b27841d22a76e364c3a471222c15fd648379 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Tue, 1 Sep 2026 20:08:34 -0500 Subject: [PATCH] Write NULL, not 0, into location's foreign-key column The core half of this is FOGProject/fogproject#1655: "Clear on all" on Image failed because the mass edit cleared `hosts`.`hostImage` to 0 and a foreign key refuses a 0 that names no row. Asked to sweep for the same shape here, and it found one. LocationDeleteMassItems::deletemassitems() cleaned up after a deleted storage node with ->update(['storagenodeID' => $arguments['itemIDs']], '', 0) where the third argument is the UPDATE DATA. FOGManagerController::update() takes an associative array of columns, or an array of them, and returns false for anything else -- so the call had ALWAYS been a silent no-op, and had it ever worked, 0 is precisely the value the constraint now refuses. `location`.`lStorageNodeID` is nullable with ON DELETE SET NULL, so it writes null now. Kept rather than left to the constraint alone: a server between deploying code and running the schema updater has the column and not the foreign key, and there this is the only thing doing the work. The storagegroup arm above it was the same no-op and is removed, because there is nothing it could ever have done. `lStorageGroupID` is NOT NULL with a RESTRICT foreign key -- confirmed on a live install -- so a storage group any location still points at cannot be deleted at all, and the database refusing it is the intended answer. A location without a storage group is not a valid location. The line read as cleanup that made the delete safe; it never ran. tests/fk-writes-are-null-not-zero.test.php gates all 23 plugin-owned columns that core's schema-constraints.php constrains, not just the three `config` ones where a sentinel was ever plausible -- enumerating only the risky half is how the next one gets missed. It also refuses update($find, $op, ) anywhere in the tree, which is the shape that made this a no-op and which no caller can have meant. Comments are stripped with the tokenizer first, so documenting a defect does not trip the check that catches it. The field-name mapping is asserted, not trusted: renaming a field behind a constrained column fails here rather than quietly making every grep match nothing. Four mutations were run. The first version of the scalar-update check did NOT catch the real bug -- `\[[^\]]*\]` cannot span the inner `]` in `$arguments['itemIDs']`, so it skipped the exact line it existed for and passed. Bounded on `;` instead, it goes red. Also checked and clean: capone already migrated cImageID/cOSID off the 0 sentinel and guards with isValid() before writing; tasktypeedit and taskstateedit own no tables and delete through core's shared path, which already explains a refusal via ConstraintViolation::explain(). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM --- .../src/Hooks/LocationDeleteMassItems.php | 31 ++- tests/fk-writes-are-null-not-zero.test.php | 241 ++++++++++++++++++ 2 files changed, 270 insertions(+), 2 deletions(-) create mode 100644 tests/fk-writes-are-null-not-zero.test.php diff --git a/location/src/Hooks/LocationDeleteMassItems.php b/location/src/Hooks/LocationDeleteMassItems.php index 9c91c5e..1a1e4cb 100644 --- a/location/src/Hooks/LocationDeleteMassItems.php +++ b/location/src/Hooks/LocationDeleteMassItems.php @@ -76,10 +76,37 @@ public function deletemassitems($arguments) ]; break; case 'storagegroup': - self::getClass('LocationManager')->update(['storagegroupID' => $arguments['itemIDs']], '', 0); + // Nothing to do, and nothing that CAN be done. `location`. + // `lStorageGroupID` is NOT NULL with a RESTRICT foreign key + // to `nfsGroups` (ADR 0031), so a storage group that any + // location still points at cannot be deleted at all -- the + // database refuses it, which is the intended answer. A + // location without a storage group is not a valid location. + // + // This arm used to call + // ->update(['storagegroupID' => $ids], '', 0) + // which passed 0 as the UPDATE DATA. FOGManagerController:: + // update() takes an associative array or an array of them + // and returns false for anything else, so the call has + // always been a silent no-op. It read as cleanup that made + // the delete safe; it never ran. break; case 'storagenode': - self::getClass('LocationManager')->update(['storagenodeID' => $arguments['itemIDs']], '', 0); + // NULL, not 0. The column is nullable and its foreign key is + // ON DELETE SET NULL, so "no node" IS null -- a 0 names a + // storage node that does not exist and the constraint + // rejects it. Same silent no-op as above before this: the + // third argument was the bare integer 0, not a map. + // + // Kept rather than left to the constraint, because a server + // between deploying this code and running the schema updater + // has the column and not the foreign key, and there this is + // the only thing doing the work. + self::getClass('LocationManager')->update( + ['storagenodeID' => $arguments['itemIDs']], + '', + ['storagenodeID' => null] + ); break; case 'location': $arguments['removeItems']['locationassociation'] = [ diff --git a/tests/fk-writes-are-null-not-zero.test.php b/tests/fk-writes-are-null-not-zero.test.php new file mode 100644 index 0000000..60ee374 --- /dev/null +++ b/tests/fk-writes-are-null-not-zero.test.php @@ -0,0 +1,241 @@ +update(['storagenodeID' => $ids], '', 0) + * + * where the third argument is the UPDATE DATA. FOGManagerController::update() + * accepts an associative array or an array of them and returns false for + * anything else, so the call had always been a silent no-op -- and had it + * ever worked, 0 is precisely the value the constraint now refuses. + * + * WHAT IS CHECKED. Three plugin columns are constrained by core's + * commons/schema-constraints.php, and their field names come from each + * plugin's own $databaseFields: + * + * capone.cImageID -> Capone::imageID + * capone.cOSID -> Capone::osID + * location.lStorageNodeID -> Location::storagenodeID + * + * (subnetgroup.sgGroupID is classified `poly` and takes no constraint.) + * + * For each, no write of a literal 0 or empty string, and no update() whose + * data argument is a bare scalar -- the shape that made the original a + * no-op, and which no caller can ever have meant. + * + * PINNED BY HAND, like its two sibling tests, and for the same reason they + * state: bin/fetch-plugins.sh fetches this repository on its own and cannot + * assume a fogproject checkout is anywhere nearby, so the constraint file + * cannot be read. Adding a constrained plugin column means adding it here. + * + * Usage: php tests/fk-writes-are-null-not-zero.test.php + * Exit status 0 = pass, 1 = fail. + * + * PHP version 7.4+ + * + * @category Tests + * @package FOGProject + * @author Tom Elliott + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +$root = dirname(__DIR__); +$checks = 0; +$failures = []; + +// Every plugin-owned column that core's schema-constraints.php constrains, +// column => [plugin, Item class, field name as that Item's $databaseFields +// keys it]. +// +// The `config` ones at the top are where a 0 sentinel was ever plausible -- +// a nullable reference whose "none" used to be 0. The `junction` ones below +// cannot hold a sentinel by construction (an association row either exists or +// it does not), but they are covered anyway: a 0 in one of them is an orphan +// naming a row that does not exist, which is the same refusal by the same +// constraint, and enumerating only the "risky" half is how the next one gets +// missed. +// +// ldapUserGrant.lugTargetID and oidcUserGrant.ougTargetID are deliberately +// absent: both are classified `poly` (the target table is named by a sibling +// column), so no constraint is expressible and there is nothing to comply +// with. +$constrained = [ + // config -- a real reference that can be absent + 'cImageID' => ['capone', 'Capone', 'imageID'], + 'cOSID' => ['capone', 'Capone', 'osID'], + 'lStorageGroupID' => ['location', 'Location', 'storagegroupID'], + 'lStorageNodeID' => ['location', 'Location', 'storagenodeID'], + // junction -- one row per association + 'laLocationID' => ['location', 'LocationAssociation', 'locationID'], + 'laHostID' => ['location', 'LocationAssociation', 'hostID'], + 'oaOUID' => ['ou', 'OUAssociation', 'ouID'], + 'oaHostID' => ['ou', 'OUAssociation', 'hostID'], + 'sgGroupID' => ['subnetgroup', 'SubnetGroup', 'groupID'], + // satellite / junction, ldap + 'lgServerID' => ['ldap', 'LDAPGroup', 'serverID'], + 'lgraGroupID' => ['ldap', 'LDAPGroupRoleAssociation', 'ldapgroupID'], + 'lgraRoleID' => ['ldap', 'LDAPGroupRoleAssociation', 'roleID'], + 'lgugGroupID' => ['ldap', 'LDAPGroupUserGroupAssociation', 'ldapgroupID'], + 'lgugUserGroupID' => ['ldap', 'LDAPGroupUserGroupAssociation', 'usergroupID'], + 'lugUserID' => ['ldap', 'LDAPUserGrant', 'userID'], + // satellite / junction, oidc + 'ogProviderID' => ['oidc', 'OIDCGroup', 'providerID'], + 'oiProviderID' => ['oidc', 'OIDCIdentity', 'providerId'], + 'oiUserID' => ['oidc', 'OIDCIdentity', 'userId'], + 'ograGroupID' => ['oidc', 'OIDCGroupRoleAssociation', 'oidcgroupID'], + 'ograRoleID' => ['oidc', 'OIDCGroupRoleAssociation', 'roleID'], + 'ogugGroupID' => ['oidc', 'OIDCGroupUserGroupAssociation', 'oidcgroupID'], + 'ogugUserGroupID' => ['oidc', 'OIDCGroupUserGroupAssociation', 'usergroupID'], + 'ougUserID' => ['oidc', 'OIDCUserGrant', 'userID'], +]; + +// The mapping is asserted rather than trusted: if a plugin renames the field +// behind one of these columns, every grep below silently stops looking at +// anything and the file passes while covering nothing. +foreach ($constrained as $column => list($plugin, $item, $field)) { + $checks++; + $itemFile = $root . '/' . $plugin . '/src/Items/' . $item . '.php'; + $src = (string)@file_get_contents($itemFile); + if ('' === $src) { + $failures[] = "$plugin: $item.php is missing, so the mapping this" + . " test scans for ($field => $column) cannot be confirmed"; + continue; + } + if (false === strpos($src, "'" . $field . "' => '" . $column . "'")) { + $failures[] = "$plugin: $item no longer maps $field => $column;" + . ' this test is looking for a field that does not exist'; + } +} + +/* + * Comments are stripped before anything is matched, with the tokenizer rather + * than a regex. This test's own docblock quotes the broken call it exists to + * catch, and so does the comment that replaced that call -- a scan over raw + * text flags both, which would make the only way to document a defect be to + * stop describing it. + */ +$stripComments = static function ($src) { + $out = ''; + foreach (token_get_all($src) as $token) { + if (is_array($token)) { + if (T_COMMENT === $token[0] || T_DOC_COMMENT === $token[0]) { + // Keep the newlines so nothing on separate lines is joined + // into a match that does not exist in the code. + $out .= str_repeat("\n", substr_count($token[1], "\n")); + continue; + } + $out .= $token[1]; + continue; + } + $out .= $token; + } + return $out; +}; + +$phpFiles = []; +$it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($root, \FilesystemIterator::SKIP_DOTS) +); +foreach ($it as $file) { + $path = $file->getPathname(); + if (substr($path, -4) !== '.php') { + continue; + } + // The suite's own fixtures quote these shapes on purpose. + if (false !== strpos($path, '/tests/') + || false !== strpos($path, '/vendor/') + ) { + continue; + } + $phpFiles[] = $path; +} +$checks++; +if (count($phpFiles) < 20) { + $failures[] = 'found only ' . count($phpFiles) . ' plugin PHP files;' + . ' the walk is not reaching the tree'; +} + +foreach ($constrained as $column => list($plugin, $item, $field)) { + $q = preg_quote($field, '/'); + // ->set('field', 0) / ->set('field', '') and 'field' => 0 / => '' + $patterns = [ + "/->set\(\s*'" . $q . "'\s*,\s*(0|'')\s*\)/", + "/'" . $q . "'\s*=>\s*(0|'')\s*[,\)\]]/", + ]; + foreach ($phpFiles as $path) { + $src = $stripComments((string)file_get_contents($path)); + if (false === strpos($src, $field)) { + continue; + } + foreach ($patterns as $pattern) { + $checks++; + if (preg_match($pattern, $src, $hit)) { + $failures[] = str_replace($root . '/', '', $path) + . ": writes " . trim($hit[0]) + . " -- `$column` carries a foreign key, so 0 and '' name" + . ' a row that does not exist. Write null.'; + } + } + } +} + +/* + * The shape that made the original a no-op rather than a wrong write: + * update($find, $op, 0). A bare scalar as the third argument is never + * meaningful -- update() answers false without touching anything -- so it is + * always either a typo for an array or a misunderstanding of the signature. + * Checked across every plugin, not just the constrained columns, because the + * failure is invisible either way. + */ +foreach ($phpFiles as $path) { + $src = $stripComments((string)file_get_contents($path)); + if (false === strpos($src, '->update(')) { + continue; + } + $checks++; + // `[^;]*?` for the first argument, NOT `\[[^\]]*\]`. The find-map is + // routinely `['storagenodeID' => $arguments['itemIDs']]`, whose inner `]` + // ends a negated-class match early -- so the tighter pattern skipped the + // exact line this check exists to catch, and passed. Bounded on `;` so a + // match cannot run past the end of the statement. + if (preg_match( + "/->update\([^;]*?,\s*'[^']*'\s*,\s*(?!\[)(0|''|null|false)\s*\)/s", + $src, + $hit + )) { + $failures[] = str_replace($root . '/', '', $path) + . ': ' . preg_replace('/\s+/', ' ', trim($hit[0])) + . ' -- update() takes an associative array of columns as its' + . ' third argument and returns false for a scalar, so this' + . ' silently does nothing.'; + } +} + +if (count($failures)) { + fwrite(STDERR, "FAIL: a plugin writes a sentinel into a foreign-key column:\n"); + foreach (array_unique($failures) as $f) { + fwrite(STDERR, " - $f\n"); + } + fwrite( + STDERR, + sprintf("%d of %d checks failed\n", count(array_unique($failures)), $checks) + ); + exit(1); +} + +printf("PASS plugin foreign-key writes are null, not 0: %d checks\n", $checks); +exit(0);